Include auto vertical scrollbar in intrinsicScrollbarLogicalWidth.

Include auto vertical scrollbar in intrinsicScrollbarLogicalWidth.

Prior to this change, auto vertical scrollbars could intrude into the
content box and overlap content on the right edge.  Discussion of
this change at:

https://codereview.chromium.org/1930183002/#msg12

BUG=600036
R=skobes@chromium.org,eae@chromium.org

Committed: https://crrev.com/c9b3e99e2a71f7bcc4d9652916347782ff616e0d
Cr-Commit-Position: refs/heads/master@{#397112}

[eae, 2016-05-16 18:45:15]

This is an exciting change, looks good to me but I'd like skobes to review this
given his familiarity with the code.

[skobes, 2016-05-16 20:55:53]

https://codereview.chromium.org/1980103002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/1980103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:688:
box().updateLogicalWidth();
Won't this (updateLogicalWidth) happen during layout anyway?

https://codereview.chromium.org/1980103002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1980103002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:796: if
(isHorizontalWritingMode() && (style()->overflowY() == OverflowScroll ||
style()->overflowY() == OverflowAuto)) {
I will echo cbiesinger's question on the other thread :)

Can we kill this and just use scrollbarLogicalWidth?

[skobes, 2016-05-16 20:59:40]

Also, aren't we creating a circular dependency between preferred widths and
layout?

Auto scrollbar existence depends on the effects of PLSA::updateAfterLayout, but
intrinsicScrollbarLogicalWidth seems to exist to support
computeIntrinsicLogicalWidths, which is used by
LayoutBlock::computePreferredLogicalWidths.

Preferred logical widths probably shouldn't depend on layout.

[szager1, 2016-05-16 22:30:15]

On 2016/05/16 20:59:40, skobes wrote:
> Also, aren't we creating a circular dependency between preferred widths and
> layout?
> 
> Auto scrollbar existence depends on the effects of PLSA::updateAfterLayout,
but
> intrinsicScrollbarLogicalWidth seems to exist to support
> computeIntrinsicLogicalWidths, which is used by
> LayoutBlock::computePreferredLogicalWidths.
> 
> Preferred logical widths probably shouldn't depend on layout.

Yes, this is true; however, although this change will cause some additional
layout (for a good cause), I can't think of a way this could cause an infinite
loop.  Once it is known that a block overflows vertically, I can't think of any
situation where adding the scrollbar to the preferred width would cause layout
to make the block taller so that it doesn't overflow.

The one exception I can think of is flexbox, but flexbox is handled specially,
and I have another patch that clarifies the treatment of overflow:auto by
flexbox:

https://codereview.chromium.org/1930183002/

If you look at the changes to LayoutFlexibleBox.cpp in that patch, you can see
how PaintLayerScrollableArea::FreezeScrollbarsScope is used to prevent excessive
loops through layout.  What do you think about using FreezeScrollbarsScope in
other classes that override layoutBlock, to prevent excessive looping due to
auto scrollbars?

[skobes, 2016-05-16 23:23:48]

On 2016/05/16 22:30:15, szager1 wrote:
> If you look at the changes to LayoutFlexibleBox.cpp in that patch, you can see
> how PaintLayerScrollableArea::FreezeScrollbarsScope is used to prevent
excessive
> loops through layout.  What do you think about using FreezeScrollbarsScope in
> other classes that override layoutBlock, to prevent excessive looping due to
> auto scrollbars?

That sounds fine, but I was more worried about stale data than layout loops. 
You're recomputing the scroller's preferred width in PLSA::updateAfterLayout,
are you sure layout hasn't already relied on the preferred width in some way?  I
don't know of a specific way it will break, it just feels brittle.

The description says "auto vertical scrollbars could intrude into the content
box and overlap content on the right edge"... could you clarify how that
occurred and how this fixes it?  It's not very clear from the bug.

[szager1, 2016-05-16 23:57:10]

On 2016/05/16 23:23:48, skobes wrote:
> On 2016/05/16 22:30:15, szager1 wrote:
> > If you look at the changes to LayoutFlexibleBox.cpp in that patch, you can
see
> > how PaintLayerScrollableArea::FreezeScrollbarsScope is used to prevent
> excessive
> > loops through layout.  What do you think about using FreezeScrollbarsScope
in
> > other classes that override layoutBlock, to prevent excessive looping due to
> > auto scrollbars?
> 
> That sounds fine, but I was more worried about stale data than layout loops. 
> You're recomputing the scroller's preferred width in PLSA::updateAfterLayout,
> are you sure layout hasn't already relied on the preferred width in some way? 
I
> don't know of a specific way it will break, it just feels brittle.
> 
> The description says "auto vertical scrollbars could intrude into the content
> box and overlap content on the right edge"... could you clarify how that
> occurred and how this fixes it?  It's not very clear from the bug.

Here's the example I mentioned on the other CL, the
scrollbars/scrollbars-on-positioned-content.html test:

before: http://imgur.com/gTworqU
after: http://imgur.com/R9OTH7y

I think it's the custom scrollbars that causes the problem.

Additionally, this will some cases where flexbox creates unnecessary scrollbars,
as in css3/flexbox/flexbox-height-with-overflow-auto.html:

before: http://imgur.com/uWIDjMT
after: http://imgur.com/JwtYIEs

The reason this works and doesn't leave the layout tree in a bad state is
because at the point where PLSA::updateAfterLayout marks the preferred widths as
dirty, it is already planning to do a relayout as a result of changing the
scrollbars.

[skobes, 2016-05-16 23:57:42]

On 2016/05/16 23:23:48, skobes wrote:
> The description says "auto vertical scrollbars could intrude into the content
> box and overlap content on the right edge"... could you clarify how that
> occurred and how this fixes it?  It's not very clear from the bug.

Oh, I think I understand based on your screenshots of
scrollbars/scrollbars-on-positioned-content.html.

What does your patch do with something like https://output.jsbin.com/tukove ? 
Does the scroller's total width stay constant as the contents grow and shrink,
or does it expand?  I'd argue that the right behavior here is for the total
width to not depend on the scrollbar presence (and other browsers agree).

[szager1, 2016-05-17 00:06:44]

On 2016/05/16 23:57:42, skobes wrote:
> On 2016/05/16 23:23:48, skobes wrote:
> > The description says "auto vertical scrollbars could intrude into the
content
> > box and overlap content on the right edge"... could you clarify how that
> > occurred and how this fixes it?  It's not very clear from the bug.
> 
> Oh, I think I understand based on your screenshots of
> scrollbars/scrollbars-on-positioned-content.html.
> 
> What does your patch do with something like https://output.jsbin.com/tukove ? 
> Does the scroller's total width stay constant as the contents grow and shrink,
> or does it expand?  I'd argue that the right behavior here is for the total
> width to not depend on the scrollbar presence (and other browsers agree).

You're correct, that causes the width to change, and it doesn't add a horizontal
scrollbar.  That's not what firefox does, but I would argue that the new
behavior is preferable -- the horizontal scrollbar there is (IMO) terrible.

[szager1, 2016-05-17 00:09:57]

On 2016/05/17 00:06:44, szager1 wrote:
> On 2016/05/16 23:57:42, skobes wrote:
> > On 2016/05/16 23:23:48, skobes wrote:
> > > The description says "auto vertical scrollbars could intrude into the
> content
> > > box and overlap content on the right edge"... could you clarify how that
> > > occurred and how this fixes it?  It's not very clear from the bug.
> > 
> > Oh, I think I understand based on your screenshots of
> > scrollbars/scrollbars-on-positioned-content.html.
> > 
> > What does your patch do with something like https://output.jsbin.com/tukove
? 
> > Does the scroller's total width stay constant as the contents grow and
shrink,
> > or does it expand?  I'd argue that the right behavior here is for the total
> > width to not depend on the scrollbar presence (and other browsers agree).
> 
> You're correct, that causes the width to change, and it doesn't add a
horizontal
> scrollbar.  That's not what firefox does, but I would argue that the new
> behavior is preferable -- the horizontal scrollbar there is (IMO) terrible.

And thanks to eae, I can report that Edge does it the new (better) way.

[skobes, 2016-05-17 00:14:51]

On 2016/05/17 00:09:57, szager1 wrote:
> And thanks to eae, I can report that Edge does it the new (better) way.

Ok, that makes me feel a little better. :)  I do think this is a slightly risky
web facing change though, maybe worth an email to blink-dev?

Also see my earlier comments re. merging with scrollbarLogicalWidth and calling
updateLogicalWidth during layout.

[skobes, 2016-05-17 00:18:40]

https://codereview.chromium.org/1980103002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/scrollbars/auto-scrollbar-zero-width-block.html
(right):

https://codereview.chromium.org/1980103002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/scrollbars/auto-scrollbar-zero-width-block.html:24:
if (container.clientWidth == 0)
Also, what should happen if #empty-block has width > 0 but < scrollbar width? 
If you're special casing zero it will still be possible to get negative
clientWidth values.

[szager1, 2016-05-17 07:18:05]

On 2016/05/17 00:18:40, skobes wrote:
>
https://codereview.chromium.org/1980103002/diff/20001/third_party/WebKit/Layo...
> File
> third_party/WebKit/LayoutTests/scrollbars/auto-scrollbar-zero-width-block.html
> (right):
> 
>
https://codereview.chromium.org/1980103002/diff/20001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/scrollbars/auto-scrollbar-zero-width-block.html:24:
> if (container.clientWidth == 0)
> Also, what should happen if #empty-block has width > 0 but < scrollbar width? 
> If you're special casing zero it will still be possible to get negative
> clientWidth values.

No, that shouldn't happen at all any more (I tried it).  If #empty-block has
width:1px, #floated-container will have a vertical scrollbar and clientWidth=1.

[szager1, 2016-05-17 07:18:56]

On 2016/05/17 00:14:51, skobes wrote:
> On 2016/05/17 00:09:57, szager1 wrote:
> > And thanks to eae, I can report that Edge does it the new (better) way.
> 
> Ok, that makes me feel a little better. :)  I do think this is a slightly
risky
> web facing change though, maybe worth an email to blink-dev?
> 
> Also see my earlier comments re. merging with scrollbarLogicalWidth and
calling
> updateLogicalWidth during layout.

