Fix layout tests that would fail if ScrollAnchoring was promoted to experimental

Fix layout tests that would fail if ScrollAnchoring was promoted to experimental

A lot of these tests were failing because the test expectations such as 'shouldBe' and
'shouldBeGreaterThan', etc were actively modifying the DOM. This resulted in scroll
anchoring making adjustments, and consequently causing the succeeding expectations
to fail. The simple fix here is to add "setPrintTestResultsLazily" to the tests so that the
logging from the expectations is queued up and outputted at the end of the tests. This
CL also migrates some js-tests to use testharness.js where its easy to do so.

Some of the fixes are non-trivial and the inline comments explain the reasoning behind
the fixes.

Can't promote ScrollAnchoring to experiment until the following bug is fixed:
crbug.com/624534, which causes inspector/console/console-viewport-selection.html
to fail.

BUG=597479
Committed: https://crrev.com/4577baf99363df344a6e1180cce4ac0436f5ce04
Cr-Commit-Position: refs/heads/master@{#407941}

[ymalik, 2016-07-22 16:52:17]

Description was changed from

==========
Promote ScrollAnchoring to experimental

BUG=597479
==========

to

==========
Fix layout tests that would fail if ScrollAnchoring was promoted to experimental

A lot of these tests were failing because the test expectations such as
'shouldBe' and 
'shouldBeGreaterThan', etc were actively modifying the DOM. This resulted in
scroll 
anchoring making adjustments, and consequently causing the succeeding
expectations 
to fail. The simple fix here is to add "setPrintTestResultsLazily" to the tests
so that the 
logging from the expectations is queued up and outputted at the end of the
tests. This 
CL also migrates some js-tests to use testharness.js where its easy to do so.

Some of the fixes are non-trivial and the inline comments explain the reasoning
behind 
the fixes.

Can't promote ScrollAnchoring to experiment until the following bug is fixed:
crbug.com/624534, which causes inspector/console/console-viewport-selection.html
to fail.

BUG=597479
==========

[ymalik, 2016-07-22 16:52:17]

ymalik@chromium.org changed reviewers:
+ skobes@chromium.org

[ymalik, 2016-07-22 16:53:24]

@skobes, Please see the description and the inline comments for an explanation
for the fixes.

https://codereview.chromium.org/2165763002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/editing/execCommand/insert-line-break-no-scroll.html
(right):

https://codereview.chromium.org/2165763002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/editing/execCommand/insert-line-break-no-scroll.html:11:
var result = (window.pageYOffset >= 1000) ? "SUCCESS" : "FAILURE";
@skobes, the purpose of this test is to verify that we don't scroll to make the
selection visible. Because we're inserting a line break, scroll anchoring makes
an adjustment so that the viewport doesn't move. In this particular case, this
is probably a better user experience. Hence, I think loosening the '==' to '>='
is probably fine.

https://codereview.chromium.org/2165763002/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/css/sticky/sticky-container-moved.html
(right):

https://codereview.chromium.org/2165763002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-container-moved.html:13:
overflow-anchor: none;
@skobes, I couldn't come up with a way to modify this test to work with scroll
anchoring. I think the right fix here is to disable scroll anchoring. This test
is testing that a stickly positioned element actually sticks when the position
of its containing block changes. If opting out of scroll anchoring enables us to
test this, its probably fine? If a webpage ever does this in practice, scroll
anchoring so that the user doesn't notice a visible change is probably the right
thing to do? WDYT?

https://codereview.chromium.org/2165763002/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/css/zoom-body-scroll.html (right):

https://codereview.chromium.org/2165763002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/zoom-body-scroll.html:12:
setPrintTestResultsLazily();
@skobes, Browser zoom causes layout, which causes scroll anchoring adjustments.
Though this test is testing a specific thing and the incorrect adjustment is
caused by the test expectation code causing a layout. Filed
https://bugs.chromium.org/p/chromium/issues/detail?id=630469 for the actual
issue.

https://codereview.chromium.org/2165763002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/dom/horizontal-scrollbar-when-dir-change.html
(left):

https://codereview.chromium.org/2165763002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/horizontal-scrollbar-when-dir-change.html:35:
document.body.dir = "ltr";
@skobes, Changing rtl to ltr causes a layout and we anchor and adjust, which
causes the second half of the expectations to fail. This is probably not a
common practice anyway, and what's is testing is covered by
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/dom/...

https://codereview.chromium.org/2165763002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/dom/vertical-scrollbar-when-dir-change.html
(left):

https://codereview.chromium.org/2165763002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/vertical-scrollbar-when-dir-change.html:29:
document.body.dir = "ltr";
@skobes, removing as per the same reasoning as the horizontal case.

[skobes, 2016-07-25 20:46:39]

https://codereview.chromium.org/2165763002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/editing/execCommand/insert-line-break-no-scroll.html
(right):

https://codereview.chromium.org/2165763002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/editing/execCommand/insert-line-break-no-scroll.html:11:
var result = (window.pageYOffset >= 1000) ? "SUCCESS" : "FAILURE";
On 2016/07/22 16:53:24, ymalik wrote:
> @skobes, the purpose of this test is to verify that we don't scroll to make
the
> selection visible. Because we're inserting a line break, scroll anchoring
makes
> an adjustment so that the viewport doesn't move. In this particular case, this
> is probably a better user experience. Hence, I think loosening the '==' to
'>='
> is probably fine.

Acknowledged.

https://codereview.chromium.org/2165763002/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/css/sticky/sticky-container-moved.html
(right):

https://codereview.chromium.org/2165763002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-container-moved.html:13:
overflow-anchor: none;
On 2016/07/22 16:53:24, ymalik wrote:
> @skobes, I couldn't come up with a way to modify this test to work with scroll
> anchoring. I think the right fix here is to disable scroll anchoring. This
test
> is testing that a stickly positioned element actually sticks when the position
> of its containing block changes. If opting out of scroll anchoring enables us
to
> test this, its probably fine? If a webpage ever does this in practice, scroll
> anchoring so that the user doesn't notice a visible change is probably the
right
> thing to do? WDYT?

How about calling scrollTo after changing height, instead of before?

https://codereview.chromium.org/2165763002/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/css/zoom-body-scroll.html (right):

https://codereview.chromium.org/2165763002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/zoom-body-scroll.html:12:
setPrintTestResultsLazily();
On 2016/07/22 16:53:24, ymalik wrote:
> @skobes, Browser zoom causes layout, which causes scroll anchoring
adjustments.
> Though this test is testing a specific thing and the incorrect adjustment is
> caused by the test expectation code causing a layout. Filed
> https://bugs.chromium.org/p/chromium/issues/detail?id=630469 for the actual
> issue.

Acknowledged.

https://codereview.chromium.org/2165763002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/dom/horizontal-scrollbar-when-dir-change.html
(left):

https://codereview.chromium.org/2165763002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/horizontal-scrollbar-when-dir-change.html:35:
document.body.dir = "ltr";
On 2016/07/22 16:53:23, ymalik wrote:
> @skobes, Changing rtl to ltr causes a layout and we anchor and adjust, which
> causes the second half of the expectations to fail. This is probably not a
> common practice anyway, and what's is testing is covered by
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/dom/...

This test is intended to check the behavior when document.body.dir changes
dynamically, which horizontal-scrollbar-in-rtl.html doesn't cover.  It looks
like both of them were added in http://crrev.com/ef39f97 which introduced the
"scroll origin" concept to FrameView.

Maybe just check for scrollX >= 0 on the next line, instead of == 0?

https://codereview.chromium.org/2165763002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/dom/vertical-scrollbar-when-dir-change.html
(left):

https://codereview.chromium.org/2165763002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/vertical-scrollbar-when-dir-change.html:29:
document.body.dir = "ltr";
On 2016/07/22 16:53:23, ymalik wrote:
> @skobes, removing as per the same reasoning as the horizontal case.

Same suggestion here.

[ymalik, 2016-07-26 21:03:50]

@skobes PTAL :)

