Use relative bounding boxes throughout Chrome accessibility

Use relative bounding boxes throughout Chrome accessibility

After a series of changes that added support in Blink for relative bounding
boxes for accessible objects, this change switches all of the code in Chrome
to use these relative bounding boxes.

This significantly cleans up the code to convert from local to global
coordinates. Instead of a complicated loop with lots of special cases
for things like out-of-process iframes, it's now a pretty straightforward
process of walking up the tree and applying offsets and transforms.

After this change lands successfully, we can delete the old absolute bounds
computation code from Blink.

BUG=618120
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/051715aa0384587adcb0614e5404720ad75757ca
Cr-Commit-Position: refs/heads/master@{#412054}

[dmazzoni, 2016-08-05 20:34:47]

Description was changed from

==========
Use relative bounding boxes throughout Chrome accessibility

After a series of changes that added support in Blink for relative bounding
boxes for accessible objects, this change switches all of the code in Chrome
to use these relative bounding boxes.

This significantly cleans up the code to convert from local to global
coordinates. Instead of a complicated loop with lots of special cases
for things like out-of-process iframes, it's now a pretty straightforward
process of walking up the tree and applying offsets and transforms.

After this change lands successfully, we can delete the old absolute bounds
computation code from Blink.

BUG=618120
==========

to

==========
Use relative bounding boxes throughout Chrome accessibility

After a series of changes that added support in Blink for relative bounding
boxes for accessible objects, this change switches all of the code in Chrome
to use these relative bounding boxes.

This significantly cleans up the code to convert from local to global
coordinates. Instead of a complicated loop with lots of special cases
for things like out-of-process iframes, it's now a pretty straightforward
process of walking up the tree and applying offsets and transforms.

After this change lands successfully, we can delete the old absolute bounds
computation code from Blink.

BUG=618120
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dmazzoni, 2016-08-05 20:35:04]

The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-05 20:35:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dmazzoni, 2016-08-05 20:36:33]

dmazzoni@chromium.org changed reviewers:
+ aboxhall@chromium.org

[commit-bot: I haz the power, 2016-08-05 21:47:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-05 21:47:46]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[aboxhall, 2016-08-12 14:51:22]

I suspect this will fix http://crbug.com/635329 - what do you think?

[aboxhall, 2016-08-12 16:05:10]

https://codereview.chromium.org/2217363002/diff/1/chrome/browser/resources/ch...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs
(right):

https://codereview.chromium.org/2217363002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs:84:
<div role="group">
Why this change?

https://codereview.chromium.org/2217363002/diff/1/chrome/renderer/extensions/...
File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right):

