Update CSSOM SmoothScroll Web Platform Test

Update CSSOM SmoothScroll Web Platform Test

This patch improves the coverage of smooth scroll web platform test by
including tests for shadowDom and tests of empty or undefined arguments.
It reduces the frames needed for the test by checking the change in scroll
position. The test is changed to be more manual-test-friendly as users can
see the smooth scroll animation by opening the test in browser.
It also changes the interface of Element.scrollIntoView() to match the spec.

BUG=648446
Review-Url: https://codereview.chromium.org/2936283003
Cr-Commit-Position: refs/heads/master@{#479903}
Committed: https://chromium.googlesource.com/chromium/src/+/3cab89b759fb285bb1b0a5f7842e9e1f0770a910

[sunyunjia, 2017-06-14 23:45:07]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 23:45:47]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sunyunjia, 2017-06-14 23:57:23]

Description was changed from

==========
Update smoothscroll webplatform test

BUG=648446
==========

to

==========
Update CSSOM SmoothScroll Web Platform Test

This patch improves the coverage of smooth scroll web platform test by
including tests for shadowDom and tests of empty or undefined arguments.
It reduces the frames needed for the test by checking the change in scroll
position. The test is changed to be more manual-test-friendly as users can
see the smooth scroll animation by opening the test in browser.
It also changes the interface of Element.scrollIntoView() to match the spec.

BUG=648446
==========

[sunyunjia, 2017-06-14 23:57:23]

sunyunjia@chromium.org changed reviewers:
+ foolip@chromium.org

[sunyunjia, 2017-06-15 00:24:21]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-15 00:24:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sunyunjia, 2017-06-15 00:27:07]

Hi Philip,

I've updated the test according to the feedback in
https://codereview.chromium.org/2650343008/  Please take a look and let me know
if anything needs to be improved. Thanks!

[commit-bot: I haz the power, 2017-06-15 02:18:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-15 02:18:37]

Dry run: This issue passed the CQ dry run.

[foolip, 2017-06-15 08:38:04]

https://codereview.chromium.org/2936283003/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/external/wpt/cssom-view/scrollIntoView.html
(right):

https://codereview.chromium.org/2936283003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/cssom-view/scrollIntoView.html:25:
function animate (test, next_test) {
Optional nits/suggestions for how you could have less global state and manual
serialization of tests here. Unlike async_test, promise_test will run one after
the other by default. By also making animate return a promise, I think you could
simplify the first test into something like this:

function waitForScrollEnd() {
  return new Promise((resolve, reject) => {
    // A rAF loop that waits for 20 frames without change and rejects
    // if it doesn't stop, or maybe just let the test time out.
    resolve();
  });
}

promise_test(t => {
  // assert preconditions or maybe scroll to required start position
  content.scrollIntoView({behavior: 'smooth', block: 'nearest', inlinePosition:
'nearest'});
  return waitForScrollEnd().then(() => {
    assert_approx_equals(window.scrollX, content.offsetLeft, 1);
    assert_approx_equals(window.scrollX, content.offsetTop, 1);
  });
}, "Smooth scrollIntoView should scroll the element to the 'nearest' position");

The other tests would be similar. If you try it and you don't like the look of
it, feel free to ignore.

https://codereview.chromium.org/2936283003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/cssom-view/scrollIntoView.html:26:
if (frames < 500 && frames - last_changed_frame < 20) {
Can you add a comment about what this means, that we rAF either for 500 frames
or until 20 frames with no change have been observed?

https://codereview.chromium.org/2936283003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/cssom-view/scrollIntoView.html:154:
window.scrollTo(0, 0);
Is this because the page can scroll to the old position when reloading the page
manually? Can you add a comment?

Since the tests are run one after the other, can you also assert the
preconditions for each test? At the very least, I think they assume that they're
not already at the target scroll position?

https://codereview.chromium.org/2936283003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Element.idl (right):

https://codereview.chromium.org/2936283003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.idl:99: void
scrollIntoView((ScrollIntoViewOptions or boolean)? arg);
This argument should not be nullable.

[sunyunjia, 2017-06-15 17:36:25]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-15 17:36:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sunyunjia, 2017-06-15 18:00:26]

PTAL, thanks!

https://codereview.chromium.org/2936283003/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/external/wpt/cssom-view/scrollIntoView.html
(right):

https://codereview.chromium.org/2936283003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/cssom-view/scrollIntoView.html:25:
function animate (test, next_test) {
On 2017/06/15 08:38:04, foolip wrote:
> Optional nits/suggestions for how you could have less global state and manual
> serialization of tests here. Unlike async_test, promise_test will run one
after
> the other by default. By also making animate return a promise, I think you
could
> simplify the first test into something like this:
> 
> function waitForScrollEnd() {
>   return new Promise((resolve, reject) => {
>     // A rAF loop that waits for 20 frames without change and rejects
>     // if it doesn't stop, or maybe just let the test time out.
>     resolve();
>   });
> }
> 
> promise_test(t => {
>   // assert preconditions or maybe scroll to required start position
>   content.scrollIntoView({behavior: 'smooth', block: 'nearest',
inlinePosition:
> 'nearest'});
>   return waitForScrollEnd().then(() => {
>     assert_approx_equals(window.scrollX, content.offsetLeft, 1);
>     assert_approx_equals(window.scrollX, content.offsetTop, 1);
>   });
> }, "Smooth scrollIntoView should scroll the element to the 'nearest'
position");
> 
> The other tests would be similar. If you try it and you don't like the look of
> it, feel free to ignore.

Thanks! Finally learned how to use promise today!

https://codereview.chromium.org/2936283003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/cssom-view/scrollIntoView.html:26:
if (frames < 500 && frames - last_changed_frame < 20) {
On 2017/06/15 08:38:04, foolip wrote:
> Can you add a comment about what this means, that we rAF either for 500 frames
> or until 20 frames with no change have been observed?

Done.

https://codereview.chromium.org/2936283003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/cssom-view/scrollIntoView.html:154:
window.scrollTo(0, 0);
On 2017/06/15 08:38:03, foolip wrote:
> Is this because the page can scroll to the old position when reloading the
page
> manually? Can you add a comment?
> 
> Since the tests are run one after the other, can you also assert the
> preconditions for each test? At the very least, I think they assume that
they're
> not already at the target scroll position?

Done.

https://codereview.chromium.org/2936283003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Element.idl (right):

https://codereview.chromium.org/2936283003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.idl:99: void
scrollIntoView((ScrollIntoViewOptions or boolean)? arg);
On 2017/06/15 08:38:04, foolip wrote:
> This argument should not be nullable.

If I remove the "?", and pass in null or undefined, it would be parsed as an
empty ScrollIntoViewOptions, which has the default value of "center, center".
According to the spec however, if the argument is not specified, the destination
should be "start, nearest". Simon(the author) just submitted another
scrollIntoView
test(https://github.com/w3c/web-platform-tests/commit/1ca1e676dcd995ca693bebfb7c095a977830d89c)
today, and expects the destination to be "start, nearest" for "undefined"
testcase.

[foolip, 2017-06-15 19:41:53]

test lgtm % nits, but nullability of the arguments seems like it should be
fixable still.

https://codereview.chromium.org/2936283003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Element.idl (right):

https://codereview.chromium.org/2936283003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.idl:99: void
scrollIntoView((ScrollIntoViewOptions or boolean)? arg);
On 2017/06/15 18:00:26, sunyunjia wrote:
> On 2017/06/15 08:38:04, foolip wrote:
> > This argument should not be nullable.
> 
> If I remove the "?", and pass in null or undefined, it would be parsed as an
> empty ScrollIntoViewOptions, which has the default value of "center, center".
> According to the spec however, if the argument is not specified, the
destination
> should be "start, nearest". Simon(the author) just submitted another
> scrollIntoView
>
test(https://github.com/w3c/web-platform-tests/commit/1ca1e676dcd995ca693bebfb7c095a977830d89c)
> today, and expects the destination to be "start, nearest" for "undefined"
> testcase.

The "If arg is not specified" case in the spec would be when the method overload
with no arguments is selected. If you make the C++ "void scrollIntoView(bool
align_to_top = true)" have no argument, then you should be able to handle the
"no argument" case internally while keeping the IDL matching the spec. The
easiest way is probably to have the no-argument variant call into the 1-argument
one with a ScrollIntoViewOptionsOrBoolean instance that holds true.

https://codereview.chromium.org/2936283003/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/cssom-view/scrollIntoView-smooth.html
(right):

https://codereview.chromium.org/2936283003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/cssom-view/scrollIntoView-smooth.html:29:
waitForScrollEnd(frames + 1, last_changed_frame, window.scrollX,
window.scrollY).then(resolve);
optional nit: This will build up a very long promise chain. Clearly it'll work,
but since waitForScrollEnd is always called with the same arguments, you could
make the arguments variables instead. An inner function tick() or some such
would be what increments frames and either calls resolve() or
requestAnimationFrame(tick).

https://codereview.chromium.org/2936283003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/cssom-view/scrollIntoView-smooth.html:94:
function instantScrollToTestArgs(arg, expected_x, expected_y) {
This and the following test you might want to split into separate tests or fold
into the new test in
https://github.com/w3c/web-platform-tests/commit/1ca1e676dcd995ca693bebfb7c09...
given that the name of this file is scrollIntoView-smooth.html.

https://codereview.chromium.org/2936283003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/cssom-view/scrollIntoView-smooth.html:144:
}, "scrollIntoView should behave correctly if appliex to shadow dom elements");
typo: appliex

[commit-bot: I haz the power, 2017-06-15 20:05:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-15 20:05:26]

Dry run: This issue passed the CQ dry run.

[sunyunjia, 2017-06-15 21:43:48]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-15 21:44:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-16 00:11:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-16 00:11:21]

Dry run: This issue passed the CQ dry run.

[sunyunjia, 2017-06-16 01:28:34]

The CQ bit was checked by sunyunjia@chromium.org

[sunyunjia, 2017-06-16 01:28:35]

The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2936283003/#ps60001
(title: "Update the tests and idl")

[commit-bot: I haz the power, 2017-06-16 01:29:06]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-16 01:51:34]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1497576514955240,
"parent_rev": "d23ec6a9bd043274e7bf28ad00380fce930b31f2", "commit_rev":
"3cab89b759fb285bb1b0a5f7842e9e1f0770a910"}

[commit-bot: I haz the power, 2017-06-16 01:51:51]

Description was changed from

==========
Update CSSOM SmoothScroll Web Platform Test

This patch improves the coverage of smooth scroll web platform test by
including tests for shadowDom and tests of empty or undefined arguments.
It reduces the frames needed for the test by checking the change in scroll
position. The test is changed to be more manual-test-friendly as users can
see the smooth scroll animation by opening the test in browser.
It also changes the interface of Element.scrollIntoView() to match the spec.

BUG=648446
==========

to

==========
Update CSSOM SmoothScroll Web Platform Test

This patch improves the coverage of smooth scroll web platform test by
including tests for shadowDom and tests of empty or undefined arguments.
It reduces the frames needed for the test by checking the change in scroll
position. The test is changed to be more manual-test-friendly as users can
see the smooth scroll animation by opening the test in browser.
It also changes the interface of Element.scrollIntoView() to match the spec.

BUG=648446

Review-Url: https://codereview.chromium.org/2936283003
Cr-Commit-Position: refs/heads/master@{#479903}
Committed:
https://chromium.googlesource.com/chromium/src/+/3cab89b759fb285bb1b0a5f7842e...
==========

[commit-bot: I haz the power, 2017-06-16 01:51:52]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/3cab89b759fb285bb1b0a5f7842e...

[foolip, 2017-06-16 08:20:33]

https://codereview.chromium.org/2936283003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2936283003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:470: } else if (arg.isNull()) {
This can be removed again because the argument can never be null.

https://codereview.chromium.org/2936283003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Element.idl (right):

https://codereview.chromium.org/2936283003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.idl:98: void scrollIntoView(optional
(ScrollIntoViewOptions or boolean) arg);
Per https://github.com/w3c/csswg-drafts/pull/1505 it should also have the
default value true. Then you should be able to have just one signature in the
C++ as well, the generated bindings should always call with a
ScrollIntoViewOptions argument.