https://codereview.chromium.org/2165763002/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/css/sticky/sticky-container-moved.html
(right):

https://codereview.chromium.org/2165763002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-container-moved.html:13:
overflow-anchor: none;
On 2016/07/25 20:46:38, skobes wrote:
> On 2016/07/22 16:53:24, ymalik wrote:
> > @skobes, I couldn't come up with a way to modify this test to work with
scroll
> > anchoring. I think the right fix here is to disable scroll anchoring. This
> test
> > is testing that a stickly positioned element actually sticks when the
position
> > of its containing block changes. If opting out of scroll anchoring enables
us
> to
> > test this, its probably fine? If a webpage ever does this in practice,
scroll
> > anchoring so that the user doesn't notice a visible change is probably the
> right
> > thing to do? WDYT?
> 
> How about calling scrollTo after changing height, instead of before?

I think the purpose of the test is to verify that a change in the containing
block causes the sticky positioned element to stick. If we scroll after, we
can't tell whether the stick positioned element stuck because of the scroll or
because of the change in the containing block.

https://codereview.chromium.org/2165763002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/dom/horizontal-scrollbar-when-dir-change.html
(left):

https://codereview.chromium.org/2165763002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/horizontal-scrollbar-when-dir-change.html:35:
document.body.dir = "ltr";
On 2016/07/25 20:46:38, skobes wrote:
> On 2016/07/22 16:53:23, ymalik wrote:
> > @skobes, Changing rtl to ltr causes a layout and we anchor and adjust, which
> > causes the second half of the expectations to fail. This is probably not a
> > common practice anyway, and what's is testing is covered by
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/dom/...
> 
> This test is intended to check the behavior when document.body.dir changes
> dynamically, which horizontal-scrollbar-in-rtl.html doesn't cover.  It looks
> like both of them were added in http://crrev.com/ef39f97 which introduced the
> "scroll origin" concept to FrameView.
> 
> Maybe just check for scrollX >= 0 on the next line, instead of == 0?

