Fix console viewport test failures with scroll anchoring.

Fix console viewport test failures with scroll anchoring.

Update defaultConsoleRowHeight to account for the 1px border, and grow a visible
item instead of an offscreen one when testing setStickToBottom.

See bug for detailed analysis.

BUG=624534
Committed: https://crrev.com/9f69ce15656df3f06add00f591f68e36348027f6
Cr-Commit-Position: refs/heads/master@{#418681}

[skobes, 2016-09-09 22:49:24]

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

[commit-bot: I haz the power, 2016-09-09 22:49:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skobes, 2016-09-09 22:50:58]

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

[dgozman, 2016-09-09 22:57:44]

dgozman@chromium.org changed reviewers:
+ luoe@chromium.org, lushnikov@chromium.org

[dgozman, 2016-09-09 22:57:44]

Over to lushnikov/luoe.

[skobes, 2016-09-09 23:00:23]

Description was changed from

==========
Fix console viewport test failures with scroll anchoring.

Update defaultConsoleRowHeight to account for padding in #console-prompt, and
grow a visible item instead of an offscreen one when testing setStickToBottom.

See bug for detailed analysis.

BUG=624534
==========

to

==========
Fix console viewport test failures with scroll anchoring.

Update defaultConsoleRowHeight to account for padding in .console-message, and
grow a visible item instead of an offscreen one when testing setStickToBottom.

See bug for detailed analysis.

BUG=624534
==========

[commit-bot: I haz the power, 2016-09-10 01:54:12]

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

[commit-bot: I haz the power, 2016-09-10 01:54:13]

Dry run: This issue passed the CQ dry run.

[luoe, 2016-09-12 17:56:59]

On 2016/09/09 22:57:44, dgozman wrote:
> Over to lushnikov/luoe.

The reasoning in the bug, comment #13 looks sound to me.  That extra 1px likely
comes from the border-bottom on console-message-wrapper.

Since the 19px value is no longer synced with consoleView.css, I would update
the comment to reflect this change.  Otherwise, LGTM!

[skobes, 2016-09-12 18:17:16]

Description was changed from

==========
Fix console viewport test failures with scroll anchoring.

Update defaultConsoleRowHeight to account for padding in .console-message, and
grow a visible item instead of an offscreen one when testing setStickToBottom.

See bug for detailed analysis.

BUG=624534
==========

to

==========
Fix console viewport test failures with scroll anchoring.

Update defaultConsoleRowHeight to account for the 1px border, and grow a visible
item instead of an offscreen one when testing setStickToBottom.

See bug for detailed analysis.

BUG=624534
==========

[skobes, 2016-09-12 18:18:25]

On 2016/09/12 17:56:59, luoe wrote:
> The reasoning in the bug, comment #13 looks sound to me.  That extra 1px
likely
> comes from the border-bottom on console-message-wrapper.
> 
> Since the 19px value is no longer synced with consoleView.css, I would update
> the comment to reflect this change.  Otherwise, LGTM!

Updated comment.

[lushnikov, 2016-09-12 20:18:39]

https://codereview.chromium.org/2317343006/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/console/console-viewport-stick-to-bottom.html
(right):

https://codereview.chromium.org/2317343006/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/console/console-viewport-stick-to-bottom.html:91:
viewport._contentElement.lastChild.innerText = "More than 2 lines: foo\n\nbar";
how does scroll anchoring picks a node to anchor?
Consider the following scenario:

1. log window object in console
2. log multiple messages after window object: for (var i = 0; i < 100; ++i)
console.log(i);
3. expand window object

Expected: scroll didn't change

[skobes, 2016-09-12 20:22:37]

https://codereview.chromium.org/2317343006/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/console/console-viewport-stick-to-bottom.html
(right):

https://codereview.chromium.org/2317343006/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/console/console-viewport-stick-to-bottom.html:91:
viewport._contentElement.lastChild.innerText = "More than 2 lines: foo\n\nbar";
On 2016/09/12 20:18:39, lushnikov wrote:
> how does scroll anchoring picks a node to anchor?
> Consider the following scenario:
> 
> 1. log window object in console
> 2. log multiple messages after window object: for (var i = 0; i < 100; ++i)
> console.log(i);
> 3. expand window object
> 
> Expected: scroll didn't change

Roughly, it will pick the first visible element.  If the window object is inside
the viewport when it is expanded, the scroll position won't change.

[skobes, 2016-09-12 22:29:52]

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

[commit-bot: I haz the power, 2016-09-12 22:30:39]

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, 2016-09-12 23:51:43]

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

[commit-bot: I haz the power, 2016-09-12 23:51:44]