I got rid of intrinsicScrollbarLogicalWidth.

[szager1, 2016-05-17 07:25:45]

Also, just for fun, here's another goofy example:

https://jsbin.com/vaniwuhiri/edit?html,js,output

Even on first layout, it has a horizontal scrollbar; and even when the entire
#inner content should fit without overflow, it still has two scrollbars.  By
contrast, here is what it looks like with my patch:

#inner height:100px -- http://imgur.com/Va01z5f
#inner height:200px -- http://imgur.com/5LmPKg5

[skobes, 2016-05-17 17:23:46]

ok, lgtm :)

[szager1, 2016-05-17 18:16:09]

The CQ bit was checked by szager@chromium.org

[commit-bot: I haz the power, 2016-05-17 18:16:38]

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

[szager1, 2016-05-17 18:19:25]

The CQ bit was unchecked by szager@chromium.org

[szager1, 2016-05-17 18:20:01]

Description was changed from

==========
Include auto vertical scrollbar in intrinsicScrollbarLogicalWidth.

Prior to this change, auto vertical scrollbars could intrude into the
content box and overlap content on the right edge.  Discussion of
this change at:

https://codereview.chromium.org/1930183002/#msg12

BUG=600036
R=skobes@chromium.org,eae@chromium.org
==========

to