https://codereview.chromium.org/2217363002/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:117: static
gfx::Rect ComputeGlobalNodeBounds(TreeCache* cache, ui::AXNode* node) {
Is there a reason this needs to return Rect rather than RectF?

Also, can you refresh my memory on why we need this code both here and in
BrowserAccessibility?

https://codereview.chromium.org/2217363002/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:140: int sx =
0;
Maybe s/sx/scroll_x/ and similarly for sy, while we're here?

https://codereview.chromium.org/2217363002/diff/1/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/2217363002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:1043: gfx::Rect
bounds = GetScreenBoundsRect();
Ahhh this is so great.

https://codereview.chromium.org/2217363002/diff/1/content/test/data/accessibi...
File
content/test/data/accessibility/html/iframe-transform-cross-process-expected-blink.txt
(right):

https://codereview.chromium.org/2217363002/diff/1/content/test/data/accessibi...
content/test/data/accessibility/html/iframe-transform-cross-process-expected-blink.txt:5:
++iframe pageLocation=(0, 300) transform
Where does this ("transform") come from now?

https://codereview.chromium.org/2217363002/diff/1/ui/accessibility/ax_node.cc
File ui/accessibility/ax_node.cc (right):

https://codereview.chromium.org/2217363002/diff/1/ui/accessibility/ax_node.cc...
ui/accessibility/ax_node.cc:27: if (transform)
Should this reset .transform to nullptr otherwise?

https://codereview.chromium.org/2217363002/diff/1/ui/accessibility/ax_node.h
File ui/accessibility/ax_node.h (right):

https://codereview.chromium.org/2217363002/diff/1/ui/accessibility/ax_node.h#...
ui/accessibility/ax_node.h:47: void SetLocation(int offset_container_id,
Maybe add to the description to explain the three arguments?

https://codereview.chromium.org/2217363002/diff/1/ui/accessibility/ax_relativ...
File ui/accessibility/ax_relative_bounds.cc (right):

https://codereview.chromium.org/2217363002/diff/1/ui/accessibility/ax_relativ...
ui/accessibility/ax_relative_bounds.cc:48: bool
AXRelativeBounds::operator!=(const AXRelativeBounds& other) {
Why can't this delegate to operator==?

[dmazzoni, 2016-08-15 05:02:55]

On Fri, Aug 12, 2016 at 7:51 AM <aboxhall@chromium.org> wrote:

> I suspect this will fix http://crbug.com/635329 - what do you think?
>

That might be unrelated. I'm suspecting we have a bug on Windows with the
device scale factor isn't 1, like on high-res "retina" displays. It's
always unclear in situations like that whether we're supposed to return
coordinates in "true" pixels or "normal-res" pixels

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2016-08-15 05:31:10]

dmazzoni@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org, tedchoc@chromium.org, tsepez@chromium.org

[dmazzoni, 2016-08-15 05:31:11]

Adding OWNERS reviewers now.

+rdevlin.cronin for chrome_extension_messages.h
+tsepez for content/common/accessibility_messages.h
+tedchoc for content/browser/web_contents/web_contents_android.cc

https://codereview.chromium.org/2217363002/diff/1/chrome/browser/resources/ch...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs
(right):

https://codereview.chromium.org/2217363002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs:84:
<div role="group">
On 2016/08/12 at 16:05:10, aboxhall wrote:
> Why this change?

A very obscure side effect of this change was that the
bounding box of the HTML <article> element with no children
had zero height before this patch, and nonzero height with
this patch, just due to differences in what functions we're
calling to compute the bounding box. With a nonzero bounding
box ChromeVox was landing on this element and saying "Article"
now, breaking the test. I just changed it to an element that
would still show up in the accessibility tree but would have
zero height when empty.

https://codereview.chromium.org/2217363002/diff/1/chrome/renderer/extensions/...
File chrome/renderer/extensions/automation_internal_custom_bindings.cc (right):

https://codereview.chromium.org/2217363002/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:117: static
gfx::Rect ComputeGlobalNodeBounds(TreeCache* cache, ui::AXNode* node) {
On 2016/08/12 at 16:05:10, aboxhall wrote:
> Is there a reason this needs to return Rect rather than RectF?

We need to use floats for the "intermediate" computation so we
don't get way off due to rounding errors, now that we're possibly
doing matrix transformations to convert from local to global
coordinates.

But once we have global coordinates, integer pixel coordinates are
fine. I didn't see any need to also rewrite the automation API and
ChromeVox code that gets the coordinates of objects on the screen
and make them deal with floats. Once you have global coordinates,
pixels are close enough.

> Also, can you refresh my memory on why we need this code both here and in
BrowserAccessibility?

Because BrowserAccessibility is in */browser/ and Automation
Internals are in */renderer/

Ideally we should share this code, but to do that would require
some more refactoring. Basically the code in ui/accessibility
handles anything relating to a single AXTree, but not a bunch of
frames. Right now BrowserAccessibility and Automation both have
their own code to deal with multiple trees (iframes and things like
that) and join them together. It'd be nice to move more of that
to ui/accessibility, but it's nontrivial because ui/accessibility
shouldn't know anything about iframes, and content/ shouldn't know
about the "desktop" tree, and so on.

https://codereview.chromium.org/2217363002/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/automation_internal_custom_bindings.cc:140: int sx =
0;
On 2016/08/12 at 16:05:10, aboxhall wrote:
> Maybe s/sx/scroll_x/ and similarly for sy, while we're here?

Done

https://codereview.chromium.org/2217363002/diff/1/content/test/data/accessibi...
File
content/test/data/accessibility/html/iframe-transform-cross-process-expected-blink.txt
(right):

https://codereview.chromium.org/2217363002/diff/1/content/test/data/accessibi...
content/test/data/accessibility/html/iframe-transform-cross-process-expected-blink.txt:5:
++iframe pageLocation=(0, 300) transform
On 2016/08/12 at 16:05:10, aboxhall wrote:
> Where does this ("transform") come from now?

The html for this test really does have it transformed -
the first iframe is normal and the second iframe has
style="transform: scale(2.0);" (plus some other things)
on it.

However, prior to this patch, when it was a cross
process frame, that frame didn't actually know its own
transformation! The only way we got it was by asking
the frame tree in the browser process for the
transformation matrix for that iframe, and it was never
part of the AX tree. It worked, but it was really
complicated to make it work.

With this patch, it's so much simpler. The element
that actually has the style="transform: scale(2.0);"
attribute on it is the one that gets a transformation
matrix in the AX tree, and all coordinates in that subtree
get transformed automatically when converting to global
coordinates.

After this change lands I'll be able to get rid of all
of that complicated code to do it the other way!

https://codereview.chromium.org/2217363002/diff/1/ui/accessibility/ax_node.cc
File ui/accessibility/ax_node.cc (right):

https://codereview.chromium.org/2217363002/diff/1/ui/accessibility/ax_node.cc...
ui/accessibility/ax_node.cc:27: if (transform)
On 2016/08/12 at 16:05:10, aboxhall wrote:
> Should this reset .transform to nullptr otherwise?

Yes! Good catch. Fixed similar bug in AXNodeData, too.

https://codereview.chromium.org/2217363002/diff/1/ui/accessibility/ax_node.h
File ui/accessibility/ax_node.h (right):

https://codereview.chromium.org/2217363002/diff/1/ui/accessibility/ax_node.h#...
ui/accessibility/ax_node.h:47: void SetLocation(int offset_container_id,
On 2016/08/12 at 16:05:10, aboxhall wrote:
> Maybe add to the description to explain the three arguments?

Done

https://codereview.chromium.org/2217363002/diff/1/ui/accessibility/ax_relativ...
File ui/accessibility/ax_relative_bounds.cc (right):

https://codereview.chromium.org/2217363002/diff/1/ui/accessibility/ax_relativ...
ui/accessibility/ax_relative_bounds.cc:48: bool
AXRelativeBounds::operator!=(const AXRelativeBounds& other) {
On 2016/08/12 at 16:05:10, aboxhall wrote:
> Why can't this delegate to operator==?

Much better idea. Done.

[dmazzoni, 2016-08-15 05:31:16]

The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-15 05:31:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-15 06:31:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-15 06:31:47]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-08-15 16:31:11]

Messages LGTM.

[Devlin, 2016-08-15 16:35:27]

> +rdevlin.cronin for chrome_extension_messages.h
messages need an IPC reviewer.  Luckily, Tom is just that reviewer. :)  So I
think I'm unneeded here.

[Devlin, 2016-08-15 16:35:43]

Description was changed from

==========
Use relative bounding boxes throughout Chrome accessibility

After a series of changes that added support in Blink for relative bounding
boxes for accessible objects, this change switches all of the code in Chrome
to use these relative bounding boxes.

This significantly cleans up the code to convert from local to global
coordinates. Instead of a complicated loop with lots of special cases
for things like out-of-process iframes, it's now a pretty straightforward
process of walking up the tree and applying offsets and transforms.

After this change lands successfully, we can delete the old absolute bounds
computation code from Blink.

BUG=618120
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Use relative bounding boxes throughout Chrome accessibility

After a series of changes that added support in Blink for relative bounding
boxes for accessible objects, this change switches all of the code in Chrome
to use these relative bounding boxes.

This significantly cleans up the code to convert from local to global
coordinates. Instead of a complicated loop with lots of special cases
for things like out-of-process iframes, it's now a pretty straightforward
process of walking up the tree and applying offsets and transforms.

After this change lands successfully, we can delete the old absolute bounds
computation code from Blink.

BUG=618120
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Devlin, 2016-08-15 16:35:43]

rdevlin.cronin@chromium.org changed reviewers:
- rdevlin.cronin@chromium.org

[Ted C, 2016-08-15 21:04:51]

On 2016/08/15 16:35:27, Devlin wrote:
> > +rdevlin.cronin for chrome_extension_messages.h
> messages need an IPC reviewer.  Luckily, Tom is just that reviewer. :)  So I
> think I'm unneeded here.

