Don't update the logical width when a scrollbar gets added

Don't update the logical width when a scrollbar gets added

This behavior is incorrect because the content width of a CSS box
*includes* the scrollbar width and should therefore not change.

See the layout test change in this patch -- Because min-width: 100px
includes the scrollbar, the clientWidth should be less than that.

R=szager@chromium.org,skobes@chromium.org
BUG=556717, 512229, 542197
Committed: https://crrev.com/80a9f8200b090daac5d849bfe6e304a1eaa8f6cc
Cr-Commit-Position: refs/heads/master@{#360529}

[cbiesinger, 2015-11-18 22:15:28]

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

[commit-bot: I haz the power, 2015-11-18 22:16:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1459743002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1459743002/1

[cbiesinger, 2015-11-18 22:16:27]

Stefan - interestingly, *with* this patch vbox.clientWidth is closer to
vbox.scrollWidth than without. Either way, I do think this patch is the correct
behavior.

[cbiesinger, 2015-11-18 22:26:03]

Description was changed from

==========
Don't update the logical width when a scrollbar gets added

This behavior is incorrect because the content width of a CSS box
*includes* the scrollbar width and should therefore not change.

See the layout test change in this patch -- Because min-width: 100px
includes the scrollbar, the clientWidth should be less than that.

R=szager@chromium.org,skobes@chromium.org
BUG=556717,512229
==========

to

==========
Don't update the logical width when a scrollbar gets added

This behavior is incorrect because the content width of a CSS box
*includes* the scrollbar width and should therefore not change.

See the layout test change in this patch -- Because min-width: 100px
includes the scrollbar, the clientWidth should be less than that.

R=szager@chromium.org,skobes@chromium.org
BUG=556717,512229,542197
==========

[szager1, 2015-11-18 22:35:22]

¯\_(ツ)_/¯

lgtm

[cbiesinger, 2015-11-18 22:38:43]

The CQ bit was unchecked by cbiesinger@chromium.org

[cbiesinger, 2015-11-18 22:38:47]

The CQ bit was checked by cbiesinger@chromium.org

[commit-bot: I haz the power, 2015-11-18 22:40:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1459743002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1459743002/1

[skobes, 2015-11-18 22:45:24]

https://codereview.chromium.org/1459743002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/1459743002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:897:
setLogicalHeight(logicalHeightExcludingScrollbar + scrollbarLogicalHeight());
Is the setLogicalHeight call wrong for the same reason?

[cbiesinger, 2015-11-18 22:52:51]

https://codereview.chromium.org/1459743002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/1459743002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:897:
setLogicalHeight(logicalHeightExcludingScrollbar + scrollbarLogicalHeight());
So, that is a very good question and I was wondering about that. The short
answer is that I don't know. Since height is the *output* of layout it may be
OK.

No layout test fails if I do remove it...

So I think I'll remove it in a new patch (I want to land this one as soon as
possible due to the 4pm today deadline in bug 542197)

[cbiesinger, 2015-11-18 23:00:24]

Actually I decided to just reuse this one for the height. New patch uploaded.

[cbiesinger, 2015-11-18 23:00:28]

The CQ bit was checked by cbiesinger@chromium.org

[cbiesinger, 2015-11-18 23:00:28]

The patchset sent to the CQ was uploaded after l-g-t-m from szager@chromium.org
Link to the patchset: https://codereview.chromium.org/1459743002/#ps20001
(title: "also remove height call")

[skobes, 2015-11-18 23:01:55]

lgtm

https://codereview.chromium.org/1459743002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/1459743002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:897:
setLogicalHeight(logicalHeightExcludingScrollbar + scrollbarLogicalHeight());
On 2015/11/18 22:52:51, cbiesinger wrote:
> So, that is a very good question and I was wondering about that. The short
> answer is that I don't know. Since height is the *output* of layout it may be
> OK.

Width and height are both outputs of layout, and in general, neither should
change as scrollbars are added or removed.  Though I am not very familiar with
flexboxes and it's possible there are strange cases there.

[commit-bot: I haz the power, 2015-11-18 23:04:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1459743002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1459743002/20001

[commit-bot: I haz the power, 2015-11-19 01:18:17]

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

[commit-bot: I haz the power, 2015-11-19 01:18:17]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[cbiesinger1, 2015-11-19 04:16:46]

The CQ bit was checked by cbiesinger@google.com

[commit-bot: I haz the power, 2015-11-19 04:17:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1459743002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1459743002/20001

[commit-bot: I haz the power, 2015-11-19 05:31:44]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-11-19 05:33:04]

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

[cbiesinger, 2016-02-12 20:50:46]

For future reference, this change basically updated/partially reverted
https://codereview.chromium.org/1295933003