==========
Include auto vertical scrollbar in intrinsicScrollbarLogicalWidth.

Prior to this change, auto vertical scrollbars could intrude into the
content box and overlap content on the right edge.  Discussion of
this change at:

https://codereview.chromium.org/1930183002/#msg12

BUG=600036
R=skobes@chromium.org,eae@chromium.org
==========

[szager1, 2016-05-17 18:20:01]

szager@chromium.org changed reviewers:
+ tkent@chromium.org

[szager1, 2016-05-17 18:20:17]

+tkent for Source/web/resources/OWNERS

[tkent, 2016-05-17 22:59:27]

lgtm

[szager1, 2016-05-17 23:04:55]

The CQ bit was checked by szager@chromium.org

[szager1, 2016-05-17 23:04:56]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
skobes@chromium.org
Link to the patchset: https://codereview.chromium.org/1980103002/#ps60001
(title: "Don't alter scrollbars during relayout.")

[commit-bot: I haz the power, 2016-05-17 23:05:47]

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

[szager1, 2016-05-17 23:08:19]

The CQ bit was unchecked by szager@chromium.org

[szager1, 2016-05-17 23:17:25]

The CQ bit was checked by szager@chromium.org

[szager1, 2016-05-17 23:17:26]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
skobes@chromium.org
Link to the patchset: https://codereview.chromium.org/1980103002/#ps80001
(title: "refactor use of m_inOverflowRelayout")