Dry run: This issue passed the CQ dry run.

[lushnikov, 2016-09-13 00:09:21]

https://codereview.chromium.org/2317343006/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/console/console-viewport-stick-to-bottom.html
(right):

https://codereview.chromium.org/2317343006/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/console/console-viewport-stick-to-bottom.html:91:
viewport._contentElement.lastChild.innerText = "More than 2 lines: foo\n\nbar";
I still don't understand how the change helps to make the test pass. Does scroll
anchoring pick the first non-modified node?

[luoe, 2016-09-13 01:30:38]

https://codereview.chromium.org/2317343006/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/console/console-viewport-stick-to-bottom.html
(right):

https://codereview.chromium.org/2317343006/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/console/console-viewport-stick-to-bottom.html:91:
viewport._contentElement.lastChild.innerText = "More than 2 lines: foo\n\nbar";
skobes@, on second thought, I'm a bit uncertain, too.

When the first visible message is the anchor and an unseen message above it
grows, the content moves down and anchoring *would jump down with it.  However,
SANACLAP should suppress the snap because the content height changes, so it
won't jump.  Then, we are no longer at the bottom and the test should pass.

Is this the case?

[skobes, 2016-09-13 20:25:04]

https://codereview.chromium.org/2317343006/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/console/console-viewport-stick-to-bottom.html
(right):

https://codereview.chromium.org/2317343006/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/console/console-viewport-stick-to-bottom.html:91:
viewport._contentElement.lastChild.innerText = "More than 2 lines: foo\n\nbar";
On 2016/09/13 01:30:38, luoe wrote:
> When the first visible message is the anchor and an unseen message above it
> grows, the content moves down and anchoring *would jump down with it. 
However,
> SANACLAP should suppress the snap because the content height changes, so it
> won't jump.  Then, we are no longer at the bottom and the test should pass.

SANACLAP doesn't trigger because no CSS properties have changed, except on the
gap top and bottom gap elements which are not ancestors of the anchor node.

https://codereview.chromium.org/2317343006/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/console/console-viewport-stick-to-bottom.html:91:
viewport._contentElement.lastChild.innerText = "More than 2 lines: foo\n\nbar";
On 2016/09/13 00:09:21, lushnikov wrote:
> I still don't understand how the change helps to make the test pass. Does
scroll
> anchoring pick the first non-modified node?

What I observe is that when we set innerText for the second time (on line 97),
the content element has 13 children (log messages) but the first seven of these
are above the viewport.  So, we anchor inside
viewport._contentElement.children[7], which moves down if a preceding message is
modified.

I'm not sure exactly why the first seven messages are above the viewport at this
point.

[lushnikov, 2016-09-14 19:20:05]

got it, lgtm

[skobes, 2016-09-14 19:21:27]

The CQ bit was checked by skobes@chromium.org

[skobes, 2016-09-14 19:21:28]

The patchset sent to the CQ was uploaded after l-g-t-m from luoe@chromium.org
Link to the patchset: https://codereview.chromium.org/2317343006/#ps20001
(title: "Add comment.")

[commit-bot: I haz the power, 2016-09-14 19:22: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-09-14 21:30:52]

Description was changed from

==========
Fix console viewport test failures with scroll anchoring.

Update defaultConsoleRowHeight to account for the 1px border, and grow a visible
item instead of an offscreen one when testing setStickToBottom.

See bug for detailed analysis.

BUG=624534
==========

to

==========
Fix console viewport test failures with scroll anchoring.

Update defaultConsoleRowHeight to account for the 1px border, and grow a visible
item instead of an offscreen one when testing setStickToBottom.

See bug for detailed analysis.

BUG=624534
==========

[commit-bot: I haz the power, 2016-09-14 21:30:54]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-14 21:32:05]

Description was changed from

==========
Fix console viewport test failures with scroll anchoring.

Update defaultConsoleRowHeight to account for the 1px border, and grow a visible
item instead of an offscreen one when testing setStickToBottom.

See bug for detailed analysis.

BUG=624534
==========

to

==========
Fix console viewport test failures with scroll anchoring.

Update defaultConsoleRowHeight to account for the 1px border, and grow a visible
item instead of an offscreen one when testing setStickToBottom.

See bug for detailed analysis.

BUG=624534

Committed: https://crrev.com/9f69ce15656df3f06add00f591f68e36348027f6
Cr-Commit-Position: refs/heads/master@{#418681}
==========

[commit-bot: I haz the power, 2016-09-14 21:32:07]

Patchset 2 (id:??) landed as
https://crrev.com/9f69ce15656df3f06add00f591f68e36348027f6
Cr-Commit-Position: refs/heads/master@{#418681}