Thanks. Done.

https://codereview.chromium.org/2165763002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/dom/vertical-scrollbar-when-dir-change.html
(left):

https://codereview.chromium.org/2165763002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/vertical-scrollbar-when-dir-change.html:29:
document.body.dir = "ltr";
On 2016/07/25 20:46:38, skobes wrote:
> On 2016/07/22 16:53:23, ymalik wrote:
> > @skobes, removing as per the same reasoning as the horizontal case.
> 
> Same suggestion here.

Done.

[skobes, 2016-07-26 21:19:14]

lgtm

https://codereview.chromium.org/2165763002/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/css/sticky/sticky-container-moved.html
(right):

https://codereview.chromium.org/2165763002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-container-moved.html:13:
overflow-anchor: none;
On 2016/07/26 21:03:50, ymalik wrote:
> On 2016/07/25 20:46:38, skobes wrote:
> > On 2016/07/22 16:53:24, ymalik wrote:
> > > @skobes, I couldn't come up with a way to modify this test to work with
> scroll
> > > anchoring. I think the right fix here is to disable scroll anchoring. This
> > test
> > > is testing that a stickly positioned element actually sticks when the
> position
> > > of its containing block changes. If opting out of scroll anchoring enables
> us
> > to
> > > test this, its probably fine? If a webpage ever does this in practice,
> scroll
> > > anchoring so that the user doesn't notice a visible change is probably the
> > right
> > > thing to do? WDYT?
> > 
> > How about calling scrollTo after changing height, instead of before?
> 
> I think the purpose of the test is to verify that a change in the containing
> block causes the sticky positioned element to stick. If we scroll after, we
> can't tell whether the stick positioned element stuck because of the scroll or
> because of the change in the containing block.

Good point.  Using the opt out here is fine.

[ymalik, 2016-07-26 21:23:35]

The CQ bit was checked by ymalik@chromium.org

