Issue 2047873002: Add interface to get relative bounding box rect of AX objects.

Issue 2047873002: Add interface to get relative bounding box rect of AX objects. (Closed)

Created:
4 years, 6 months ago by dmazzoni

Modified:
4 years, 6 months ago

Reviewers:
chrishtr, aboxhall

CC:
aboxhall, aboxhall+watch_chromium.org, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, dglazkov+blink, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, haraken, je_julie, jochen+watch_chromium.org, kinuko+watch, mlamouri+watch-test-runner_chromium.org, nektar+watch_chromium.org, nektarios, yuzo+watch_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Add interface to get relative bounding box rect of AX objects. Currently accessibility bounding box rects are absolute coordinates within their frames. This means we have to recompute the bounding box of every AX object when part of the page scrolls or animates. To improve efficiency, this patch adds a new interface that returns the relative bounds of an object, consisting of three pieces: * Its "container" (an ancestor that scrolls or has a paint layer) * Its bounding box relative to its container * Optionally, its matrix transform relative to its container This patch just implements the new interface and tests it. Adds test_runner interfaces to access this interface at a low level (relative bounds) and high level (computed bounds) plus interfaces for accessing the scroll position. This will be followed up with a patch to use this interface instead throughout the rest of Chrome, and then delete the old interface. BUG=618120 Committed: https://crrev.com/e51c64c415fe268350cb451e4678284ae5c8433f Cr-Commit-Position: refs/heads/master@{#399230}

Patch Set 1 #

Patch Set 2 : Rebase #

Patch Set 3 : Remove incorrect dependent patchset #

Patch Set 4 : Guard DCHECK #

Patch Set 5 : Separate out one change that broke existing tests #

Total comments: 15

Patch Set 6 : Address feedback from aboxhall, add aria-owns test #

Created: 4 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+514 lines, -3 lines)			Patch
M	components/test_runner/web_ax_object_proxy.h	View	1 2 3 4 5	2 chunks	+12 lines, -0 lines	0 comments	Download
M	components/test_runner/web_ax_object_proxy.cc	View	1 2 3 4 5	7 chunks	+135 lines, -1 line	0 comments	Download
A	third_party/WebKit/LayoutTests/accessibility/bounds-calc.html	View	1 2 3 4 5	1 chunk	+250 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/modules/accessibility/AXLayoutObject.h	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp	View	1 2 3 4	1 chunk	+64 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/modules/accessibility/AXObject.h	View	1	1 chunk	+13 lines, -1 line	0 comments	Download
M	third_party/WebKit/Source/modules/accessibility/AXObject.cpp	View	1	1 chunk	+7 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/web/WebAXObject.cpp	View	1 2 3	2 chunks	+17 lines, -0 lines	0 comments	Download
M	third_party/WebKit/public/web/WebAXObject.h	View	1 2 3 4 5	3 chunks	+15 lines, -1 line	0 comments	Download

Messages