web_contents_android.cc lgtm

[aboxhall, 2016-08-15 21:18:00]

lgtm

[aboxhall, 2016-08-15 21:18:01]

lgtm

lgtm

[dmazzoni, 2016-08-15 21:18:37]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2016-08-15 21:19:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-15 21:37:31]

Description was changed from

==========
Use relative bounding boxes throughout Chrome accessibility

After a series of changes that added support in Blink for relative bounding
boxes for accessible objects, this change switches all of the code in Chrome
to use these relative bounding boxes.

This significantly cleans up the code to convert from local to global
coordinates. Instead of a complicated loop with lots of special cases
for things like out-of-process iframes, it's now a pretty straightforward
process of walking up the tree and applying offsets and transforms.

After this change lands successfully, we can delete the old absolute bounds
computation code from Blink.

BUG=618120
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Use relative bounding boxes throughout Chrome accessibility

After a series of changes that added support in Blink for relative bounding
boxes for accessible objects, this change switches all of the code in Chrome
to use these relative bounding boxes.

This significantly cleans up the code to convert from local to global
coordinates. Instead of a complicated loop with lots of special cases
for things like out-of-process iframes, it's now a pretty straightforward
process of walking up the tree and applying offsets and transforms.

After this change lands successfully, we can delete the old absolute bounds
computation code from Blink.

