Bug 73460 – Accessibility: Chromium needs methods to scroll an object into view or to a specific location.

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Dominic Mazzoni]

This includes the when the main document frame, an iframe, or any container element that has CSS overflow:scroll changes its scroll position.

[Dominic Mazzoni]

Adding more details. In order to fully implement Windows accessibility APIs, Chromium needs to:

* Know when any AccessibilityObject is scrollable
* Access the current scroll offsets and min/max offsets
* Be able to scroll the view / change the scroll offsets within that range
* Get a notification when any AccessibilityObject scrolls

[Dominic Mazzoni]

Created attachment 119322 [details]
Proposed scrolling API

Not for review yet, just for discussion.

Does this new API seem reasonable, once I implement support for the notification and add test infrastructure and layout tests?

[Dominic Mazzoni]

Created attachment 122186 [details]
patch

As discussed offline, here's a different proposal - this more closely mirrors the exact functions needed by Windows accessibility APIs.

[Dominic Mazzoni]

Created attachment 122511 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Darin Fisher (:fishd, Google)]

Comment on attachment 122511 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122511&action=review

> Source/WebKit/chromium/public/WebAccessibilityObject.h:168
> +    WEBKIT_EXPORT void scrollToMakeVisibleWithSubFocus(WebRect) const;

nit: pass WebRect as |const WebRect&|
nit: please define the coordinate space
nit: what does "subfocus" mean?

> Source/WebKit/chromium/public/WebAccessibilityObject.h:169
> +    WEBKIT_EXPORT void scrollToGlobalPoint(WebPoint) const;

nit: pass WebPoint as |const WebPoint&|
nit: please define the coordinate space

[Dominic Mazzoni]

Created attachment 122593 [details]
Patch

Addresses comments from fishd

[chris fleizach]

Comment on attachment 122593 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122593&action=review

> Source/WebCore/accessibility/AccessibilityObject.cpp:1587
> +void AccessibilityObject::scrollToMakeVisibleWithSubFocus(IntRect subfocus) const

const IntRect& subfocus

> Source/WebCore/accessibility/AccessibilityObject.cpp:1592
> +    while (scrollParent && !(scrollableArea = scrollParent->getScrollableAreaIfScrollable()))

you can make this into an empty for loop

> Source/WebCore/accessibility/AccessibilityObject.cpp:1619
> +void AccessibilityObject::scrollToGlobalPoint(IntPoint point) const

can this be a reference

