This includes the when the main document frame, an iframe, or any container element that has CSS overflow:scroll changes its scroll position.
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
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?
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.
Created attachment 122511 [details] Patch
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
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
Created attachment 122593 [details] Patch Addresses comments from fishd
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 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
Created attachment 122597 [details] Patch Addresses comments from cfleizach
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 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
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 on attachment 122722 [details] Patch looks ok. r=me
Created attachment 122774 [details] Patch for landing
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
Created attachment 122871 [details] Patch for landing
Comment on attachment 122871 [details] Patch for landing Clearing flags on attachment: 122871 Committed r105244: <http://trac.webkit.org/changeset/105244>
All reviewed patches have been landed. Closing bug.
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.
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).
Created attachment 122956 [details] Patch for landing
Comment on attachment 122956 [details] Patch for landing Clearing flags on attachment: 122956 Committed r105295: <http://trac.webkit.org/changeset/105295>
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.
Fixed in http://trac.webkit.org/changeset/105392