Total messages: 20 (6 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

dmazzoni

aboxhall: please review everything chrishtr: this is a follow-up to crrev.com/1777613002 where I added localToAncestorTransform. ...

4 years, 6 months ago (2016-06-07 22:23:57 UTC) #2

chrishtr

https://codereview.chromium.org/2047873002/diff/80001/third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp File third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp (right): https://codereview.chromium.org/2047873002/diff/80001/third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp#newcode289 third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:289: LayoutRect localBounds; Does visualOverflowRect() meet your needs? There is ...

4 years, 6 months ago (2016-06-09 22:24:53 UTC) #3

dmazzoni

4 years, 6 months ago (2016-06-09 22:45:29 UTC) #4

aboxhall

lgtm https://codereview.chromium.org/2047873002/diff/80001/components/test_runner/web_ax_object_proxy.cc File components/test_runner/web_ax_object_proxy.cc (right): https://codereview.chromium.org/2047873002/diff/80001/components/test_runner/web_ax_object_proxy.cc#newcode325 components/test_runner/web_ax_object_proxy.cc:325: gfx::RectF boundsf(0, 0, bounds.width, bounds.height); What does the ...

4 years, 6 months ago (2016-06-09 23:05:30 UTC) #5

chrishtr

4 years, 6 months ago (2016-06-09 23:06:42 UTC) #6

dmazzoni

On 2016/06/09 23:06:42, chrishtr wrote: > https://codereview.chromium.org/2047873002/diff/80001/third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp > File third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp (right): > > https://codereview.chromium.org/2047873002/diff/80001/third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp#newcode289 > ...

4 years, 6 months ago (2016-06-09 23:31:57 UTC) #7

dmazzoni

Description was changed from ========== Add interface to get relative bounding box rect of AX ...

4 years, 6 months ago (2016-06-10 16:17:47 UTC) #8

dmazzoni

https://codereview.chromium.org/2047873002/diff/80001/components/test_runner/web_ax_object_proxy.cc File components/test_runner/web_ax_object_proxy.cc (right): https://codereview.chromium.org/2047873002/diff/80001/components/test_runner/web_ax_object_proxy.cc#newcode325 components/test_runner/web_ax_object_proxy.cc:325: gfx::RectF boundsf(0, 0, bounds.width, bounds.height); On 2016/06/09 23:05:30, aboxhall ...

4 years, 6 months ago (2016-06-10 16:55:40 UTC) #9

chrishtr

On 2016/06/09 at 23:31:57, dmazzoni wrote: > On 2016/06/09 23:06:42, chrishtr wrote: > > https://codereview.chromium.org/2047873002/diff/80001/third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp ...

4 years, 6 months ago (2016-06-10 16:58:54 UTC) #10

On 2016/06/09 at 23:31:57, dmazzoni wrote:
> On 2016/06/09 23:06:42, chrishtr wrote:
> >
https://codereview.chromium.org/2047873002/diff/80001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp
(right):
> > 
> >
https://codereview.chromium.org/2047873002/diff/80001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:289:
> > LayoutRect localBounds;
> > On 2016/06/09 at 22:45:29, dmazzoni wrote:
> > > On 2016/06/09 22:24:53, chrishtr wrote:
> > > > Does visualOverflowRect() meet your needs? There is also
> > > > localOverfowRectForPaintInvalidation, which
> > > > works for SVG, but has a paint-invalidation sounding name.
> > > 
> > > It's in the right coordinate system but it seems to be a few pixels larger
in
> > all dimensions. Perhaps it takes outline into account?
> > 
> > For what example? But yes it takes into account everything, including
outline.
> 
> Here's the output. I really do want the tight bounding box,
> basically matching our existing code (based on absoluteQuads())
> and basically the same as getBoundingClientRect().
> 
> checkbox left: expected 146, got 142
> checkbox top: expected 12, got 8
> checkbox width: expected 12, got 18
> checkbox height: expected 12, got 19
> radio left: expected 168, got 165
> radio top: expected 11, got 8
> radio width: expected 12, got 16
> radio height: expected 13, got 19
> button left: expected 261, got 255
> button top: expected 9, got 5
> button width: expected 50, got 60
> button height: expected 18, got 28
> li1 left: expected 48, got 31
> li1 width: expected 744, got 761
> li2 left: expected 48, got 31
> li2 width: expected 744, got 761
> div5 left: expected 28, got 24
> div5 width: expected 230, got 233

I see, ok. I looked deeper and no, there is no cleaner way I can see at present,
other than introducing more virtual methods on the various classes, which is not
really simpler than this.

dmazzoni

On 2016/06/10 16:58:54, chrishtr wrote: > On 2016/06/09 at 23:31:57, dmazzoni wrote: > > On ...

4 years, 6 months ago (2016-06-10 17:02:20 UTC) #12

On 2016/06/10 16:58:54, chrishtr wrote:
> On 2016/06/09 at 23:31:57, dmazzoni wrote:
> > On 2016/06/09 23:06:42, chrishtr wrote:
> > >
>
https://codereview.chromium.org/2047873002/diff/80001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp
> (right):
> > > 
> > >
>
https://codereview.chromium.org/2047873002/diff/80001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:289:
> > > LayoutRect localBounds;
> > > On 2016/06/09 at 22:45:29, dmazzoni wrote:
> > > > On 2016/06/09 22:24:53, chrishtr wrote:
> > > > > Does visualOverflowRect() meet your needs? There is also
> > > > > localOverfowRectForPaintInvalidation, which
> > > > > works for SVG, but has a paint-invalidation sounding name.
> > > > 
> > > > It's in the right coordinate system but it seems to be a few pixels
larger
> in
> > > all dimensions. Perhaps it takes outline into account?
> > > 
> > > For what example? But yes it takes into account everything, including
> outline.
> > 
> > Here's the output. I really do want the tight bounding box,
> > basically matching our existing code (based on absoluteQuads())
> > and basically the same as getBoundingClientRect().
> > 
> > checkbox left: expected 146, got 142
> > checkbox top: expected 12, got 8
> > checkbox width: expected 12, got 18
> > checkbox height: expected 12, got 19
> > radio left: expected 168, got 165
> > radio top: expected 11, got 8
> > radio width: expected 12, got 16
> > radio height: expected 13, got 19
> > button left: expected 261, got 255
> > button top: expected 9, got 5
> > button width: expected 50, got 60
> > button height: expected 18, got 28
> > li1 left: expected 48, got 31
> > li1 width: expected 744, got 761
> > li2 left: expected 48, got 31
> > li2 width: expected 744, got 761
> > div5 left: expected 28, got 24
> > div5 width: expected 230, got 233
> 
> I see, ok. I looked deeper and no, there is no cleaner way I can see at
present,
> other than introducing more virtual methods on the various classes, which is
not
> really simpler than this.

Thanks!

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2047873002/100001

4 years, 6 months ago (2016-06-10 17:02:49 UTC) #15

commit-bot: I haz the power

Description was changed from ========== Add interface to get relative bounding box rect of AX ...

4 years, 6 months ago (2016-06-10 18:11:26 UTC) #16

Message was sent while issue was closed.

Description was changed from

==========
Add interface to get relative bounding box rect of AX objects.

Currently accessibility bounding box rects are absolute coordinates
within their frames. This means we have to recompute the bounding box
of every AX object when part of the page scrolls or animates.

To improve efficiency, this patch adds a new interface that returns
the relative bounds of an object, consisting of three pieces:

* Its "container" (an ancestor that scrolls or has a paint layer)
* Its bounding box relative to its container
* Optionally, its matrix transform relative to its container

This patch just implements the new interface and tests it.
Adds test_runner interfaces to access this interface at a
low level (relative bounds) and high level (computed bounds)
plus interfaces for accessing the scroll position.
This will be followed up with a patch to use this interface instead
throughout the rest of Chrome, and then delete the old interface.

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

to

==========
Add interface to get relative bounding box rect of AX objects.

Currently accessibility bounding box rects are absolute coordinates
within their frames. This means we have to recompute the bounding box
of every AX object when part of the page scrolls or animates.

To improve efficiency, this patch adds a new interface that returns
the relative bounds of an object, consisting of three pieces:

* Its "container" (an ancestor that scrolls or has a paint layer)
* Its bounding box relative to its container
* Optionally, its matrix transform relative to its container

This patch just implements the new interface and tests it.
Adds test_runner interfaces to access this interface at a
low level (relative bounds) and high level (computed bounds)
plus interfaces for accessing the scroll position.
This will be followed up with a patch to use this interface instead
throughout the rest of Chrome, and then delete the old interface.

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

commit-bot: I haz the power

Description was changed from ========== Add interface to get relative bounding box rect of AX ...

4 years, 6 months ago (2016-06-10 18:13:12 UTC) #19

Message was sent while issue was closed.

Description was changed from

==========
Add interface to get relative bounding box rect of AX objects.

Currently accessibility bounding box rects are absolute coordinates
within their frames. This means we have to recompute the bounding box
of every AX object when part of the page scrolls or animates.

To improve efficiency, this patch adds a new interface that returns
the relative bounds of an object, consisting of three pieces:

* Its "container" (an ancestor that scrolls or has a paint layer)
* Its bounding box relative to its container
* Optionally, its matrix transform relative to its container

This patch just implements the new interface and tests it.
Adds test_runner interfaces to access this interface at a
low level (relative bounds) and high level (computed bounds)
plus interfaces for accessing the scroll position.
This will be followed up with a patch to use this interface instead
throughout the rest of Chrome, and then delete the old interface.

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

to

==========
Add interface to get relative bounding box rect of AX objects.

Currently accessibility bounding box rects are absolute coordinates
within their frames. This means we have to recompute the bounding box
of every AX object when part of the page scrolls or animates.

To improve efficiency, this patch adds a new interface that returns
the relative bounds of an object, consisting of three pieces:

* Its "container" (an ancestor that scrolls or has a paint layer)
* Its bounding box relative to its container
* Optionally, its matrix transform relative to its container

This patch just implements the new interface and tests it.
Adds test_runner interfaces to access this interface at a
low level (relative bounds) and high level (computed bounds)
plus interfaces for accessing the scroll position.
This will be followed up with a patch to use this interface instead
throughout the rest of Chrome, and then delete the old interface.

BUG=618120

Committed: https://crrev.com/e51c64c415fe268350cb451e4678284ae5c8433f
Cr-Commit-Position: refs/heads/master@{#399230}
==========

commit-bot: I haz the power

4 years, 6 months ago (2016-06-10 18:13:13 UTC) #20

Message was sent while issue was closed.

Patchset 6 (id:??) landed as
https://crrev.com/e51c64c415fe268350cb451e4678284ae5c8433f
Cr-Commit-Position: refs/heads/master@{#399230}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages