third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html - Issue 2769793002: Implement CSS: scroll-boundary-behavior

Side by Side Diff: third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html

Issue 2769793002: Implement CSS: scroll-boundary-behavior (Closed)

Patch Set: Disable touch on mac Created 3 years, 5 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:

« cc/trees/layer_tree_host_impl.cc ('K') | « cc/trees/scroll_node.cc ('k') | third_party/WebKit/LayoutTests/webexposed/css-properties-as-js-properties-expected.txt » ('j') | third_party/WebKit/Source/core/input/ScrollManager.cpp » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
(Empty)
	1 <!DOCTYPE html>

	2 <script src="../../resources/testharness.js"></script>

	3 <script src="../../resources/testharnessreport.js"></script>

	4 <div id='space1' style='height: 300px; width: 1000px;'></div>

	5 <div id='container' style='height: 400px; width: 500px; overflow: scroll; will-c hange: transform'>
	bokan 2017/06/30 15:44:12 Please remove will-change: transform. This forces Please remove will-change: transform. This forces the scroller to be composited. I think it technically doesn't matter since you'll still test the main thread path in the non-virtual test suite but the virtual/threaded/fast/scroll-behavior tests run with --enable-prefer-compositing-to-lcd-text which should cause this scroller to be composited anyway. It'd be better to let the normal test not composite this. (If for some reason that doesn't work (ensure we're hitting both main and impl paths) then I wouldn't worry about it. sunyunjia 2017/06/30 18:26:58 Done. Show quoted text On 2017/06/30 15:44:12, bokan wrote: > Please remove will-change: transform. This forces the scroller to be composited. > I think it technically doesn't matter since you'll still test the main thread > path in the non-virtual test suite but the virtual/threaded/fast/scroll-behavior > tests run with --enable-prefer-compositing-to-lcd-text which should cause this > scroller to be composited anyway. It'd be better to let the normal test not > composite this. (If for some reason that doesn't work (ensure we're hitting both > main and impl paths) then I wouldn't worry about it. Done.
	6 <div id='red' style='height: 300px; width: 1000px; background-color: red; will -change: transform'></div>

	7 <div id='blue' style='height: 300px; background-color: blue; will-change: tran sform'></div>
	bokan 2017/06/30 15:44:11 Nit: These inner <div>s don't need `will-change:tr Nit: These inner <div>s don't need `will-change:transform` sunyunjia 2017/06/30 18:26:58 Done. Show quoted text On 2017/06/30 15:44:11, bokan wrote: > Nit: These inner <div>s don't need `will-change:transform` Done.
	8 </div>

	9 <div id='space2' style='height: 1000px;'></div>

	10

	11 <script>

	12 const TOUCH_SOURCE_TYPE = 1; // TOUCH_INPUT from synthetic_gesture_params.h

	13 const WHEEL_SOURCE_TYPE = 2; // MOUSE_INPUT from synthetic_gesture_params.h

	14 const TYPES = ["", "touch", "wheel"];

	15 const MAX_FRAME = 500;

	16 var container = document.getElementById('container');

	17 var begin_type = navigator.platform.indexOf('Mac') == 0 ? WHEEL_SOURCE_TYPE

	18 : TOUCH_SOURCE_TYPE;

	19 test_y(begin_type);

	20

	21 function waitForAnimationEnd(next_function) {

	22 var last_changed_frame = 0;

	23 var last_window_x = window.scrollX;

	24 var last_window_y = window.scrollY;

	25 var last_container_x = container.scrollLeft;

	26 var last_container_y = container.scrollTop;

	27 tick(0);

	28 function tick(frames) {

	29 // We requestAnimationFrame either for 500 frames or until 20 frames with
	bokan 2017/06/30 15:44:12 If this is a callback that's called after the gest If this is a callback that's called after the gesture is completed, why do we have to wait? sunyunjia 2017/06/30 18:26:58 We don't need to wait for touch, but need to wait Show quoted text On 2017/06/30 15:44:12, bokan wrote: > If this is a callback that's called after the gesture is completed, why do we > have to wait? We don't need to wait for touch, but need to wait for the wheel event to finish the animation. bokan 2017/06/30 18:55:36 We should turn off wheel animations, they're unrel Show quoted text On 2017/06/30 18:26:58, sunyunjia wrote: > On 2017/06/30 15:44:12, bokan wrote: > > If this is a callback that's called after the gesture is completed, why do we > > have to wait? > > We don't need to wait for touch, but need to wait for the wheel event to finish > the animation. We should turn off wheel animations, they're unrelated to what we're trying to test and can only add flakiness. I think you should be able to do that with internals.settings.setScrollAnimatorEnabled sunyunjia 2017/07/14 02:59:00 I think it can help test the ScrollAnimated() bran Show quoted text On 2017/06/30 18:55:36, bokan wrote: > On 2017/06/30 18:26:58, sunyunjia wrote: > > On 2017/06/30 15:44:12, bokan wrote: > > > If this is a callback that's called after the gesture is completed, why do > we > > > have to wait? > > > > We don't need to wait for touch, but need to wait for the wheel event to > finish > > the animation. > > We should turn off wheel animations, they're unrelated to what we're trying to > test and can only add flakiness. I think you should be able to do that with > internals.settings.setScrollAnimatorEnabled I think it can help test the ScrollAnimated() branch? I tried internals.settings.setScrollAnimatorEnabled(false) and removed the animation, both touch_type and wheel_type passed on main thread but failed on impl thread. I did some codesearch and noticed that internals.settings.XXX are mainly used for eventSender. I guess this setting only works for main thread. majidvp 2017/07/14 14:29:43 Impl thread has its own internal flag that control Show quoted text On 2017/07/14 02:59:00, sunyunjia wrote: > On 2017/06/30 18:55:36, bokan wrote: > > On 2017/06/30 18:26:58, sunyunjia wrote: > > > On 2017/06/30 15:44:12, bokan wrote: > > > > If this is a callback that's called after the gesture is completed, why do > > we > > > > have to wait? > > > > > > We don't need to wait for touch, but need to wait for the wheel event to > > finish > > > the animation. > > > > We should turn off wheel animations, they're unrelated to what we're trying to > > test and can only add flakiness. I think you should be able to do that with > > internals.settings.setScrollAnimatorEnabled > > I think it can help test the ScrollAnimated() branch? > > I tried internals.settings.setScrollAnimatorEnabled(false) and removed the > animation, both touch_type and wheel_type passed on main thread but failed on > impl thread. I did some codesearch and noticed that internals.settings.XXX are > mainly used for eventSender. I guess this setting only works for main thread. Impl thread has its own internal flag that controls smooth scrolling InputHandlerProxy::smooth_scroll_enabled_. This is actually controlled by command line flags during initialization but I suspect that it should not be too complex to set this impl flag from WebView anytime that internals.settings.setScrollAnimatorEnable is called. sunyunjia 2017/07/14 22:07:23 Seems like internals.settings.setScrollAnimatorEna Show quoted text On 2017/07/14 14:29:43, majidvp wrote: > On 2017/07/14 02:59:00, sunyunjia wrote: > > On 2017/06/30 18:55:36, bokan wrote: > > > On 2017/06/30 18:26:58, sunyunjia wrote: > > > > On 2017/06/30 15:44:12, bokan wrote: > > > > > If this is a callback that's called after the gesture is completed, why > do > > > we > > > > > have to wait? > > > > > > > > We don't need to wait for touch, but need to wait for the wheel event to > > > finish > > > > the animation. > > > > > > We should turn off wheel animations, they're unrelated to what we're trying > to > > > test and can only add flakiness. I think you should be able to do that with > > > internals.settings.setScrollAnimatorEnabled > > > > I think it can help test the ScrollAnimated() branch? > > > > I tried internals.settings.setScrollAnimatorEnabled(false) and removed the > > animation, both touch_type and wheel_type passed on main thread but failed on > > impl thread. I did some codesearch and noticed that internals.settings.XXX are > > mainly used for eventSender. I guess this setting only works for main thread. > > > > Impl thread has its own internal flag that controls smooth scrolling > InputHandlerProxy::smooth_scroll_enabled_. This is actually controlled by > command line flags during initialization but I suspect that it should not be too > complex to set this impl flag from WebView anytime that > internals.settings.setScrollAnimatorEnable > is called. Seems like internals.settings.setScrollAnimatorEnabled modifies page.settings by a series of generated code. And the main_thread flag is set by looking at the value in settings. I consulted dtapuska@ about this and he said we could disable the animator it by disabling the smooth_scroll runtime flag for all virtual/threaded tests, which is not worth it. Instead, he suggests waiting for some frames as it simulates the actual user behavior. majidvp 2017/07/17 15:49:30 This is fine. Following our offline discussion I f Show quoted text On 2017/07/14 22:07:23, sunyunjia wrote: > On 2017/07/14 14:29:43, majidvp wrote: > > On 2017/07/14 02:59:00, sunyunjia wrote: > > > On 2017/06/30 18:55:36, bokan wrote: > > > > On 2017/06/30 18:26:58, sunyunjia wrote: > > > > > On 2017/06/30 15:44:12, bokan wrote: > > > > > > If this is a callback that's called after the gesture is completed, > why > > do > > > > we > > > > > > have to wait? > > > > > > > > > > We don't need to wait for touch, but need to wait for the wheel event to > > > > finish > > > > > the animation. > > > > > > > > We should turn off wheel animations, they're unrelated to what we're > trying > > to > > > > test and can only add flakiness. I think you should be able to do that > with > > > > internals.settings.setScrollAnimatorEnabled > > > > > > I think it can help test the ScrollAnimated() branch? > > > > > > I tried internals.settings.setScrollAnimatorEnabled(false) and removed the > > > animation, both touch_type and wheel_type passed on main thread but failed > on > > > impl thread. I did some codesearch and noticed that internals.settings.XXX > are > > > mainly used for eventSender. I guess this setting only works for main > thread. > > > > > > > > Impl thread has its own internal flag that controls smooth scrolling > > InputHandlerProxy::smooth_scroll_enabled_. This is actually controlled by > > command line flags during initialization but I suspect that it should not be > too > > complex to set this impl flag from WebView anytime that > > internals.settings.setScrollAnimatorEnable > > is called. > Seems like internals.settings.setScrollAnimatorEnabled modifies page.settings by > a series of generated code. And the main_thread flag is set by looking at the > value in settings. > > I consulted dtapuska@ about this and he said we could disable the animator it by > disabling the smooth_scroll runtime flag for all virtual/threaded tests, which > is not worth it. Instead, he suggests waiting for some frames as it simulates > the actual user behavior. This is fine. Following our offline discussion I filed a separate bug https://crbug.com/744666 to explore various option to disable/control threaded scroll animations in layout tests. nit: You actually only need to wait for some frames for wheel! The touch scrolling does not do any animation which means by the time the callback is called it should be finished.
	30 // no change have been observed.

	31 if (frames >= MAX_FRAME \|\| frames - last_changed_frame > 20) {

	32 next_function();

	33 } else {

	34 if (window.scrollX != last_window_x \|\|

	35 window.scrollY != last_window_y \|\|

	36 container.scrollLeft != last_container_x \|\|

	37 container.scrollTop != last_container_y) {

	38 last_changed_frame = frames;

	39 last_window_x = window.scrollX;

	40 last_window_y = window.scrollY;

	41 last_container_x = container.scrollLeft;

	42 last_container_y = container.scrollTop;

	43 }

	44 requestAnimationFrame(tick.bind(null, frames + 1));

	45 }

	46 };

	47 }

	48

	49 function test_y(source_type) {

	50 var test = async_test('scroll-boundary-behavior-y: none should only prevent sc roll propagation on y axis with: ' + TYPES[source_type]);

	51 function checkNoChangeAndScroll() {

	52 assert_equals(window.scrollY, 100);

	53 chrome.gpuBenchmarking.smoothScrollBy(200, waitForAnimationEnd.bind(this, ch eckChangeAndFinish), 100, 400, source_type, "left", 4000);

	54 }

	55 function checkChangeAndFinish() {

	56 assert_equals(window.scrollX, 0);

	57 if (source_type == TOUCH_SOURCE_TYPE) {
	bokan 2017/06/30 15:44:11 make begin_type always start with WHEEL and move t make begin_type always start with WHEEL and move the `indexOf('Max')` check in here so that it's more clear that we avoid the TOUCH test on Mac. sunyunjia 2017/06/30 18:26:58 Done. Show quoted text On 2017/06/30 15:44:11, bokan wrote: > make begin_type always start with WHEEL and move the `indexOf('Max')` check in > here so that it's more clear that we avoid the TOUCH test on Mac. Done.
	58 test_y(WHEEL_SOURCE_TYPE);

	59 test.done();
	bokan 2017/06/30 15:44:12 test.done() should be called first, no? (below too test.done() should be called first, no? (below too) sunyunjia 2017/06/30 18:26:58 The later tests won't run if I call test.done() fi Show quoted text On 2017/06/30 15:44:12, bokan wrote: > test.done() should be called first, no? (below too) The later tests won't run if I call test.done() first.
	60 } else {

	61 test_x(begin_type);

	62 test.done();

	63 }

	64 }

	65

	66 test.step(function() {

	67 container.style.scrollBoundaryBehaviorX = 'auto';

	68 container.style.scrollBoundaryBehaviorY = 'none';

	69 window.scrollTo(100, 100);

	70 container.scrollTo(0, 0);

	71 assert_equals(window.scrollY, 100);

	72 assert_equals(window.scrollX, 100);

	73 chrome.gpuBenchmarking.smoothScrollBy(200, waitForAnimationEnd.bind(this, checkNoChangeAndScroll), 100, 400, source_type, "up", 4000);

	74 });

	75 }

	76

	77 function test_x(source_type) {

	78 var test = async_test('scroll-boundary-behavior-x: none should only prevent sc roll propagation on x axis with: ' + TYPES[source_type]);

	79 function checkNoChangeAndScroll() {

	80 assert_equals(window.scrollX, 100);

	81 chrome.gpuBenchmarking.smoothScrollBy(200, waitForAnimationEnd.bind(this, ch eckChangeAndFinish), 100, 400, source_type, "up", 4000);

	82 }

	83 function checkChangeAndFinish() {

	84 assert_equals(window.scrollY, 0);

	85 if (source_type == TOUCH_SOURCE_TYPE) {

	86 test_x(WHEEL_SOURCE_TYPE);

	87 test.done();

	88 } else {

	89 test_scroll(begin_type);

	90 test.done();

	91 }

	92 }

	93

	94 test.step(function() {

	95 container.style.scrollBoundaryBehaviorX = 'none';

	96 container.style.scrollBoundaryBehaviorY = 'auto';

	97 window.scrollTo(100, 100);

	98 container.scrollTo(0, 0);

	99 assert_equals(window.scrollY, 100);

	100 assert_equals(window.scrollX, 100);

	101 chrome.gpuBenchmarking.smoothScrollBy(200, waitForAnimationEnd.bind(this, checkNoChangeAndScroll), 100, 400, source_type, "left", 4000);

	102 });

	103 }

	104

	105 function test_scroll(source_type) {

	106 var test = async_test('scroll-boundary-behavior should not affect scrolling in side the applied container with: ' + TYPES[source_type]);

	107 function checkChangeAndFinish() {

	108 assert_equals(container.scrollTop, 0);

	109 assert_equals(container.scrollLeft, 0);

	110 if (source_type == TOUCH_SOURCE_TYPE) {

	111 test_scroll(WHEEL_SOURCE_TYPE);

	112 test.done();

	113 } else {

	114 test.done();

	115 }

	116 }

	117

	118 test.step(function() {

	119 container.style.scrollBoundaryBehaviorX = 'none';

	120 container.style.scrollBoundaryBehaviorY = 'none';

	121 window.scrollTo(0, 0);

	122 container.scrollTo(100, 100);

	123 assert_equals(container.scrollTop, 100);

	124 assert_equals(container.scrollLeft, 100);

	125 chrome.gpuBenchmarking.smoothScrollBy(200, waitForAnimationEnd.bind(this, checkChangeAndFinish), 100, 400, source_type, "upleft", 4000);
	bokan 2017/06/30 15:44:11 nit: extra spaces between `this, checkChangeAndF nit: extra spaces between `this, checkChangeAndFinish` sunyunjia 2017/06/30 18:26:58 Done. Show quoted text On 2017/06/30 15:44:11, bokan wrote: > nit: extra spaces between `this, checkChangeAndFinish` Done.
	126 });

	127 }

	128

	129 </script>

OLD	NEW