[commit-bot: I haz the power, 2016-07-26 21:24:15]

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, 2016-07-26 22:37:34]

Description was changed from

==========
Fix layout tests that would fail if ScrollAnchoring was promoted to experimental

A lot of these tests were failing because the test expectations such as
'shouldBe' and 
'shouldBeGreaterThan', etc were actively modifying the DOM. This resulted in
scroll 
anchoring making adjustments, and consequently causing the succeeding
expectations 
to fail. The simple fix here is to add "setPrintTestResultsLazily" to the tests
so that the 
logging from the expectations is queued up and outputted at the end of the
tests. This 
CL also migrates some js-tests to use testharness.js where its easy to do so.

Some of the fixes are non-trivial and the inline comments explain the reasoning
behind 
the fixes.

Can't promote ScrollAnchoring to experiment until the following bug is fixed:
crbug.com/624534, which causes inspector/console/console-viewport-selection.html
to fail.

BUG=597479
==========

to

==========
Fix layout tests that would fail if ScrollAnchoring was promoted to experimental

A lot of these tests were failing because the test expectations such as
'shouldBe' and 
'shouldBeGreaterThan', etc were actively modifying the DOM. This resulted in
scroll 
anchoring making adjustments, and consequently causing the succeeding
expectations 
to fail. The simple fix here is to add "setPrintTestResultsLazily" to the tests
so that the 
logging from the expectations is queued up and outputted at the end of the
tests. This 
CL also migrates some js-tests to use testharness.js where its easy to do so.

Some of the fixes are non-trivial and the inline comments explain the reasoning
behind 
the fixes.

Can't promote ScrollAnchoring to experiment until the following bug is fixed:
crbug.com/624534, which causes inspector/console/console-viewport-selection.html
to fail.

BUG=597479
==========

[commit-bot: I haz the power, 2016-07-26 22:37:42]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-07-26 22:39:50]

Description was changed from

==========
Fix layout tests that would fail if ScrollAnchoring was promoted to experimental

A lot of these tests were failing because the test expectations such as
'shouldBe' and 
'shouldBeGreaterThan', etc were actively modifying the DOM. This resulted in
scroll 
anchoring making adjustments, and consequently causing the succeeding
expectations 
to fail. The simple fix here is to add "setPrintTestResultsLazily" to the tests
so that the 
logging from the expectations is queued up and outputted at the end of the
tests. This 
CL also migrates some js-tests to use testharness.js where its easy to do so.

Some of the fixes are non-trivial and the inline comments explain the reasoning
behind 
the fixes.

Can't promote ScrollAnchoring to experiment until the following bug is fixed:
crbug.com/624534, which causes inspector/console/console-viewport-selection.html
to fail.

BUG=597479
==========

to

==========
Fix layout tests that would fail if ScrollAnchoring was promoted to experimental

A lot of these tests were failing because the test expectations such as
'shouldBe' and
'shouldBeGreaterThan', etc were actively modifying the DOM. This resulted in
scroll
anchoring making adjustments, and consequently causing the succeeding
expectations
to fail. The simple fix here is to add "setPrintTestResultsLazily" to the tests
so that the
logging from the expectations is queued up and outputted at the end of the
tests. This
CL also migrates some js-tests to use testharness.js where its easy to do so.

Some of the fixes are non-trivial and the inline comments explain the reasoning
behind
the fixes.

Can't promote ScrollAnchoring to experiment until the following bug is fixed:
crbug.com/624534, which causes inspector/console/console-viewport-selection.html
to fail.

BUG=597479

Committed: https://crrev.com/4577baf99363df344a6e1180cce4ac0436f5ce04
Cr-Commit-Position: refs/heads/master@{#407941}
==========

[commit-bot: I haz the power, 2016-07-26 22:39:52]

Patchset 7 (id:??) landed as
https://crrev.com/4577baf99363df344a6e1180cce4ac0436f5ce04
Cr-Commit-Position: refs/heads/master@{#407941}