Bug 142358

Summary:

Create a set of scroll snap point tests

Product:

WebKit

Reporter:

Brent Fulgham <bfulgham>

Component:

Layout and Rendering

Assignee:

Brent Fulgham <bfulgham>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, buildbot, rniwa, simon.fraser

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

See Also:

https://bugs.webkit.org/show_bug.cgi?id=144591

Bug Depends on:

144482

Bug Blocks:

Attachments:

Description	Flags
Patch	none
Patch	none
Archive of layout-test-results from ews101 for mac-mavericks	none
Archive of layout-test-results from ews104 for mac-mavericks-wk2	none
Patch to see what testers are encountering.	none
Archive of layout-test-results from ews104 for mac-mavericks-wk2	none
Archive of layout-test-results from ews100 for mac-mavericks	none
Patch	none
Patch v3 - resolve flakiness.	simon.fraser: review+

Brent Fulgham

Reported 2015-03-05 12:48:59 PST

Add layout tests for scroll snap animations, now that they are working properly for the "Mandatory" scroll point case.

Attachments
Patch (17.31 KB, patch) 2015-03-05 21:07 PST, Brent Fulgham	no flags	Details Formatted Diff Diff
Patch (17.16 KB, patch) 2015-03-05 21:12 PST, Brent Fulgham	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews101 for mac-mavericks (530.41 KB, application/zip) 2015-03-05 21:59 PST, Build Bot	no flags	Details
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (679.62 KB, application/zip) 2015-03-05 22:05 PST, Build Bot	no flags	Details
Patch to see what testers are encountering. (17.38 KB, patch) 2015-03-05 23:02 PST, Brent Fulgham	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (737.85 KB, application/zip) 2015-03-05 23:45 PST, Build Bot	no flags	Details
Archive of layout-test-results from ews100 for mac-mavericks (549.11 KB, application/zip) 2015-03-05 23:48 PST, Build Bot	no flags	Details
Patch (16.71 KB, patch) 2015-03-06 09:19 PST, Brent Fulgham	no flags	Details Formatted Diff Diff
Patch v3 - resolve flakiness. (30.21 KB, patch) 2015-05-01 18:27 PDT, Brent Fulgham	simon.fraser: review+	Details Formatted Diff Diff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.

Brent Fulgham

Comment 1 2015-03-05 21:00:06 PST

This set of tests exercises the 'slow' code path. Changes will be needed in WebKitTestRunner to support the scrolling-thread code path.

Brent Fulgham

Comment 2 2015-03-05 21:07:15 PST

Created attachment 248036 [details] Patch

Brent Fulgham

Comment 3 2015-03-05 21:12:22 PST

Created attachment 248037 [details] Patch

Build Bot

Comment 4 2015-03-05 21:59:54 PST

Comment on attachment 248037 [details] Patch Attachment 248037 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6208385754071040 New failing tests: css3/scroll-snap/scroll-snap-mandatory-vertical.html css3/scroll-snap/scroll-snap-mandatory-horizontal.html

Build Bot

Comment 5 2015-03-05 21:59:58 PST

Created attachment 248039 [details] Archive of layout-test-results from ews101 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5

Build Bot

Comment 6 2015-03-05 22:05:42 PST

Comment on attachment 248037 [details] Patch Attachment 248037 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6208172079448064 New failing tests: css3/scroll-snap/scroll-snap-mandatory-vertical.html css3/scroll-snap/scroll-snap-mandatory-horizontal.html

Build Bot

Comment 7 2015-03-05 22:05:45 PST

Created attachment 248040 [details] Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5

Brent Fulgham

Comment 8 2015-03-05 23:02:58 PST

Created attachment 248042 [details] Patch to see what testers are encountering.

Darin Adler

Comment 9 2015-03-05 23:19:12 PST

Comment on attachment 248042 [details] Patch to see what testers are encountering. View in context: https://bugs.webkit.org/attachment.cgi?id=248042&action=review I’m not sure we want to add tests with fixed 2+ second delays in them. We should discuss this. Otherwise I would have already said review+. > LayoutTests/ChangeLog:9 > + regions with sroll snap points defined. scroll > LayoutTests/css3/scroll-snap/scroll-snap-mandatory-horizontal.html:1 > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> The HTML5 DOCTYPE is simpler than this: <!DOCTYPE html> I don’t see any reason to use the longer one; not really sure if we should use any at all. > LayoutTests/css3/scroll-snap/scroll-snap-mandatory-horizontal.html:21 > + /*-webkit-scroll-snap-coordinate: 50% 50%;*/ Why include commented out code? > LayoutTests/css3/scroll-snap/scroll-snap-mandatory-horizontal.html:24 > + background-color: #FF0000; red instead? > LayoutTests/css3/scroll-snap/scroll-snap-mandatory-horizontal.html:27 > + background-color: #00FF00; green > LayoutTests/css3/scroll-snap/scroll-snap-mandatory-horizontal.html:30 > + background-color: #0000FF; blue > LayoutTests/css3/scroll-snap/scroll-snap-mandatory-horizontal.html:33 > + background-color: #00FFFF; etc. > LayoutTests/css3/scroll-snap/scroll-snap-mandatory-horizontal.html:74 > + setTimeout(checkForScrollSnap, 1000); It’s not good to have a 1 second delay in a regression test. That means the test will take a full second to run! Also, we want the tests to run even on loaded machines, so dependencies on certain speeds aren’t great. > LayoutTests/css3/scroll-snap/scroll-snap-mandatory-horizontal.html:85 > + setTimeout(scrollSnapTest, 100); Another magic number, 100ms? > LayoutTests/css3/scroll-snap/scroll-snap-mandatory-horizontal.html:103 > + setTimeout(checkForScrollGlide, 1000); Another 1 second delay! > LayoutTests/css3/scroll-snap/scroll-snap-mandatory-vertical-expected.txt:10 > +PASS successfullyParsed is true > + > +TEST COMPLETE > +PASS div scrolled to next window. > +PASS div honored snap points. This is really messy output. Maybe we can clean it up somehow?

