LayoutTests/scrollingcoordinator/non-fast-scrollable-animation.html - Issue 1256043007: Recompute slow scroll regions after overflow recalc

Side by Side Diff: LayoutTests/scrollingcoordinator/non-fast-scrollable-animation.html

Issue 1256043007: Recompute slow scroll regions after overflow recalc (Closed) Base URL: https://chromium.googlesource.com/chromium/blink.git@master

Patch Set: Minor test updates Created 5 years, 4 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

OLD	NEW
(Empty)
	1 <!DOCTYPE html>

	2 <style>

	3 body {

	4 margin: 0;

	5 }

	6

	7 .container {

	8 height: 250px;

	9 width: 250px;

	10 display: block;

	11 }

	12

	13 #nonFastRegion {

	14 height: 222px;

	15 width: 222px;

	16 display: block;

	17 border: none;

	18 }

	19

	20 #nonFastRegion.animate {

	21 transform: translateX(100px) translateY(100px);

	22 transition: transform 5ms;

	23 }

	24 </style>

	25

	26 <div class="container">

	27 <iframe id='nonFastRegion' onload='runAfterLayoutAndPaint(window.runTests)' sr c="data:text/html;charset=utf-8,<div style='height:1000px; width:1000px; backgro und: linear-gradient(to bottom, red, white);'>This should be covered.</div>">
	Rick Byers 2015/08/05 15:02:59 nit: any reason to use 'src=data:text/html;...' in nit: any reason to use 'src=data:text/html;...' instead of the more modern 'srcdoc='? majidvp 2015/08/05 15:39:34 Acknowledged. Show quoted text On 2015/08/05 15:02:59, Rick Byers wrote: > nit: any reason to use 'src=data:text/html;...' instead of the more modern > 'srcdoc='? Acknowledged.
	28 </iframe>
	Rick Byers 2015/08/05 15:02:59 The original bug report relied only on a div, not The original bug report relied only on a div, not an iframe. Any reason you choose the more complicated iframe case here? I assume the FrameView method you hooked into gets invoked for the div case too, so you still fixed the original bug right? majidvp 2015/08/05 15:39:35 The original case is fixed because the same code p Show quoted text On 2015/08/05 15:02:59, Rick Byers wrote: > The original bug report relied only on a div, not an iframe. Any reason you > choose the more complicated iframe case here? I assume the FrameView method you > hooked into gets invoked for the div case too, so you still fixed the original > bug right? The original case is fixed because the same code path is exercised. The div's are not non-fast region on HDPI displays but iframes are. This means that if you load the test interactively it is more likely that it works as expected with iframe which is why I used it. Note run-webkit-tests runs in LDPI mode by default so it does not matter there. Rick Byers 2015/08/06 03:15:19 Are you sure about this? I believe iframes are fa Show quoted text On 2015/08/05 15:39:35, majidvp wrote: > On 2015/08/05 15:02:59, Rick Byers wrote: > > The original bug report relied only on a div, not an iframe. Any reason you > > choose the more complicated iframe case here? I assume the FrameView method > you > > hooked into gets invoked for the div case too, so you still fixed the original > > bug right? > > The original case is fixed because the same code path is exercised. The div's > are not non-fast region on HDPI displays but iframes are. Are you sure about this? I believe iframes are fast-scrolling on Android and ChromeOS high-dpi at least (although IIRC there was a separate set of flags involved in enabling that - maybe there is some confusion there?). Show quoted text > This means that if you > load the test interactively it is more likely that it works as expected with > iframe which is why I used it. Note run-webkit-tests runs in LDPI mode by > default so it does not matter there.
	29 </div>

	30

	31 <p>A single square should be visible covered by a green overlay.</p>

	32

	33 <div id="console"></div>

	34

	35 <script src="../resources/js-test.js"></script>

	36 <script src="../resources/run-after-layout-and-paint.js"></script>

	37 <script src="resources/non-fast-scrollable-region-testing.js"></script>

	38 <script>

	39 jsTestIsAsync = true;

	40 // print result lazily to avoid layouts during the test

	41 setPrintTestResultsLazily();

	42 description('This test ensures that animating a non-fast scrollable area ' +

	43 'correctly updates list of non-fast scrollable rects ' +

	44 '(See http://crbug.com/417345).');

	45

	46 runTests = function() {

	47 nonFastScrollableRects = internals.nonFastScrollableRects(document);

	48 shouldBe('nonFastScrollableRects.length', '1');

	49 shouldBeEqualToString('rectToString(nonFastScrollableRects[0])', '[0, 0, 222 , 222]');

	50

	51 // animating transforms hits an optimized layout path which is root cause of

	52 // http://crbug.com/417345

	53 var nonFast = document.getElementById('nonFastRegion');

	54 nonFast.classList.add('animate');

	55

	56 // wait for 1 frame plus animation duration before checking
	Rick Byers 2015/08/05 15:02:59 I'm a little worried that this test is brittle to I'm a little worried that this test is brittle to other things that trigger layout (if layout got triggered accidentally the test would pass without testing your fix). Can you add some checks for window.internals layout APIs to verify that a layout hasn't occurred? I think the typical pattern is to dirty layout and verify that needsLayoutCount is still non-zero at the end of your test (eg. see my scrollingElement-noLayout-quirks.html test). majidvp 2015/08/05 15:39:34 This makes sense. Thanks for pointing it out. Show quoted text On 2015/08/05 15:02:59, Rick Byers wrote: > I'm a little worried that this test is brittle to other things that trigger > layout (if layout got triggered accidentally the test would pass without testing > your fix). Can you add some checks for window.internals layout APIs to verify > that a layout hasn't occurred? I think the typical pattern is to dirty layout > and verify that needsLayoutCount is still non-zero at the end of your test (eg. > see my scrollingElement-noLayout-quirks.html test). This makes sense. Thanks for pointing it out.
	57 waitForAnimationWalltime(5 + 16).then(function() {

	58 nonFastScrollableRects = internals.nonFastScrollableRects(document);

	59 shouldBe('nonFastScrollableRects.length', '1');

	60 shouldBeEqualToString('rectToString(nonFastScrollableRects[0])', '[100, 10 0, 222, 222]');

	61

	62 runAfterLayoutAndPaint(function() {

	63 // Add green overlays to help visualize the test

	64 drawNonFastScrollableRegionOverlays();

	65 finishJSTest();

	66 });

	67 });

	68 }

	69

	70 function waitForAnimationWalltime(wait) {
	Rick Byers 2015/08/05 15:02:59 Is this guaranteed to be non-flaky? I thought it Is this guaranteed to be non-flaky? I thought it was possible for the animation to take longer than this (eg. under heavy load). Can you point to an example of another animation-time-dependent test for how we normally handle cases like this? Alternately maybe we could use the transitionend event? Or even just a single transform instead of an animation (just need to be careful to ensure you don't start the test until all loading/layouts are done)? majidvp 2015/08/05 15:39:34 I was originally looking at using webkitanimatione Show quoted text On 2015/08/05 15:02:59, Rick Byers wrote: > Is this guaranteed to be non-flaky? I thought it was possible for the animation > to take longer than this (eg. under heavy load). Can you point to an example of > another animation-time-dependent test for how we normally handle cases like > this? > > Alternately maybe we could use the transitionend event? Or even just a single > transform instead of an animation (just need to be careful to ensure you don't > start the test until all loading/layouts are done)? I was originally looking at using webkitanimationend event but that didn't fire for transition animation. I was not aware of \|transitionend\|. Using that makes sense.
	71 return new Promise(function(resolve, reject) {

	72 var start;

	73 window.requestAnimationFrame(rAF);

	74 function rAF(now) {

	75 start = start \|\| now;

	76 if (now - start > wait)

	77 resolve();

	78 else

	79 window.requestAnimationFrame(rAF);

	80 }

	81 });

	82 }

	83 </script>

OLD	NEW

« no previous file with comments | « no previous file | LayoutTests/scrollingcoordinator/non-fast-scrollable-animation-expected.txt » ('j') | Source/core/frame/FrameView.cpp » ('J')