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: update the tests 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/property_tree_builder.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/layout/compositing/CompositedLayerMapping.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>
	majidvp 2017/07/06 19:43:59 nit: blank line nit: blank line sunyunjia 2017/07/14 02:59:00 Done. Show quoted text On 2017/07/06 19:43:59, majidvp wrote: > nit: blank line Done.
	4 <div id='space1' style='height: 300px; width: 1000px;'></div>

	5 <div id='container' style='height: 400px; width: 500px; overflow: scroll;'>
	majidvp 2017/07/06 19:43:59 Best to move these styles into a <style> element. Best to move these styles into a <style> element. Also it seems possible to dedup multiple of them with a common class (e.g., heigh:300px; width:1000px) sunyunjia 2017/07/14 02:59:00 Done. Show quoted text On 2017/07/06 19:43:59, majidvp wrote: > Best to move these styles into a <style> element. > > Also it seems possible to dedup multiple of them with a common > class (e.g., heigh:300px; width:1000px) Done.
	6 <div id='red' style='height: 300px; width: 1000px; background-color: red;'></d iv>

	7 <div id='blue' style='height: 300px; background-color: blue;'></div>
	majidvp 2017/07/06 19:43:59 What is the significant of these colors? Why even What is the significant of these colors? Why even have two? sunyunjia 2017/07/14 02:59:00 Thanks for pointing this out. They were originally Show quoted text On 2017/07/06 19:43:59, majidvp wrote: > What is the significant of these colors? Why even have two? Thanks for pointing this out. They were originally for manually debugging. I've removed them.
	8 </div>

	9 <div id='space2' style='height: 1000px;'></div>
	majidvp 2017/07/06 19:43:59 why do you need two 'space' div one before and one why do you need two 'space' div one before and one after? sunyunjia 2017/07/14 02:59:01 ditto Show quoted text On 2017/07/06 19:43:59, majidvp wrote: > why do you need two 'space' div one before and one after? ditto
	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 const IS_MAC = navigator.platform.indexOf('Mac') == 0;

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

	18 test_y(WHEEL_SOURCE_TYPE);

	19

	20 function waitForAnimationEnd(next_function) {

	21 var last_changed_frame = 0;

	22 var last_window_x = window.scrollX;

	23 var last_window_y = window.scrollY;

	24 var last_container_x = container.scrollLeft;

	25 var last_container_y = container.scrollTop;

	26 tick(0);

	27 function tick(frames) {

	28 // We requestAnimationFrame either for 500 frames or until 20 frames with

	29 // no change have been observed.

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

	31 next_function();

	32 } else {

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

	34 window.scrollY != last_window_y \|\|

	35 container.scrollLeft != last_container_x \|\|

	36 container.scrollTop != last_container_y) {

	37 last_changed_frame = frames;

	38 last_window_x = window.scrollX;

	39 last_window_y = window.scrollY;

	40 last_container_x = container.scrollLeft;

	41 last_container_y = container.scrollTop;

	42 }

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

	44 }

	45 };

	46 }

	47

	48 function test_y(source_type) {

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

	50 function checkNoChangeAndScroll() {

	51 assert_equals(window.scrollY, 100);

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

	53 }

	54 function checkChangeAndFinish() {

	55 assert_equals(window.scrollX, 0);

	56 if (source_type == WHEEL_SOURCE_TYPE && !IS_MAC) {

	57 test_y(TOUCH_SOURCE_TYPE);

	58 test.done();

	59 } else {

	60 test_x(WHEEL_SOURCE_TYPE);

	61 test.done();

	62 }

	63 }

	64

	65 test.step(function() {

	66 container.style.scrollBoundaryBehaviorX = 'auto';

	67 container.style.scrollBoundaryBehaviorY = 'none';

	68 window.scrollTo(100, 100);

	69 container.scrollTo(0, 0);

	70 assert_equals(window.scrollY, 100);

	71 assert_equals(window.scrollX, 100);
	majidvp 2017/07/06 19:43:59 The above 4 lines are just a setup and repeated fo The above 4 lines are just a setup and repeated for all tests. Please create a function e.g., setUp that does this and use that. In fact, testharness.js has some way[1] to define a setup but I am not sure if it works as expected for async tests. Feel free to use that if it works. [1] http://web-platform-tests.org/writing-tests/testharness-api.html#setup sunyunjia 2017/07/14 02:59:01 Done. Show quoted text On 2017/07/06 19:43:59, majidvp wrote: > The above 4 lines are just a setup and repeated for all tests. > Please create a function e.g., setUp that does this and use that. > > In fact, testharness.js has some way[1] to define a setup but I am > not sure if it works as expected for async tests. Feel free to > use that if it works. > > [1] http://web-platform-tests.org/writing-tests/testharness-api.html#setup Done.
	72 chrome.gpuBenchmarking.smoothScrollBy(200, waitForAnimationEnd.bind(this, checkNoChangeAndScroll), 100, 400, source_type, "up", 4000);

	73 });

	74 }

	75

	76 function test_x(source_type) {

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

	78 function checkNoChangeAndScroll() {

	79 assert_equals(window.scrollX, 100);

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

	81 }

	82 function checkChangeAndFinish() {

	83 assert_equals(window.scrollY, 0);

	84 if (source_type == WHEEL_SOURCE_TYPE && !IS_MAC) {

	85 test_x(TOUCH_SOURCE_TYPE);
	majidvp 2017/07/06 19:43:59 I have difficulty reading and understanding what t I have difficulty reading and understanding what this test is trying to do. The main problem I encountered is that the actual test logics are intermixed with async control flow to sequence multiple tests. In particular, test_y triggers another test_y or test_x based on what test_y occurred before. I suggest these two concerns be separated. A neat way to do this is to have the test functions return a promise and then start next test once the first test promise is resolved e test_y(wheel).then(()=> { test_y(touch).then( () => {... } } In fact, if you do it that way you can use \|promise_test\| facility from test_harness which does exactly what you need [1]. So you can just do: promise_test(test_y('wheel')); promise_test(test_y('touch')); promise_test(test_x('wheel')); ... And your test function will return a promise instead for example: test_y = function(type) { return new Promise(resolve => { // do async work that calls back to waitForAnimationEnd.bind(this, resolve) // which resolves this promise. }).then(() => { // verify result by asserts }); } [1] http://web-platform-tests.org/writing-tests/testharness-api.html "Unlike Asynchronous Tests, Promise Tests don’t start running until after the previous Promise Test finishes." sunyunjia 2017/07/14 02:59:00 Done. Show quoted text On 2017/07/06 19:43:59, majidvp wrote: > I have difficulty reading and understanding what this test is trying to do. The > main problem I > encountered is that the actual test logics are intermixed with async control > flow to sequence > multiple tests. In particular, test_y triggers another test_y or test_x based on > what test_y > occurred before. > > I suggest these two concerns be separated. A neat way to do this is to have the > test functions > return a promise and then start next test once the first test promise is > resolved e > > test_y(wheel).then(()=> { > test_y(touch).then( () => {... } > } > > In fact, if you do it that way you can use \|promise_test\| facility from > test_harness which does > exactly what you need [1]. So you can just do: > promise_test(test_y('wheel')); > promise_test(test_y('touch')); > promise_test(test_x('wheel')); > ... > > And your test function will return a promise instead for example: > > test_y = function(type) { > return new Promise(resolve => { > // do async work that calls back to waitForAnimationEnd.bind(this, resolve) > // which resolves this promise. > }).then(() => { > // verify result by asserts > }); > } > > > > [1] http://web-platform-tests.org/writing-tests/testharness-api.html > "Unlike Asynchronous Tests, Promise Tests don’t start running until after > the previous Promise Test finishes." Done.
	86 test.done();

	87 } else {

	88 test_scroll(WHEEL_SOURCE_TYPE);

	89 test.done();

	90 }

	91 }

	92

	93 test.step(function() {

	94 container.style.scrollBoundaryBehaviorX = 'none';

	95 container.style.scrollBoundaryBehaviorY = 'auto';

	96 window.scrollTo(100, 100);

	97 container.scrollTo(0, 0);

	98 assert_equals(window.scrollY, 100);

	99 assert_equals(window.scrollX, 100);

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

	101 });

	102 }

	103

	104 function test_scroll(source_type) {
	majidvp 2017/07/06 19:43:59 The function names test_x, test_y, test_scroll do The function names test_x, test_y, test_scroll do no match. The first two are test cases for when scroll is large enough to cause the boundary action to get triggered while last one does not do so. Perhaps: test_boundary_scroll_x test_boundary_scroll_y test_none_boundary_scroll sunyunjia 2017/07/14 02:59:00 Done. Show quoted text On 2017/07/06 19:43:59, majidvp wrote: > The function names test_x, test_y, test_scroll do no match. > The first two are test cases for when scroll is large enough > to cause the boundary action to get triggered while last one > does not do so. > > Perhaps: > test_boundary_scroll_x > test_boundary_scroll_y > test_none_boundary_scroll Done.
	105 var test = async_test('scroll-boundary-behavior should not affect scrolling in side the applied container with: ' + TYPES[source_type]);

	106 function checkChangeAndFinish() {

	107 assert_equals(container.scrollTop, 0);

	108 assert_equals(container.scrollLeft, 0);

	109 if (source_type == WHEEL_SOURCE_TYPE && !IS_MAC) {

	110 test_scroll(TOUCH_SOURCE_TYPE);

	111 test.done();

	112 } else {

	113 test.done();

	114 }

	115 }

	116

	117 test.step(function() {

	118 container.style.scrollBoundaryBehaviorX = 'none';

	119 container.style.scrollBoundaryBehaviorY = 'none';

	120 window.scrollTo(0, 0);

	121 container.scrollTo(100, 100);

	122 assert_equals(container.scrollTop, 100);

	123 assert_equals(container.scrollLeft, 100);

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

	125 });

	126 }

	127

	128 </script>

OLD	NEW