[commit-bot: I haz the power, 2016-05-17 23:18:01]

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

[commit-bot: I haz the power, 2016-05-17 23:33:45]

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

[commit-bot: I haz the power, 2016-05-17 23:33:46]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[szager1, 2016-05-18 00:01:00]

The CQ bit was checked by szager@chromium.org

[commit-bot: I haz the power, 2016-05-18 00:02:18]

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

[commit-bot: I haz the power, 2016-05-18 01:34:02]

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

[commit-bot: I haz the power, 2016-05-18 01:34:03]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[szager1, 2016-05-23 21:29:49]

The CQ bit was checked by szager@chromium.org

[szager1, 2016-05-23 21:29:49]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
skobes@chromium.org
Link to the patchset: https://codereview.chromium.org/1980103002/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2016-05-23 21:30:30]

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

[commit-bot: I haz the power, 2016-05-23 21:37:29]

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

[commit-bot: I haz the power, 2016-05-23 21:37:30]

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

[cbiesinger, 2016-05-23 21:46:02]

I'm late to the party, but I have a couple questions:

How do you handle the case when the parent of the o:auto depends on the child's
preferred width? I believe (?) this will not be handled correctly by your patch?
E.g. http://jsbin.com/dasikurahe/edit?html,output

[cbiesinger, 2016-05-23 21:46:11]

cbiesinger@chromium.org changed reviewers:
+ cbiesinger@chromium.org

[cbiesinger, 2016-05-23 21:46:12]

https://codereview.chromium.org/1980103002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/1980103002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:693:
box().updateLogicalWidth();
I think skobes asked this too -- why is this call necessary?

[szager1, 2016-05-23 22:06:06]

On 2016/05/23 21:46:02, cbiesinger wrote:
> I'm late to the party, but I have a couple questions:
> 
> How do you handle the case when the parent of the o:auto depends on the
child's
> preferred width? I believe (?) this will not be handled correctly by your
patch?
> E.g. http://jsbin.com/dasikurahe/edit?html,output

That works as expected, i.e., there is no horizontal scrollbar.

[szager1, 2016-05-23 22:17:32]

https://codereview.chromium.org/1980103002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/1980103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:688:
box().updateLogicalWidth();
On 2016/05/16 20:55:53, skobes wrote:
> Won't this (updateLogicalWidth) happen during layout anyway?

If delayedLayoutScope is not null, then layout won't necessarily run again right
away, but the logical width still needs to be correct for the parent.  I can
move updateLogicalWidth() into the delayedLayoutScope clause.

[cbiesinger, 2016-05-23 22:49:45]

But is the float sized wide enough to contain the scrollbar? Ie. Does the
black border fully contain the inner div?

I would prefer moving updateLogicalWidth into the delayedLayoutScope block
with a comment, yes. Thanks!
On May 23, 2016 18:06, <szager@chromium.org> wrote:

> On 2016/05/23 21:46:02, cbiesinger wrote:
> > I'm late to the party, but I have a couple questions:
> >
> > How do you handle the case when the parent of the o:auto depends on the
> child's
> > preferred width? I believe (?) this will not be handled correctly by your
> patch?
> > E.g. http://jsbin.com/dasikurahe/edit?html,output
>
> That works as expected, i.e., there is no horizontal scrollbar.
>
> https://codereview.chromium.org/1980103002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[cbiesinger, 2016-05-23 22:49:46]

