Bug 154300 - Add tests for iframe and overflow scrollability after navigating back
Summary: Add tests for iframe and overflow scrollability after navigating back
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-02-16 11:18 PST by Simon Fraser (smfr)
Modified: 2016-02-17 13:44 PST (History)
5 users (show)

See Also:


Attachments
Patch (9.40 KB, patch)
2016-02-16 11:20 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (10.16 KB, patch)
2016-02-16 11:32 PST, Simon Fraser (smfr)
bfulgham: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (1.24 MB, application/zip)
2016-02-16 12:35 PST, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-yosemite (993.34 KB, application/zip)
2016-02-16 12:46 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2016-02-16 11:18:38 PST
Add tests for iframe and overflow scrollability after navigating back
Comment 1 Simon Fraser (smfr) 2016-02-16 11:20:18 PST
Created attachment 271452 [details]
Patch
Comment 2 Simon Fraser (smfr) 2016-02-16 11:32:55 PST
Created attachment 271458 [details]
Patch
Comment 3 Brent Fulgham 2016-02-16 11:45:19 PST
Comment on attachment 271458 [details]
Patch

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

r=me, but please consider only checking for "changed" rather than specific scroll position numbers.

> LayoutTests/fast/scrolling/iframe-scrollable-after-back.html:35
> +            logResult('Scrolled; scrollTop is ' + document.getElementById('iframe').contentDocument.scrollingElement.scrollTop);

This might be flaky on different machines (e.g., with different scroll precision). You might just confirm that the scrollTop changed from whatever value you see at the start of the test.
Comment 4 Simon Fraser (smfr) 2016-02-16 11:46:02 PST
(In reply to comment #3)
> Comment on attachment 271458 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271458&action=review
> 
> r=me, but please consider only checking for "changed" rather than specific
> scroll position numbers.
> 
> > LayoutTests/fast/scrolling/iframe-scrollable-after-back.html:35
> > +            logResult('Scrolled; scrollTop is ' + document.getElementById('iframe').contentDocument.scrollingElement.scrollTop);
> 
> This might be flaky on different machines (e.g., with different scroll
> precision). You might just confirm that the scrollTop changed from whatever
> value you see at the start of the test.

If it is, that's a problem with the test harness.
Comment 5 Build Bot 2016-02-16 12:35:41 PST
Comment on attachment 271458 [details]
Patch

Attachment 271458 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/841496

New failing tests:
fast/scrolling/overflow-scrollable-after-back.html
fast/scrolling/iframe-scrollable-after-back.html
Comment 6 Build Bot 2016-02-16 12:35:44 PST
Created attachment 271469 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 7 Build Bot 2016-02-16 12:46:45 PST
Comment on attachment 271458 [details]
Patch

Attachment 271458 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/841508

New failing tests:
fast/scrolling/overflow-scrollable-after-back.html
fast/scrolling/iframe-scrollable-after-back.html
Comment 8 Build Bot 2016-02-16 12:46:48 PST
Created attachment 271471 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 9 Simon Fraser (smfr) 2016-02-16 15:24:35 PST
https://trac.webkit.org/r196665
Comment 10 Brent Fulgham 2016-02-16 17:20:16 PST
(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 271458 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=271458&action=review
> > 
> > r=me, but please consider only checking for "changed" rather than specific
> > scroll position numbers.
> > 
> > > LayoutTests/fast/scrolling/iframe-scrollable-after-back.html:35
> > > +            logResult('Scrolled; scrollTop is ' + document.getElementById('iframe').contentDocument.scrollingElement.scrollTop);
> > 
> > This might be flaky on different machines (e.g., with different scroll
> > precision). You might just confirm that the scrollTop changed from whatever
> > value you see at the start of the test.
> 
> If it is, that's a problem with the test harness.

Ermmmm (from test failures):

 Sending wheel event; scrollTop is 0
-Scrolled; scrollTop is 20
+Scrolled; scrollTop is 28
Comment 11 Simon Fraser (smfr) 2016-02-16 17:42:08 PST
I skipped on WK1 when landing.
Comment 12 Carlos Garcia Campos 2016-02-16 22:59:41 PST
(In reply to comment #10)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > Comment on attachment 271458 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=271458&action=review
> > > 
> > > r=me, but please consider only checking for "changed" rather than specific
> > > scroll position numbers.
> > > 
> > > > LayoutTests/fast/scrolling/iframe-scrollable-after-back.html:35
> > > > +            logResult('Scrolled; scrollTop is ' + document.getElementById('iframe').contentDocument.scrollingElement.scrollTop);
> > > 
> > > This might be flaky on different machines (e.g., with different scroll
> > > precision). You might just confirm that the scrollTop changed from whatever
> > > value you see at the start of the test.
> > 
> > If it is, that's a problem with the test harness.
> 
> Ermmmm (from test failures):
> 
>  Sending wheel event; scrollTop is 0
> -Scrolled; scrollTop is 20
> +Scrolled; scrollTop is 28

Similar failures in GTK+ where minimum scroll is 40.
Comment 13 Simon Fraser (smfr) 2016-02-17 09:05:13 PST
I'll fix the tests.
Comment 14 Carlos Garcia Campos 2016-02-17 09:08:38 PST
(In reply to comment #13)
> I'll fix the tests.

Thanks!, I also think that just checking that the view did actually scroll should be enough as Brent suggested, how much it scrolled doesn't really matter in this test.
Comment 15 Simon Fraser (smfr) 2016-02-17 13:44:11 PST
Tests fixed in https://trac.webkit.org/r196719