Bug 73460 - Accessibility: Chromium needs methods to scroll an object into view or to a specific location.
: Accessibility: Chromium needs methods to scroll an object into view or to a s...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Accessibility
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Dominic Mazzoni
:
Depends on: 76518
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-30 10:06 PST by Dominic Mazzoni
Modified: 2012-01-19 01:15 PST (History)
8 users (show)

See Also:


Attachments
Proposed scrolling API (8.92 KB, patch)
2011-12-14 16:27 PST, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
patch (25.57 KB, patch)
2012-01-12 01:15 PST, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Patch (52.54 KB, patch)
2012-01-13 15:43 PST, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Patch (52.84 KB, patch)
2012-01-15 22:28 PST, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Patch (53.11 KB, patch)
2012-01-15 23:41 PST, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Patch (62.30 KB, patch)
2012-01-17 00:15 PST, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Patch for landing (62.30 KB, patch)
2012-01-17 08:35 PST, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Patch for landing (62.23 KB, patch)
2012-01-17 22:20 PST, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Patch for landing (62.33 KB, patch)
2012-01-18 10:01 PST, Dominic Mazzoni
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominic Mazzoni 2011-11-30 10:06:17 PST
This includes the when the main document frame, an iframe, or any container element that has CSS overflow:scroll changes its scroll position.
Comment 1 Dominic Mazzoni 2011-12-14 16:07:46 PST
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
Comment 2 Dominic Mazzoni 2011-12-14 16:27:35 PST
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?
Comment 3 Dominic Mazzoni 2012-01-12 01:15:07 PST
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.
Comment 4 Dominic Mazzoni 2012-01-13 15:43:22 PST
Created attachment 122511 [details]
Patch
Comment 5 WebKit Review Bot 2012-01-13 15:45:51 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 6 Darin Fisher (:fishd, Google) 2012-01-13 16:09:34 PST
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
Comment 7 Dominic Mazzoni 2012-01-15 22:28:33 PST
Created attachment 122593 [details]
Patch

Addresses comments from fishd
Comment 8 chris fleizach 2012-01-15 22:44:40 PST
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
Comment 9 Dominic Mazzoni 2012-01-15 23:40:32 PST
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
Comment 10 Dominic Mazzoni 2012-01-15 23:41:16 PST
Created attachment 122597 [details]
Patch

Addresses comments from cfleizach
Comment 11 chris fleizach 2012-01-16 09:23:49 PST
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
Comment 12 Dominic Mazzoni 2012-01-17 00:14:36 PST
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
Comment 13 Dominic Mazzoni 2012-01-17 00:15:12 PST
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.
Comment 14 chris fleizach 2012-01-17 08:19:37 PST
Comment on attachment 122722 [details]
Patch

looks ok. r=me
Comment 15 Dominic Mazzoni 2012-01-17 08:35:24 PST
Created attachment 122774 [details]
Patch for landing
Comment 16 WebKit Review Bot 2012-01-17 10:01:18 PST
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
Comment 17 Dominic Mazzoni 2012-01-17 22:20:54 PST
Created attachment 122871 [details]
Patch for landing
Comment 18 WebKit Review Bot 2012-01-17 22:42:08 PST
Comment on attachment 122871 [details]
Patch for landing

Clearing flags on attachment: 122871

Committed r105244: <http://trac.webkit.org/changeset/105244>
Comment 19 WebKit Review Bot 2012-01-17 22:42:13 PST
All reviewed patches have been landed.  Closing bug.
Comment 20 Roland Steiner 2012-01-17 23:42:20 PST
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.
Comment 21 Dominic Mazzoni 2012-01-17 23:45:20 PST
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).
Comment 22 Dominic Mazzoni 2012-01-18 10:01:15 PST
Created attachment 122956 [details]
Patch for landing
Comment 23 WebKit Review Bot 2012-01-18 10:46:58 PST
Comment on attachment 122956 [details]
Patch for landing

Clearing flags on attachment: 122956

Committed r105295: <http://trac.webkit.org/changeset/105295>
Comment 24 WebKit Review Bot 2012-01-18 10:47:04 PST
All reviewed patches have been landed.  Closing bug.
Comment 25 James Robinson 2012-01-18 13:21:50 PST
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.
Comment 26 Dominic Mazzoni 2012-01-19 01:15:20 PST
Fixed in http://trac.webkit.org/changeset/105392