BUG=618120
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-08-15 21:37:33]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-15 21:49:31]

Description was changed from

==========
Use relative bounding boxes throughout Chrome accessibility

After a series of changes that added support in Blink for relative bounding
boxes for accessible objects, this change switches all of the code in Chrome
to use these relative bounding boxes.

This significantly cleans up the code to convert from local to global
coordinates. Instead of a complicated loop with lots of special cases
for things like out-of-process iframes, it's now a pretty straightforward
process of walking up the tree and applying offsets and transforms.

After this change lands successfully, we can delete the old absolute bounds
computation code from Blink.

BUG=618120
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Use relative bounding boxes throughout Chrome accessibility

After a series of changes that added support in Blink for relative bounding
boxes for accessible objects, this change switches all of the code in Chrome
to use these relative bounding boxes.

This significantly cleans up the code to convert from local to global
coordinates. Instead of a complicated loop with lots of special cases
for things like out-of-process iframes, it's now a pretty straightforward
process of walking up the tree and applying offsets and transforms.

After this change lands successfully, we can delete the old absolute bounds
computation code from Blink.

BUG=618120
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/051715aa0384587adcb0614e5404720ad75757ca
Cr-Commit-Position: refs/heads/master@{#412054}
==========

[commit-bot: I haz the power, 2016-08-15 21:49:32]

Patchset 3 (id:??) landed as
https://crrev.com/051715aa0384587adcb0614e5404720ad75757ca
Cr-Commit-Position: refs/heads/master@{#412054}