> Source/WebCore/accessibility/AccessibilityObject.cpp:1635
> +    for (size_t i = 0; i < objects.size() - 1; i++) {

objects.size() should be in a size_t variable so it's not called on every loop

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3956
> +void AccessibilityRenderObject::scrollTo(int x, int y) const

Can this be an IntPoint instead of int x, int y

[Dominic Mazzoni]

Comment on attachment 122593 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122593&action=review

>> Source/WebCore/accessibility/AccessibilityObject.cpp:1587
>> +void AccessibilityObject::scrollToMakeVisibleWithSubFocus(IntRect subfocus) const
> 
> const IntRect& subfocus

Done, changed all arguments to const refs

>> Source/WebCore/accessibility/AccessibilityObject.cpp:1592
>> +    while (scrollParent && !(scrollableArea = scrollParent->getScrollableAreaIfScrollable()))
> 
> you can make this into an empty for loop

Sure

>> Source/WebCore/accessibility/AccessibilityObject.cpp:1635
>> +    for (size_t i = 0; i < objects.size() - 1; i++) {
> 
> objects.size() should be in a size_t variable so it's not called on every loop

Done

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3956
>> +void AccessibilityRenderObject::scrollTo(int x, int y) const
> 
> Can this be an IntPoint instead of int x, int y

Good idea

[Dominic Mazzoni]

Created attachment 122597 [details]
Patch

Addresses comments from cfleizach

[chris fleizach]

Comment on attachment 122597 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122597&action=review

looks like Mac is still failing too...
/Volumes/Data/WebKit/Source/WebCore/accessibility/AccessibilityObject.cpp:1594: warning: suggest a space before ';' or explicit braces around empty body in 'for' statement

> Source/WebCore/accessibility/AccessibilityObject.cpp:1564
> +        && objectMax - currentScrollOffset <= viewportMax) {

no brackets needed here

> Source/WebCore/accessibility/AccessibilityObject.cpp:1577
> +    return currentScrollOffset;

put in ASSERT_NOT_REACHED

> Source/WebCore/accessibility/AccessibilityObject.cpp:1626
> +    while (scrollParent) {

this can be a for loop.
i would also name scrollParent -> parentObject, since that's a bit closer to the meaning of this variable.

> Source/WebCore/accessibility/AccessibilityObject.cpp:1666
> +        // transformation to apply to all future nested calculations. // (If this is another

there's an "//" at the end of the line that's not necessary

> Tools/DumpRenderTree/chromium/AccessibilityUIElement.cpp:783
> +        result->setNull();

looks like you can set result->setNull() at the top of this method and remove it from the end and in this block, since that gets called anyway. then you can remove the brackets from this if

> Tools/DumpRenderTree/chromium/AccessibilityUIElement.cpp:800
> +        result->setNull();

same comments here, result->setNull() can be called in the beginning

[Dominic Mazzoni]

Comment on attachment 122597 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122597&action=review

>> Source/WebCore/accessibility/AccessibilityObject.cpp:1564
>> +        && objectMax - currentScrollOffset <= viewportMax) {
> 
> no brackets needed here

Done

>> Source/WebCore/accessibility/AccessibilityObject.cpp:1577
>> +    return currentScrollOffset;
> 
> put in ASSERT_NOT_REACHED

Done

>> Source/WebCore/accessibility/AccessibilityObject.cpp:1626
>> +    while (scrollParent) {
> 
> this can be a for loop.
> i would also name scrollParent -> parentObject, since that's a bit closer to the meaning of this variable.

Sure

>> Source/WebCore/accessibility/AccessibilityObject.cpp:1666
>> +        // transformation to apply to all future nested calculations. // (If this is another
> 
> there's an "//" at the end of the line that's not necessary

Oops

>> Tools/DumpRenderTree/chromium/AccessibilityUIElement.cpp:783
>> +        result->setNull();
> 
> looks like you can set result->setNull() at the top of this method and remove it from the end and in this block, since that gets called anyway. then you can remove the brackets from this if

Good idea

>> Tools/DumpRenderTree/chromium/AccessibilityUIElement.cpp:800
>> +        result->setNull();
> 
> same comments here, result->setNull() can be called in the beginning

Done

[Dominic Mazzoni]

Created attachment 122722 [details]
Patch

Addresses comments from most recent review, and also adds two more tests and fixes a logic error detected by those tests.

[chris fleizach]

Comment on attachment 122722 [details]
Patch

looks ok. r=me

[Dominic Mazzoni]

Created attachment 122774 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 122774 [details]
Patch for landing

Rejecting attachment 122774 [details] from commit-queue.

New failing tests:
inspector/protocol/runtime-agent.html
Full output: http://queues.webkit.org/results/11117240

[Dominic Mazzoni]

Created attachment 122871 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 122871 [details]
Patch for landing

Clearing flags on attachment: 122871

Committed r105244: <http://trac.webkit.org/changeset/105244>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Roland Steiner]

Reverted in r105246 as it broke builds.

The reason for the build break is an unused variable 'scrollVisibleRect' in AccessibilityObject.cpp. I was pondering whether to just remove the line declaring the variable, but the surrounding code made me think there might be an inadvertent bug (rather than just a copy/paste error). Since I'm not familiar with the accessibility code enough, I felt reverting was safer.

[Dominic Mazzoni]

Thanks for the extra info, and sorry about the build break. I'll test on Mac before re-landing (after making sure this isn't an error).

[Dominic Mazzoni]

Created attachment 122956 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 122956 [details]
Patch for landing

Clearing flags on attachment: 122956

Committed r105295: <http://trac.webkit.org/changeset/105295>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

These tests:

platform/chromium/accessibility/scroll-to-make-visible-div-overflow.html
platform/chromium/accessibility/scroll-to-make-visible-main-window.html
platform/chromium/accessibility/scroll-to-global-point-iframe-nested.html
platform/chromium/accessibility/scroll-to-global-point-main-window.html
platform/chromium/accessibility/scroll-to-global-point-nested.html
platform/chromium/accessibility/scroll-to-global-point-iframe.html
platform/chromium/accessibility/scroll-to-make-visible-with-subfocus.html
platform/chromium/accessibility/scroll-to-make-visible-iframe.html

all seem to be failing on chromium mac (but pass on other chromium platforms). Here's an example diff:

--- /b/build/slave/Webkit_Mac10_6/build/layout-test-results/platform/chromium/accessibility/scroll-to-make-visible-div-overflow-expected.txt 
+++ /b/build/slave/Webkit_Mac10_6/build/layout-test-results/platform/chromium/accessibility/scroll-to-make-visible-div-overflow-actual.txt 
@@ -7,9 +7,9 @@
 5000-pixel box
 Lower Target
 PASS container.scrollTop is 0
-PASS container.scrollTop is >= 4952
-PASS 5030 is >= container.scrollTop
-PASS container.scrollTop is >= -76
+PASS container.scrollTop is >= 4944
+PASS 5026 is >= container.scrollTop
+PASS container.scrollTop is >= -80
 PASS 2 is >= container.scrollTop
 PASS successfullyParsed is true

which looks like it might just need a rebaseline.  But here's another failure which looks worse:

--- /b/build/slave/Webkit_Mac10_6/build/layout-test-results/platform/chromium/accessibility/scroll-to-global-point-iframe-nested-expected.txt 
+++ /b/build/slave/Webkit_Mac10_6/build/layout-test-results/platform/chromium/accessibility/scroll-to-global-point-iframe-nested-actual.txt 
@@ -9,9 +9,9 @@
 PASS window.pageYOffset is 0
 PASS frameWindow.pageYOffset is 0
 PASS container.scrollTop is 0
-PASS target.getBoundingClientRect().top is 0
-PASS target.getBoundingClientRect().top is 300
-PASS target.getBoundingClientRect().top is 3000
+FAIL target.getBoundingClientRect().top should be 0. Was 4.
+FAIL target.getBoundingClientRect().top should be 300. Was 304.
+FAIL target.getBoundingClientRect().top should be 3000. Was 3004.
 PASS successfullyParsed is true
 
 TEST COMPLETE

I'm suppressing the set on chromium mac for now.

[Dominic Mazzoni]

Fixed in http://trac.webkit.org/changeset/105392