Bug 125720 – AX: When navigating the elements of a scrollable element with VoiceOver, the scrollTop() position of the element does not permanently change

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

[Jesse Bunch]

Created attachment 219218 [details]
Test case

* SUMMARY
When navigating elements inside a scrolling div (using overflow: auto), WebKit is firing multiple scroll notifications and erroneously resetting the scrollTop value of the containing div with each navigation event.

* STEPS TO REPRODUCE
1. Open the attached test case
2. Turn on VoiceOver and begin navigating the elements inside the scrolling container.

* RESULTS
The container jumps to the newly focused element, but then quickly scrolls back up to it's original position. The focused item should be scrolled to when focused. 

* NOTES
After further investigation, it appears that a second scroll event is also being fired. Check the console in the test case for that evidence.

[James Craig]

<rdar://problem/15662667>

[Jeremy Elbourn]

Here is an example where this behavior with VoiceOver completely breaks the date-picker component in Angular Material:

https://dl.dropboxusercontent.com/u/18203969/oneoff/v4/datepicker.html

Summary of what's going on:
The pop-up calendar has a table (with role=grid) inside of a very large scrollable container. As the container scrolls, tbody elements in the table are added and removed so that only 6 or 7 are rendered at a time. 

When the calendar is opened, the container is scrolled to about half its scrollHeight and sets focus to the gridcell for the selected date. When this focus occurs with VoiceOver active, however, the container jumps to scrollTop = 0, which then both shows the wrong date and destroys the gridcell that had focus, leaving the user in quite the unhappy state. 

This bug only seems to occur in Safari with VoiceOver; Chrome with VoiceOver works as expected.

[James Craig]

Related to bug 132310 and bug 147700.

[chris fleizach]

Created attachment 259431 [details]
patch

[WebKit Commit Bot]

Attachment 259431 [details] did not pass style-queue:


ERROR: Source/WebCore/accessibility/AccessibilityObject.cpp:2406:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Created attachment 259433 [details]
patch

[James Craig]

Thanks for working on this. Will you please check to see if your patch also resolves bug 132310?

[chris fleizach]