But is the float sized wide enough to contain the scrollbar? Ie. Does the
black border fully contain the inner div?

I would prefer moving updateLogicalWidth into the delayedLayoutScope block
with a comment, yes. Thanks!
On May 23, 2016 18:06, <szager@chromium.org> wrote:

> On 2016/05/23 21:46:02, cbiesinger wrote:
> > I'm late to the party, but I have a couple questions:
> >
> > How do you handle the case when the parent of the o:auto depends on the
> child's
> > preferred width? I believe (?) this will not be handled correctly by your
> patch?
> > E.g. http://jsbin.com/dasikurahe/edit?html,output
>
> That works as expected, i.e., there is no horizontal scrollbar.
>
> https://codereview.chromium.org/1980103002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[szager1, 2016-05-27 18:15:28]

PTAL

This should now cover the case of a parent block that sizes to fit its content,
due to changes in the latest patch to LayoutBlockFlow.cpp.  I added a test to
cover that use case.

Also, I cleaned up the treatment of scrollbars in drop-down select menus.  Since
listPicker.js takes complete control of styling the menu box, including
scrollbar treatment, LayoutListBox should just never add scrollbars (since it
has no idea whether it will have vertical overflow).

Also added a fix to avoid creating a composited layer for a zero-area
PaintLayer.

[skobes, 2016-05-27 19:10:12]

lgtm

[cbiesinger, 2016-05-27 20:36:42]

lgtm, thanks for persevering with this patch!

[eae, 2016-05-27 20:45:17]

This is awesome, LGTM++

[szager1, 2016-06-01 10:55:23]

The CQ bit was checked by szager@chromium.org

[szager1, 2016-06-01 10:55:23]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
cbiesinger@chromium.org, skobes@chromium.org, eae@chromium.org
Link to the patchset: https://codereview.chromium.org/1980103002/#ps180001
(title: "Restore list box height calculation, fix scrollbar width factor")

[commit-bot: I haz the power, 2016-06-01 10:55:30]

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

[commit-bot: I haz the power, 2016-06-01 12:33:59]

Description was changed from

==========
Include auto vertical scrollbar in intrinsicScrollbarLogicalWidth.

Prior to this change, auto vertical scrollbars could intrude into the
content box and overlap content on the right edge.  Discussion of
this change at:

https://codereview.chromium.org/1930183002/#msg12

BUG=600036
R=skobes@chromium.org,eae@chromium.org
==========

to

==========
Include auto vertical scrollbar in intrinsicScrollbarLogicalWidth.

Prior to this change, auto vertical scrollbars could intrude into the
content box and overlap content on the right edge.  Discussion of
this change at:

https://codereview.chromium.org/1930183002/#msg12

BUG=600036
R=skobes@chromium.org,eae@chromium.org
==========

[commit-bot: I haz the power, 2016-06-01 12:34:01]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-06-01 12:35:15]

Description was changed from

==========
Include auto vertical scrollbar in intrinsicScrollbarLogicalWidth.

Prior to this change, auto vertical scrollbars could intrude into the
content box and overlap content on the right edge.  Discussion of
this change at:

https://codereview.chromium.org/1930183002/#msg12

BUG=600036
R=skobes@chromium.org,eae@chromium.org
==========

to

==========
Include auto vertical scrollbar in intrinsicScrollbarLogicalWidth.

Prior to this change, auto vertical scrollbars could intrude into the
content box and overlap content on the right edge.  Discussion of
this change at:

https://codereview.chromium.org/1930183002/#msg12

BUG=600036
R=skobes@chromium.org,eae@chromium.org

Committed: https://crrev.com/c9b3e99e2a71f7bcc4d9652916347782ff616e0d
Cr-Commit-Position: refs/heads/master@{#397112}
==========

[commit-bot: I haz the power, 2016-06-01 12:35:17]

Patchset 10 (id:??) landed as
https://crrev.com/c9b3e99e2a71f7bcc4d9652916347782ff616e0d
Cr-Commit-Position: refs/heads/master@{#397112}