Layout Test platform/mac/accessibility/search-predicate-element-count.html is flaky

• Component: Accessibility
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

The following layout test is flaky on all Mac testers

platform/mac/accessibility/search-predicate-element-count.html

Has been failing from the beginning, seems to be affected by other tests.

[Alexey Proskuryakov]

Marked as flaky in <http://trac.webkit.org/r160049>.

[Alexey Proskuryakov]

Samuel posted to webkit-dev that this is reproducible with:

Tools/Scripts/run-webkit-tests --debug LayoutTests/platform/mac/accessibility/document-links.html LayoutTests/platform/mac/accessibility/search-predicate-element-count.html

[Samuel White]

Looks like AccessibilityObject::isOnscreen() is causing the failure. This is where we check if the elements rect is onscreen by iterating the scroll ancestors and checking for intersection. Debug output shows the following problem while this test runs:

level: 1, outer: ((0, 0), (785, 600)), inner: ((8, 659), (769, 37))
this one failed
level: 1, outer: ((0, 0), (785, 600)), inner: ((8, 717), (769, 28))
this one failed
level: 1, outer: ((0, 0), (785, 600)), inner: ((8, 764), (769, 22))
this one failed
level: 1, outer: ((0, 0), (785, 600)), inner: ((8, 807), (769, 18))
this one failed
level: 1, outer: ((0, 0), (785, 600)), inner: ((8, 10846), (769, 15))
this one failed
level: 1, outer: ((0, 0), (785, 600)), inner: ((8, 10885), (769, 13))
this one failed

This shows us that "outer", which is the parent rect looks correct. However, inner, which is our element rect has a large y offset when isOnscreen is called. Odd...

These six outputs correspond to the last check in the above test which checks that 4 of 6 headings are visible. We expect the last two to have crazy y offsets because we specify such in our CSS.

[Samuel White]

Created attachment 218420 [details]
Patch.

[Samuel White]

Another option is to use fixed positioning on the elements that will be checked for visibility. Thoughts?

[James Craig]

I would not use fixed positioning. I'd just change this:

.offscreen {
  position:relative;
  top:9999px;
}

To this:

.offscreen {
  position: absolute;
  width: 500px;
  left: -999px;
}

That way it will be permanently out of view but won't cause the progressive rendering of the extra long scroll view, which might be the reason you're getting the flakiness.

[Alexey Proskuryakov]

What is the state that leaks from document-links.html? We should make sure that DumpRenderTree and WebKitTestRunner don't let one test affect another like this.

[Samuel White]

Created attachment 218424 [details]
Updated patch.

Updated patch to remove test from flaky list.

[chris fleizach]

Comment on attachment 218424 [details]
Updated patch.

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

> LayoutTests/platform/mac/accessibility/search-predicate-element-count.html:17
> +<!-- Headings. -->

won't this just push the <input> elements farther down and those would fail?

[Samuel White]

(In reply to comment #9)
> (From update of attachment 218424 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218424&action=review
> 
> > LayoutTests/platform/mac/accessibility/search-predicate-element-count.html:17
> > +<!-- Headings. -->
> 
> won't this just push the <input> elements farther down and those would fail?

We only search for "visible only" elements once. This change makes sure those will be onscreen even in a tiny tiny window.

As suggested above, the only additional safety I can think of would be also use fixed positioning on the "onscreen" elements to pile them in the top left corner (pretty much where their static positioning is anyways).

[Samuel White]

(In reply to comment #7)
> What is the state that leaks from document-links.html? We should make sure that DumpRenderTree and WebKitTestRunner don't let one test affect another like this.

I'm not convinced we have a state leak between the two. The steps I have given do reliably reproduce the issue which gives support to the idea that they're tied, but no clear link has been identified.

However, the data I later uncovered shows that elements need to be within the 800x600 window to be "visible" according to AccessibilityObject::isOnscreen(). I can't explain why this works sometimes and not others but I suspect that the images that come before these "visible" elements in the DOM are sometimes not fully loaded and thus don't force all the later content to be pushed down the page. When their full size is considered, this test should absolutely fail as it did.

This was my motivation behind simply moving the "onscreen" portion of this test to the beginning of the DOM to make sure they are visible.

[Alexey Proskuryakov]

As it is always the case with mysteries, getting to the bottom of it may be an opportunity to improve robustness of many related tests.

[Samuel White]

Created attachment 218438 [details]
Updated patch.

Added missing <br> to layout test for consistency.

[Samuel White]

(In reply to comment #12)
> As it is always the case with mysteries, getting to the bottom of it may be an opportunity to improve robustness of many related tests.

Agreed. Just confirmed my above suspicion.

1. The test WAS passing sometimes because the images were not fully loaded and thus didn't push the later elements outside the 800x600 window causing them to not be found during our "visible elements only" search as I suspected.

2. The reason the other test coming first matters is caching. The LayoutTests/platform/mac/accessibility/document-links.html test loads the cake.png image from the resource folder and ensures that it gets 'loaded quicker' by the next test. (and thus pushed our elements offscreen).

To confirm #2 you can simply mislabel the cake.png image in the LayoutTests/platform/mac/accessibility/document-links.html test which will not cause that test to fail but WILL cause the search test to pass (again, because of the smaller size the placeholder images will have before the actual image is loaded). Alternatively, you can use onload events on each img to ensure they are/aren't loaded in time.

Mystery solved, myth busted, etc. :)

In the end, this test should have never worked. The elements in question were never going to be visible once the images were loaded. So the DOM order fix seems correct.

Thanks!

[Alexey Proskuryakov]

> Mystery solved, myth busted, etc. :)

Yay!

So, would it be beneficial to run the test in onload as opposed to a script that runs before the load finishes (plus the other improvements you've made to the test)?

[Samuel White]

(In reply to comment #15)
> > Mystery solved, myth busted, etc. :)
> 
> Yay!
> 
> So, would it be beneficial to run the test in onload as opposed to a script that runs before the load finishes (plus the other improvements you've made to the test)?

This test doesn't actually care if the graphic has fully loaded now that things are reordered. So in this case speed is likely preferable to waiting for onload to fire. However, we have seen some flakiness in our other similarly named layout test that also evaluates "onscreen" elements so we might need to apply that paradigm there.

I'll open another bug and investigate that test as well.

[Samuel White]

Thanks for the help everyone. Any additional feedback on this one?

[WebKit Commit Bot]

Comment on attachment 218438 [details]
Updated patch.

Clearing flags on attachment: 218438

Committed r160160: <http://trac.webkit.org/changeset/160160>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.