(In reply to comment #7)
> Thanks for working on this. Will you please check to see if your patch also
> resolves bug 132310?

Yes it does!

[chris fleizach]

Created attachment 259468 [details]
patch

[James Craig]

*** Bug 132310 has been marked as a duplicate of this bug. ***

[James Craig]

(In reply to comment #8)
> (In reply to comment #7)
> > Thanks for working on this. Will you please check to see if your patch also
> > resolves bug 132310?
> 
> Yes it does!

Great. I duped and closed that one.

[Daniel Bates]

Comment on attachment 259468 [details]
patch

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

This patch looks sane to me. Feel free to ask a viewport/scrolling expert if you want a more through review of the scrolling code.

> Source/WebCore/accessibility/AccessibilityObject.cpp:2392
> +        if (subfocusMin - currentScrollOffset >= viewportMin
> +            && subfocusMax - currentScrollOffset <= viewportMax)

This is fine as-is. I would write this on one line.

> Source/WebCore/accessibility/AccessibilityObject.cpp:2410
> +    

Please do not add this whitespace.

> Source/WebCore/accessibility/AccessibilityObject.cpp:2419
> +    

Ditto.

> Tools/DumpRenderTree/AccessibilityUIElement.cpp:695
> +    unsigned x = 0, y = 0, width = 0, height = 0;

Although not written in the WebKit Code Style Guidelines, we tend to prefer one variable definition per line.

> Tools/DumpRenderTree/AccessibilityUIElement.cpp:702
> +    

Please remove the whitespace on this line.

> Tools/DumpRenderTree/AccessibilityUIElement.cpp:709
> +    unsigned x = 0, y = 0;

Although not written in the WebKit Code Style Guidelines, we tend to prefer one variable definition per line.

> LayoutTests/accessibility/scroll-to-global-point-iframe-nested.html:1
> +<html>

Unless you intended to test in quirks mode, I suggest adding <!DOCTYPE html> to the top of this file.

> LayoutTests/accessibility/scroll-to-global-point-iframe-nested.html:27
> +<!-- The contents of this iframe, more nicely formatted:
> + <body>
> +   <style>
> +     button {
> +       border: 0;
> +     }
> +   </style>
> +   <div style='border: 1px solid #000; height: 5000px;'>5000-pixel box</div>
> +   <div id='container' style='height: 100px; overflow: scroll'>
> +     <div style='border: 1px solid #000; height: 5000px;'>5000-pixel box</div>
> +     <button id='target'>Target</button>
> +     <div style='border: 1px solid #000; height: 5000px;'>5000-pixel box</div>
> +   </div>
> +   <div style='border: 1px solid #000; height: 5000px;'>5000-pixel box</div>
> +  </body>
> + -->
> +<iframe id="frame" src="data:text/html,<body><style>button { border: 0; }</style><div style='border: 1px solid #000; height: 5000px;'>5000-pixel box</div><div id='container' style='height: 100px; overflow: scroll'><div style='border: 1px solid #000; height: 5000px;'>5000-pixel box</div><button id='target'>Target</button><div style='border: 1px solid #000; height: 5000px;'>5000-pixel box</div></div><div style='border: 1px solid #000; height: 5000px;'>5000-pixel box</div></body>"></iframe>

I would remove the comment and write this as:

<iframe id="frame" srcdoc="<body>
    <style>button { border: 0; }</style>
    <div style='border: 1px solid #000; height: 5000px;'>5000-pixel box</div>
    <div id='container' style='height: 100px; overflow: scroll'>
        <div style='border: 1px solid #000; height: 5000px;'>5000-pixel box</div>
        <button id='target'>Target</button>
        <div style='border: 1px solid #000; height: 5000px;'>5000-pixel box</div>
    </div>
    <div style='border: 1px solid #000; height: 5000px;'>5000-pixel box</div>
</body>"></iframe>

> LayoutTests/accessibility/scroll-to-global-point-iframe-nested.html:37
> +        testRunner.waitUntilDone();

The indentation of this line differs from the indentation used on other lines with the same nesting level (e.g. line 42).

> LayoutTests/accessibility/scroll-to-global-point-iframe-nested.html:51
> +    if (window.accessibilityController) {
> +        target.focus();
> +        var targetAccessibleObject = accessibilityController.focusedElement;
> +    }

This is OK as-is. For your consideration, I suggest explicitly declaring targetAccessibleObject outside of this if-statement body, such that this code reads:

var targetAccessibleObject;
if (window.accessibilityController) {
    target.focus();
    targetAccessibleObject = accessibilityController.focusedElement
}

> LayoutTests/accessibility/scroll-to-global-point-iframe-nested.html:57
> +    shouldBe("window.pageYOffset", "0");

This is OK as-is. We can make use of the convenience function shouldBeZero() to write this line as:

shouldBeZero("window.pageYOffset");

> LayoutTests/accessibility/scroll-to-global-point-iframe-nested.html:58
> +    shouldBe("frameWindow.pageYOffset", "0");

Similarly, we can make use of shouldBeZero() on this line.

> LayoutTests/accessibility/scroll-to-global-point-iframe-nested.html:59
> +    shouldBe("container.scrollTop", "0");

Ditto.

> LayoutTests/accessibility/scroll-to-global-point-iframe-nested.html:64
> +    shouldBe("target.getBoundingClientRect().top", "0");

Ditto.

> LayoutTests/accessibility/scroll-to-global-point-iframe-nested.html:75
> +window.addEventListener('load', function() {

Nit: ' (single quotes) => " (double quotes)

> LayoutTests/accessibility/scroll-to-global-point-iframe-nested.html:76
> +    setTimeout(runTest, 10);

Is this timeout necessary? How did you come to the decision to wait 10 milliseconds?

> LayoutTests/accessibility/scroll-to-global-point-iframe.html:1
> +<html>

Unless you intended to test in quirks mode, I suggest adding <!DOCTYPE html> to the top of this file.

> LayoutTests/accessibility/scroll-to-global-point-iframe.html:11
> +<iframe id="frame" src="data:text/html,<body><style>button { border: 0; }</style><div style='border: 1px solid #000; height: 5000px;'>5000-pixel box</div><button id='target'>Target</button><div style='border: 1px solid #000; height: 5000px;'>5000-pixel box</div></body>"></iframe>

We can take a similar approach as in LayoutTests/accessibility/scroll-to-global-point-iframe-nested.html and write this using the srcdoc attribute and formatting the markup to make it more readable.

> LayoutTests/accessibility/scroll-to-global-point-iframe.html:21
> +        testRunner.waitUntilDone();

The indentation of this line differs from the indentation used on other lines with the same nesting level (e.g. line 26).

> LayoutTests/accessibility/scroll-to-global-point-iframe.html:34
> +    if (window.accessibilityController) {
> +        target.focus();
> +        var targetAccessibleObject = accessibilityController.focusedElement;
> +    }

This is OK as-is. For your consideration, I suggest explicitly declaring targetAccessibleObject outside of this if-statement body, such that this code reads:

var targetAccessibleObject;
if (window.accessibilityController) {
    target.focus();
    targetAccessibleObject = accessibilityController.focusedElement
}

> LayoutTests/accessibility/scroll-to-global-point-iframe.html:39
> +    shouldBe("window.pageYOffset", "0");

This is OK as-is. We can make use of the convenience function shouldBeZero() to write this line as:

shouldBeZero("window.pageYOffset");

> LayoutTests/accessibility/scroll-to-global-point-iframe.html:40
> +    shouldBe("frameWindow.pageYOffset", "0");

Similarly, we can make use of shouldBeZero() on this line.

> LayoutTests/accessibility/scroll-to-global-point-iframe.html:45
> +    shouldBe("target.getBoundingClientRect().top", "0");

Ditto.

> LayoutTests/accessibility/scroll-to-global-point-iframe.html:56
> +window.addEventListener('load', function() {

Nit: ' (single quotes) => " (double quotes)

> LayoutTests/accessibility/scroll-to-global-point-iframe.html:57
> +    setTimeout(runTest, 10);

Is this timeout necessary? How did you come to the decision to wait 10 milliseconds?

> LayoutTests/accessibility/scroll-to-global-point-main-window.html:1
> +<html>

Unless you intended to test in quirks mode, I suggest adding <!DOCTYPE html> to the top of this file.

> LayoutTests/accessibility/scroll-to-global-point-nested.html:1
> +<html>

Unless you intended to test in quirks mode, I suggest adding <!DOCTYPE html> to the top of this file.

> LayoutTests/accessibility/scroll-to-global-point-nested.html:40
> +        var targetAccessibleObject = accessibilityController.focusedElement;

For your consideration, I suggest explicitly declaring this variable outside of the if-statement body.

> LayoutTests/accessibility/scroll-to-global-point-nested.html:49
> +    shouldBe("window.pageYOffset", "0");
> +    shouldBe("outerContainer.scrollTop", "0");
> +    shouldBe("innerContainer.scrollTop", "0");

This is OK as-is. We can make use of the convenience function shouldBeZero() to simplify these lines.

> LayoutTests/accessibility/scroll-to-global-point-nested.html:56
> +    shouldBe("target.getBoundingClientRect().top", "0");

Ditto.

> LayoutTests/accessibility/scroll-to-make-visible-div-overflow.html:1
> +<html>

Unless you intended to test in quirks mode, I suggest adding <!DOCTYPE html> to the top of this file.

> LayoutTests/accessibility/scroll-to-make-visible-div-overflow.html:30
> +    if (window.accessibilityController) {
> +        lowerTarget.focus();
> +        var lowerTargetAccessibleObject = accessibilityController.focusedElement;
> +        upperTarget.focus();
> +        var upperTargetAccessibleObject = accessibilityController.focusedElement;
> +    }

For your consideration, I suggest declaring lowerTargetAccessibleObject and upperTargetAccessibleObject outside of the if-statement body, such that it reads:

var lowerTargetAccessibleObject;
var upperTargetAccessibleObject;
if (window.accessibilityController) {
    lowerTarget.focus();
    lowerTargetAccessibleObject = accessibilityController.focusedElement;
    upperTarget.focus();
    upperTargetAccessibleObject = accessibilityController.focusedElement;
}

> LayoutTests/accessibility/scroll-to-make-visible-div-overflow.html:34
> +    shouldBe("container.scrollTop", "0");

This is OK as-is. We can make use of the convenience function shouldBeZero() to simplify these lines.

> LayoutTests/accessibility/scroll-to-make-visible-div-overflow.html:52
> +    shouldBe("container.scrollTop >= minYOffset", "true");
> +    shouldBe("container.scrollTop <= maxYOffset", "true");

I would write these lines using the convenience function shouldBeTrue():

shouldBeTrue("container.scrollTop >= minYOffset");
shouldBeTrue("container.scrollTop <= maxYOffset");

> LayoutTests/accessibility/scroll-to-make-visible-div-overflow.html:61
> +    shouldBe("container.scrollTop >= minYOffset", "true");
> +    shouldBe("container.scrollTop <= maxYOffset", "true");

Similarly, I would write these lines using the convenience function shouldBeTrue():

shouldBeTrue("container.scrollTop >= minYOffset");
shouldBeTrue("container.scrollTop <= maxYOffset");

> LayoutTests/accessibility/scroll-to-make-visible-iframe.html:1
> +<html>

Unless you intended to test in quirks mode, I suggest adding <!DOCTYPE html> to the top of this file.

> LayoutTests/accessibility/scroll-to-make-visible-iframe.html:34
> +    if (window.accessibilityController) {
> +        lowerTarget.focus();
> +        var lowerTargetAccessibleObject = accessibilityController.focusedElement;
> +        upperTarget.focus();
> +        var upperTargetAccessibleObject = accessibilityController.focusedElement;
> +    }

See my remark about the same code in file LayoutTests/accessibility/scroll-to-make-visible-div-overflow.html.

> LayoutTests/accessibility/scroll-to-make-visible-iframe.html:38
> +    shouldBe("frameWindow.pageYOffset", "0");

This is OK as-is. We can make use of the convenience function shouldBeZero() to simplify these lines.

> LayoutTests/accessibility/scroll-to-make-visible-iframe.html:46
> +    shouldBe("frameWindow.pageYOffset >= minYOffset", "true");
> +    shouldBe("frameWindow.pageYOffset <= maxYOffset", "true");

I would write these lines using the convenience function shouldBeTrue():

shouldBeTrue("frameWindow.pageYOffset >= minYOffset");
shouldBeTrue("frameWindow.pageYOffset <= maxYOffset");

> LayoutTests/accessibility/scroll-to-make-visible-iframe.html:54
> +    shouldBe("frameWindow.pageYOffset >= minYOffset", "true");
> +    shouldBe("frameWindow.pageYOffset <= maxYOffset", "true");

Similarly, I would write this using the convenience function shouldBeTrue():

shouldBeTrue("frameWindow.pageYOffset >= minYOffset");
shouldBeTrue("frameWindow.pageYOffset <= maxYOffset");

> LayoutTests/accessibility/scroll-to-make-visible-iframe.html:61
> +window.addEventListener('load', function() {
> +    setTimeout(runTest, 10);
> +}, false);

Is this timeout necessary? How did you come to the decision to wait 10 milliseconds?

> LayoutTests/accessibility/scroll-to-make-visible-nested-2.html:1
> +<html>

Unless you intended to test in quirks mode, I suggest adding <!DOCTYPE html> to the top of this file.

> LayoutTests/accessibility/scroll-to-make-visible-nested-2.html:35
> +    if (window.accessibilityController) {
> +        target1.focus();
> +        var target1AccessibleObject = accessibilityController.focusedElement;
> +        target2.focus();
> +        var target2AccessibleObject = accessibilityController.focusedElement;
> +        target3.focus();
> +        var target3AccessibleObject = accessibilityController.focusedElement;
> +    }

This is OK as-is. We should declare the variables target{1, 2, 3}AccessibleObject outside of the if-statement body. I made a similar remark in file LayoutTests/accessibility/scroll-to-make-visible-div-overflow.html.

> LayoutTests/accessibility/scroll-to-make-visible-nested-2.html:41
> +    shouldBe("window.pageYOffset", "0");
> +    shouldBe("container.scrollTop", "0");

This is OK as-is. We can make use of the convenience function shouldBeZero() to simplify these lines.

> LayoutTests/accessibility/scroll-to-make-visible-nested.html:1
> +<html>

Unless you intended to test in quirks mode, I suggest adding <!DOCTYPE html> to the top of this file.

> LayoutTests/accessibility/scroll-to-make-visible-nested.html:40
> +    shouldBe("window.pageYOffset", "0");
> +    shouldBe("outerContainer.scrollTop", "0");
> +    shouldBe("innerContainer.scrollTop", "0");

This is OK as-is. We can make use of the convenience function shouldBeZero() to simplify these lines.

> LayoutTests/accessibility/scroll-to-make-visible-with-subfocus.html:1
> +<html>

Unless you intended to test in quirks mode, I suggest adding <!DOCTYPE html> to the top of this file.

> LayoutTests/accessibility/scroll-to-make-visible-with-subfocus.html:22
> +        var targetAccessibleObject = accessibilityController.focusedElement;

This is OK as-is. We should declare the variables targetAccessibleObject outside of the if-statement body. I made a similar remark in file LayoutTests/accessibility/scroll-to-make-visible-div-overflow.html.

> LayoutTests/accessibility/scroll-to-make-visible-with-subfocus.html:27
> +    shouldBe("window.pageYOffset", "0");

This is OK as-is. We can make use of the convenience function shouldBeZero() to simplify these lines.

> LayoutTests/accessibility/scroll-to-make-visible-with-subfocus.html:35
> +    shouldBe("window.pageYOffset >= minYOffset", "true");
> +    shouldBe("window.pageYOffset <= maxYOffset", "true");

This is OK as-is. We can make use of the convenience function shouldBeTrue() to simplify these lines.

[chris fleizach]

Comment on attachment 259468 [details]
patch

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

thanks for the thorough review. i will make all the changes before committing.

>> LayoutTests/accessibility/scroll-to-global-point-iframe-nested.html:76
>> +    setTimeout(runTest, 10);
> 
> Is this timeout necessary? How did you come to the decision to wait 10 milliseconds?

don't know. it was a blink test. i'll experiment to see if it's necessary

[chris fleizach]

http://trac.webkit.org/changeset/189149

[Alexey Proskuryakov]

A 10 ms timeout is never right in a test. When tests run, there is a lot of CPU load, so any task can be preempted for longer than that.

Separately from that, all the new tests fail on Windows, could you please fix that?

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

[chris fleizach]

(In reply to comment #15)
> A 10 ms timeout is never right in a test. When tests run, there is a lot of
> CPU load, so any task can be preempted for longer than that.
> 

I changed these all to setTimeout=0 and they worked fine. I copied them from Blink so I won't take credit for why they were originally set to 10

> Separately from that, all the new tests fail on Windows, could you please
> fix that?
> 
> accessibility/scroll-to-global-point-iframe-nested.html
> accessibility/scroll-to-global-point-iframe.html
> accessibility/scroll-to-global-point-main-window.html
> accessibility/scroll-to-global-point-nested.html
> accessibility/scroll-to-make-visible-div-overflow.html
> accessibility/scroll-to-make-visible-iframe.html
> accessibility/scroll-to-make-visible-nested-2.html
> accessibility/scroll-to-make-visible-nested.html
> accessibility/scroll-to-make-visible-with-subfocus.html

likely they all need to be skipped because the methods are not implemented. will do so

[chris fleizach]

http://trac.webkit.org/changeset/189163