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
: WebKit
Accessibility
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 76518
:
  Show dependency treegraph
 
Reported: 2011-11-30 10:06 PST by
Modified: 2012-01-19 01:15 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 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 From 2011-12-14 16:27:35 PST -------
Created an attachment (id=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 From 2012-01-12 01:15:07 PST -------
Created an attachment (id=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 From 2012-01-13 15:43:22 PST -------
Created an attachment (id=122511) [details]
Patch
------- Comment #5 From 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 From 2012-01-13 16:09:34 PST -------
(From update of attachment 122511 [details])
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 From 2012-01-15 22:28:33 PST -------
Created an attachment (id=122593) [details]
Patch

Addresses comments from fishd
------- Comment #8 From 2012-01-15 22:44:40 PST -------
(From update of attachment 122593 [details])
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 From 2012-01-15 23:40:32 PST -------
(From update of attachment 122593 [details])
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 From 2012-01-15 23:41:16 PST -------
Created an attachment (id=122597) [details]
Patch

Addresses comments from cfleizach
------- Comment #11 From 2012-01-16 09:23:49 PST -------
(From update of attachment 122597 [details])
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 From 2012-01-17 00:14:36 PST -------
(From update of attachment 122597 [details])
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 From 2012-01-17 00:15:12 PST -------
Created an attachment (id=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 From 2012-01-17 08:19:37 PST -------
(From update of attachment 122722 [details])
looks ok. r=me
------- Comment #15 From 2012-01-17 08:35:24 PST -------
Created an attachment (id=122774) [details]
Patch for landing
------- Comment #16 From 2012-01-17 10:01:18 PST -------
(From update of attachment 122774 [details])
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 From 2012-01-17 22:20:54 PST -------
Created an attachment (id=122871) [details]
Patch for landing
------- Comment #18 From 2012-01-17 22:42:08 PST -------
(From update of attachment 122871 [details])
Clearing flags on attachment: 122871

Committed r105244: <http://trac.webkit.org/changeset/105244>
------- Comment #19 From 2012-01-17 22:42:13 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #20 From 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 From 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 From 2012-01-18 10:01:15 PST -------
Created an attachment (id=122956) [details]
Patch for landing
------- Comment #23 From 2012-01-18 10:46:58 PST -------
(From update of attachment 122956 [details])
Clearing flags on attachment: 122956

Committed r105295: <http://trac.webkit.org/changeset/105295>
------- Comment #24 From 2012-01-18 10:47:04 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #25 From 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 From 2012-01-19 01:15:20 PST -------
Fixed in http://trac.webkit.org/changeset/105392