Bug 125195 - Layout Test platform/mac/accessibility/search-predicate-element-count.html is flaky
Summary: Layout Test platform/mac/accessibility/search-predicate-element-count.html is...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Samuel White
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-12-03 15:16 PST by Alexey Proskuryakov
Modified: 2013-12-04 22:41 PST (History)
10 users (show)

See Also:


Attachments
Patch. (5.92 KB, patch)
2013-12-04 11:07 PST, Samuel White
no flags Details | Formatted Diff | Diff
Updated patch. (6.59 KB, patch)
2013-12-04 11:29 PST, Samuel White
no flags Details | Formatted Diff | Diff
Updated patch. (6.60 KB, patch)
2013-12-04 13:27 PST, Samuel White
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2013-12-03 15:16:17 PST
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.
Comment 1 Alexey Proskuryakov 2013-12-03 15:18:29 PST
Marked as flaky in <http://trac.webkit.org/r160049>.
Comment 2 Alexey Proskuryakov 2013-12-03 22:19:18 PST
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
Comment 3 Samuel White 2013-12-04 10:32:11 PST
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.
Comment 4 Samuel White 2013-12-04 11:07:50 PST
Created attachment 218420 [details]
Patch.
Comment 5 Samuel White 2013-12-04 11:09:49 PST
Another option is to use fixed positioning on the elements that will be checked for visibility. Thoughts?
Comment 6 James Craig 2013-12-04 11:22:40 PST
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.
Comment 7 Alexey Proskuryakov 2013-12-04 11:27:31 PST
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.
Comment 8 Samuel White 2013-12-04 11:29:23 PST
Created attachment 218424 [details]
Updated patch.

Updated patch to remove test from flaky list.
Comment 9 chris fleizach 2013-12-04 12:08:52 PST
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?
Comment 10 Samuel White 2013-12-04 12:17:03 PST
(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).
Comment 11 Samuel White 2013-12-04 13:21:05 PST
(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.
Comment 12 Alexey Proskuryakov 2013-12-04 13:26:28 PST
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.
Comment 13 Samuel White 2013-12-04 13:27:52 PST
Created attachment 218438 [details]
Updated patch.

Added missing <br> to layout test for consistency.
Comment 14 Samuel White 2013-12-04 13:43:57 PST
(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!
Comment 15 Alexey Proskuryakov 2013-12-04 13:53:30 PST
> 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)?
Comment 16 Samuel White 2013-12-04 14:03:08 PST
(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.
Comment 17 Samuel White 2013-12-04 17:40:13 PST
Thanks for the help everyone. Any additional feedback on this one?
Comment 18 WebKit Commit Bot 2013-12-04 22:41:48 PST
Comment on attachment 218438 [details]
Updated patch.

Clearing flags on attachment: 218438

Committed r160160: <http://trac.webkit.org/changeset/160160>
Comment 19 WebKit Commit Bot 2013-12-04 22:41:52 PST
All reviewed patches have been landed.  Closing bug.