Build Bot

Comment 10 2015-03-05 23:45:14 PST

Comment on attachment 248042 [details] Patch to see what testers are encountering. Attachment 248042 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5594101277786112 New failing tests: css3/scroll-snap/scroll-snap-mandatory-vertical.html css3/scroll-snap/scroll-snap-mandatory-horizontal.html

Build Bot

Comment 11 2015-03-05 23:45:18 PST

Created attachment 248046 [details] Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5

Build Bot

Comment 12 2015-03-05 23:48:54 PST

Comment on attachment 248042 [details] Patch to see what testers are encountering. Attachment 248042 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6349123242426368 New failing tests: css3/scroll-snap/scroll-snap-mandatory-vertical.html css3/scroll-snap/scroll-snap-mandatory-horizontal.html

Build Bot

Comment 13 2015-03-05 23:48:58 PST

Created attachment 248047 [details] Archive of layout-test-results from ews100 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5

Brent Fulgham

Comment 14 2015-03-06 09:19:32 PST

Created attachment 248068 [details] Patch

Brent Fulgham

Comment 15 2015-03-06 09:32:30 PST

Comment on attachment 248042 [details] Patch to see what testers are encountering. View in context: https://bugs.webkit.org/attachment.cgi?id=248042&action=review >> LayoutTests/ChangeLog:9 >> + regions with sroll snap points defined. > > scroll Whoops! >> LayoutTests/css3/scroll-snap/scroll-snap-mandatory-horizontal.html:1 >> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> > > The HTML5 DOCTYPE is simpler than this: > > <!DOCTYPE html> > > I don’t see any reason to use the longer one; not really sure if we should use any at all. I'll switch to the short version; we seem to use it in most tests I've looked at (though it's probably not necessary). >> LayoutTests/css3/scroll-snap/scroll-snap-mandatory-horizontal.html:74 >> + setTimeout(checkForScrollSnap, 1000); > > It’s not good to have a 1 second delay in a regression test. That means the test will take a full second to run! Also, we want the tests to run even on loaded machines, so dependencies on certain speeds aren’t great. What we need is some way to register a callback for when the event has completed processing. Unfortunately, this doesn't exist. The delay is based on the time the scroll snap animation takes to run, with enough extra time to account for some variation in machine loading. Do you have any ideas for how we can better model this in testing? >> LayoutTests/css3/scroll-snap/scroll-snap-mandatory-horizontal.html:85 >> + setTimeout(scrollSnapTest, 100); > > Another magic number, 100ms? This one could probably be 0. I'll change that. >> LayoutTests/css3/scroll-snap/scroll-snap-mandatory-horizontal.html:103 >> + setTimeout(checkForScrollGlide, 1000); > > Another 1 second delay! See answer above. >> LayoutTests/css3/scroll-snap/scroll-snap-mandatory-vertical-expected.txt:10 >> +PASS div honored snap points. > > This is really messy output. Maybe we can clean it up somehow? Yes! When a test is asyncrhonous, it's apparently not enough to stick the "js-test-pos.js" script at the bottom of the file. Instead, you have to set "window.jsTestIsAsync = true;" at the start of the test, then call "finishJSTest();" at the end. Then the test output is correct.

Brent Fulgham

Comment 16 2015-05-01 17:56:12 PDT

These patches are running great in isolation, but when run in conjunction with the normal scroll tests they start failing. There must be some state being retained from run-to-run.

Brent Fulgham

Comment 17 2015-05-01 18:27:02 PDT

Created attachment 252211 [details] Patch v3 - resolve flakiness.

Brent Fulgham

Comment 18 2015-05-01 18:31:28 PDT

I had been issuing the wheel event commands in async mode, which doesn't queues them up for use by the test runner. Unfortunately, since they return immediately we could reach the "set the test callback" step before any scroll activity had started. If no one flagged a reason not to run the test, the test would fire immediately, causing it to check state before the wheel events have caused anything to move around.

Brent Fulgham

Comment 19 2015-05-01 18:46:08 PDT

Committed r183707: <http://trac.webkit.org/changeset/183707>

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