Make room for any scrollbars in the content box and border box.

Make room for any scrollbars in the content box and border box.

The spec isn't clear. It does say to perform such clamping if the specified
border-box size is less than the total size of border+padding, but there's
nothing for scrollbars in particular. However, since we have code that
assumes that subtracting a scrollbar width from a border box size won't
result in negative values, this seems like a reasonable thing to do. This
aligns us with IE / Edge, while Firefox does no such stretching.

This CL makes the fix for bug 720227
(VerticalScrollbarWidthClampedToContentBox()) unnecessary, so revert it,
but keep the tests.

Had to remove the test for bug 549174, because it assumes that a scrollbar
may overflow the border box, but that's no longer going to happen ever.

The test find-hidden-text.html also needed an update, since it assumed that
forcing a scrollbar on and at the same time specifying width or height as 0
would leave the border box width at 0. This is no longer the case.

BUG=724255

[mstensho (USE GERRIT), 2017-05-23 12:39:49]

The CQ bit was checked by mstensho@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 12:40:04]

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-05-23 14:09:34]

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

[commit-bot: I haz the power, 2017-05-23 14:09:35]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mstensho (USE GERRIT), 2017-06-09 14:06:47]

Description was changed from

==========
Make room for any scrollbars in the content box and border box.

We used to have problems when moving child content past a left-hand scrollbar,
that we jumped way outside the content box. We ended up with negative values in
parts of the code where they are not expected.

This will stretch the box size when a box would otherwise be smaller than the
width of the scrollbar. This change aligns us with IE / Edge, while Firefox
still does no such stretching.

BUG=720227,724255
==========

to

==========
Make room for any scrollbars in the content box and border box.

The spec isn't clear. It does say to perform such clamping if the specified
border-box size is less than the total size of border+padding, but there's
nothing for scrollbars in particular. However, since we have code that
assumes that subtracting a scrollbar width from a border box size won't
result in negative values, this seems like a reasonable thing to do. This
aligns us with IE / Edge, while Firefox does no such stretching. 

This CL makes the fix for bug 720227 unnecessary, so revert it, but keep the
tests.

BUG=724255
==========

[mstensho (USE GERRIT), 2017-06-09 14:29:01]

Description was changed from

==========
Make room for any scrollbars in the content box and border box.

The spec isn't clear. It does say to perform such clamping if the specified
border-box size is less than the total size of border+padding, but there's
nothing for scrollbars in particular. However, since we have code that
assumes that subtracting a scrollbar width from a border box size won't
result in negative values, this seems like a reasonable thing to do. This
aligns us with IE / Edge, while Firefox does no such stretching. 

This CL makes the fix for bug 720227 unnecessary, so revert it, but keep the
tests.

BUG=724255
==========

to

==========
Make room for any scrollbars in the content box and border box.

The spec isn't clear. It does say to perform such clamping if the specified
border-box size is less than the total size of border+padding, but there's
nothing for scrollbars in particular. However, since we have code that
assumes that subtracting a scrollbar width from a border box size won't
result in negative values, this seems like a reasonable thing to do. This
aligns us with IE / Edge, while Firefox does no such stretching. 

This CL makes the fix for bug 720227 unnecessary, so revert it, but keep
the tests.

Had to remove the test for bug 549174, because it assumes that a scrollbar
may overflow the border box, but that's no longer going to happen ever.

BUG=724255
==========

[mstensho (USE GERRIT), 2017-06-13 18:57:20]

Description was changed from

==========
Make room for any scrollbars in the content box and border box.

The spec isn't clear. It does say to perform such clamping if the specified
border-box size is less than the total size of border+padding, but there's
nothing for scrollbars in particular. However, since we have code that
assumes that subtracting a scrollbar width from a border box size won't
result in negative values, this seems like a reasonable thing to do. This
aligns us with IE / Edge, while Firefox does no such stretching. 

This CL makes the fix for bug 720227 unnecessary, so revert it, but keep
the tests.

Had to remove the test for bug 549174, because it assumes that a scrollbar
may overflow the border box, but that's no longer going to happen ever.

BUG=724255
==========

to

==========
Make room for any scrollbars in the content box and border box.

The spec isn't clear. It does say to perform such clamping if the specified
border-box size is less than the total size of border+padding, but there's
nothing for scrollbars in particular. However, since we have code that
assumes that subtracting a scrollbar width from a border box size won't
result in negative values, this seems like a reasonable thing to do. This
aligns us with IE / Edge, while Firefox does no such stretching. 

This CL makes the fix for bug 720227 unnecessary, so revert it, but keep
the tests.

Had to remove the test for bug 549174, because it assumes that a scrollbar
may overflow the border box, but that's no longer going to happen ever.

The test find-hidden-text.html also needed an update, since it assumed that
forcing a scrollbar on and at the same time specifying width or height as 0
would leave the border box width at 0. This is no longer the case.

BUG=724255
==========

[mstensho (USE GERRIT), 2017-06-13 18:58:45]

The CQ bit was checked by mstensho@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-13 18:59:36]

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-13 21:04:53]

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

[commit-bot: I haz the power, 2017-06-13 21:04:54]

Dry run: This issue passed the CQ dry run.

[mstensho (USE GERRIT), 2017-06-14 08:43:16]

Description was changed from

==========
Make room for any scrollbars in the content box and border box.

The spec isn't clear. It does say to perform such clamping if the specified
border-box size is less than the total size of border+padding, but there's
nothing for scrollbars in particular. However, since we have code that
assumes that subtracting a scrollbar width from a border box size won't
result in negative values, this seems like a reasonable thing to do. This
aligns us with IE / Edge, while Firefox does no such stretching. 

This CL makes the fix for bug 720227 unnecessary, so revert it, but keep
the tests.

Had to remove the test for bug 549174, because it assumes that a scrollbar
may overflow the border box, but that's no longer going to happen ever.

The test find-hidden-text.html also needed an update, since it assumed that
forcing a scrollbar on and at the same time specifying width or height as 0
would leave the border box width at 0. This is no longer the case.

BUG=724255
==========

to

==========
Make room for any scrollbars in the content box and border box.

The spec isn't clear. It does say to perform such clamping if the specified
border-box size is less than the total size of border+padding, but there's
nothing for scrollbars in particular. However, since we have code that
assumes that subtracting a scrollbar width from a border box size won't
result in negative values, this seems like a reasonable thing to do. This
aligns us with IE / Edge, while Firefox does no such stretching. 

This CL makes the fix for bug 720227
(VerticalScrollbarWidthClampedToContentBox()) unnecessary, so revert it,
but keep the tests.

Had to remove the test for bug 549174, because it assumes that a scrollbar
may overflow the border box, but that's no longer going to happen ever.

The test find-hidden-text.html also needed an update, since it assumed that
forcing a scrollbar on and at the same time specifying width or height as 0
would leave the border box width at 0. This is no longer the case.

BUG=724255
==========

[mstensho (USE GERRIT), 2017-06-14 08:46:30]

Description was changed from

==========
Make room for any scrollbars in the content box and border box.

The spec isn't clear. It does say to perform such clamping if the specified
border-box size is less than the total size of border+padding, but there's
nothing for scrollbars in particular. However, since we have code that
assumes that subtracting a scrollbar width from a border box size won't
result in negative values, this seems like a reasonable thing to do. This
aligns us with IE / Edge, while Firefox does no such stretching. 

This CL makes the fix for bug 720227
(VerticalScrollbarWidthClampedToContentBox()) unnecessary, so revert it,
but keep the tests.

Had to remove the test for bug 549174, because it assumes that a scrollbar
may overflow the border box, but that's no longer going to happen ever.

The test find-hidden-text.html also needed an update, since it assumed that
forcing a scrollbar on and at the same time specifying width or height as 0
would leave the border box width at 0. This is no longer the case.

BUG=724255
==========

to

==========
Make room for any scrollbars in the content box and border box.

The spec isn't clear. It does say to perform such clamping if the specified
border-box size is less than the total size of border+padding, but there's
nothing for scrollbars in particular. However, since we have code that
assumes that subtracting a scrollbar width from a border box size won't
result in negative values, this seems like a reasonable thing to do. This
aligns us with IE / Edge, while Firefox does no such stretching.

This CL makes the fix for bug 720227
(VerticalScrollbarWidthClampedToContentBox()) unnecessary, so revert it,
but keep the tests.

Had to remove the test for bug 549174, because it assumes that a scrollbar
may overflow the border box, but that's no longer going to happen ever.

The test find-hidden-text.html also needed an update, since it assumed that
forcing a scrollbar on and at the same time specifying width or height as 0
would leave the border box width at 0. This is no longer the case.

BUG=724255
==========

[mstensho (USE GERRIT), 2017-06-14 08:49:35]

mstensho@opera.com changed reviewers:
+ cbiesinger@chromium.org, skobes@chromium.org, szager@chromium.org

[mstensho (USE GERRIT), 2017-06-14 08:49:36]

Discussion regarding stretching the border box to fit the scrollbars:
https://codereview.chromium.org/2893833004#msg10

Also not too happy about simply removing the test for bug 549174. Maybe there is
some code to revert as well? It looks like that bug fixes a problem that will no
longer occur with this CL landed.

[skobes, 2017-06-14 14:25:02]

It looks like this will regress http://crbug.com/549174.  I don't understand
your comment: "It looks like that bug fixes a problem that will no longer occur
with this CL landed" - why will it no longer occur?

As I mentioned on http://crbug.com/724255, I don't agree that the border box
should be expanded to accomodate scrollbars when the width or height have been
explicitly specified in CSS.  This isn't the place for an intervention; the
request of the web author should be honored, otherwise it will be surprising (as
http://crbug.com/549174 demonstrates).

If negative values are causing a problem I'd rather we just clamp the content
box to be >= 0 in the code that computes the content box.  That should address
the primary concern of http://crbug.com/724255 since the scroll position would
never exceed the size of the overflow rect.

[mstensho (USE GERRIT), 2017-06-14 14:55:46]

On 2017/06/14 14:25:02, skobes wrote:
> It looks like this will regress http://crbug.com/549174.  I don't understand
> your comment: "It looks like that bug fixes a problem that will no longer
occur
> with this CL landed" - why will it no longer occur?

Hm, so the bug title is "Scrollbars are visible even if div has 0 width", which
will indeed become possible again. The problem I was referring to, that can no
longer occur, is to have the scrollbar overflow the border box. That will no
longer happen, since the border box will be stretched to fit it. I actually
thought that was the main reason for fixing that bug, but maybe I'm wrong. I may
have misinterpreted the test.

> As I mentioned on http://crbug.com/724255, I don't agree that the border box
> should be expanded to accomodate scrollbars when the width or height have been
> explicitly specified in CSS.  This isn't the place for an intervention; the
> request of the web author should be honored, otherwise it will be surprising
(as
> http://crbug.com/549174 demonstrates).

That's definitely worth a discussion. Doing this is extremely similar to how we
already stretch the border box to fit border and padding, though: <div
style="box-sizing:border-box; width:1px; padding-left:5px;"> will give you a
border box width of 5px, not 1px. This is in accordance with the spec.

> If negative values are causing a problem I'd rather we just clamp the content
> box to be >= 0 in the code that computes the content box.  That should address
> the primary concern of http://crbug.com/724255 since the scroll position would
> never exceed the size of the overflow rect.

Yes, there are indeed other solutions to this problem (if we want to fix it at
all). We already perform such clamping of the content-box though. In the test
case I attached to the bug report, when setting scrollLeft to 10000, take a look
at what goes on in PaintLayerScrollableArea::MaximumScrollOffsetInt() (where
we'll eventually end up, via LayoutBox::SetScrollLeft()).
MaximumScrollOffsetInt() will call LayoutBox::OverflowClipRect(). There we have
a clip_rect of size {2,2}. ExcludeScrollbars() will subtract the scrollbars from
that, and we'll end up with a clip_rect of size {-13,-13}! Back in
MaximumScrollOffsetInt() we'll store this size in visible_size. content_size is
the size of the scrollable area. We'll end up returning content_size -
visible_size, i.e. {5,5} - {-13,-13} = {18,18}. We can of course perform some
clamping here instead, but I'm not convinced it's the only place that needs it.

[skobes, 2017-06-14 15:51:05]

On 2017/06/14 14:55:46, mstensho wrote:
> Hm, so the bug title is "Scrollbars are visible even if div has 0 width",
which
> will indeed become possible again. The problem I was referring to, that can no
> longer occur, is to have the scrollbar overflow the border box. That will no
> longer happen, since the border box will be stretched to fit it. I actually
> thought that was the main reason for fixing that bug, but maybe I'm wrong. I
may
> have misinterpreted the test.

The point of the test, and of the patch that introduced it
(http://crrev.com/361037), is to consistently clip the scrollbars to the
declared bounds of the box.  Blink always did this in the common case but some
changes were needed for compositing scenarios.

> That's definitely worth a discussion. Doing this is extremely similar to how
we
> already stretch the border box to fit border and padding, though: <div
> style="box-sizing:border-box; width:1px; padding-left:5px;"> will give you a
> border box width of 5px, not 1px. This is in accordance with the spec.

I think what you're proposing is different from border/padding stretching, in an
important way.  With border/padding stretching the border box is still
predictable from the computed style of the element.  But your proposal makes the
border box depend on scrollbar thickness, which is platform-specific, and on
scrollbar existence, which depends in turn on whether there is overflow.

The latter in particular is the most concerning to me: it violates the
layout-containment properties currently conveyed by overflow clipping.  If I set
width/height and overflow-clipping, I'm saying I want my size to be isolated
from my contents.  Changes in my descendants, which might add/remove scrollbars,
should not affect the size I contribute to my surrounding layout.

In fact I suspect this change introduces layout invalidation bugs, since
PLSA::UpdateAfterLayout will only relayout the subtree underneath it.  It's not
prepared to relayout the surrounding document in case the border box got bigger
when the scrollbar was added.

> Yes, there are indeed other solutions to this problem (if we want to fix it at
> all). We already perform such clamping of the content-box though. In the test
> case I attached to the bug report, when setting scrollLeft to 10000, take a
look
> at what goes on in PaintLayerScrollableArea::MaximumScrollOffsetInt() (where
> we'll eventually end up, via LayoutBox::SetScrollLeft()).
> MaximumScrollOffsetInt() will call LayoutBox::OverflowClipRect(). There we
have
> a clip_rect of size {2,2}. ExcludeScrollbars() will subtract the scrollbars
from
> that, and we'll end up with a clip_rect of size {-13,-13}! Back in
> MaximumScrollOffsetInt() we'll store this size in visible_size. content_size
is
> the size of the scrollable area. We'll end up returning content_size -
> visible_size, i.e. {5,5} - {-13,-13} = {18,18}. We can of course perform some
> clamping here instead, but I'm not convinced it's the only place that needs
it.

I agree the negative rects are weird.  We should try to avoid having negative
rects anywhere.  This might entail exposing clipped and unclipped scrollbar
sizes separately.

[mstensho (USE GERRIT), 2017-06-14 19:37:21]

On 2017/06/14 15:51:05, skobes wrote:
> On 2017/06/14 14:55:46, mstensho wrote:
> > Hm, so the bug title is "Scrollbars are visible even if div has 0 width",
> which
> > will indeed become possible again. The problem I was referring to, that can
no
> > longer occur, is to have the scrollbar overflow the border box. That will no
> > longer happen, since the border box will be stretched to fit it. I actually
> > thought that was the main reason for fixing that bug, but maybe I'm wrong. I
> may
> > have misinterpreted the test.
> 
> The point of the test, and of the patch that introduced it
> (http://crrev.com/361037), is to consistently clip the scrollbars to the
> declared bounds of the box.  Blink always did this in the common case but some
> changes were needed for compositing scenarios.
> 
> > That's definitely worth a discussion. Doing this is extremely similar to how
> we
> > already stretch the border box to fit border and padding, though: <div
> > style="box-sizing:border-box; width:1px; padding-left:5px;"> will give you a
> > border box width of 5px, not 1px. This is in accordance with the spec.
> 
> I think what you're proposing is different from border/padding stretching, in
an
> important way.  With border/padding stretching the border box is still
> predictable from the computed style of the element.  But your proposal makes
the
> border box depend on scrollbar thickness, which is platform-specific, and on
> scrollbar existence, which depends in turn on whether there is overflow.
> 
> The latter in particular is the most concerning to me: it violates the
> layout-containment properties currently conveyed by overflow clipping.  If I
set
> width/height and overflow-clipping, I'm saying I want my size to be isolated
> from my contents.  Changes in my descendants, which might add/remove
scrollbars,
> should not affect the size I contribute to my surrounding layout.
> 
> In fact I suspect this change introduces layout invalidation bugs, since
> PLSA::UpdateAfterLayout will only relayout the subtree underneath it.  It's
not
> prepared to relayout the surrounding document in case the border box got
bigger
> when the scrollbar was added.
> 
> > Yes, there are indeed other solutions to this problem (if we want to fix it
at
> > all). We already perform such clamping of the content-box though. In the
test
> > case I attached to the bug report, when setting scrollLeft to 10000, take a
> look
> > at what goes on in PaintLayerScrollableArea::MaximumScrollOffsetInt() (where
> > we'll eventually end up, via LayoutBox::SetScrollLeft()).
> > MaximumScrollOffsetInt() will call LayoutBox::OverflowClipRect(). There we
> have
> > a clip_rect of size {2,2}. ExcludeScrollbars() will subtract the scrollbars
> from
> > that, and we'll end up with a clip_rect of size {-13,-13}! Back in
> > MaximumScrollOffsetInt() we'll store this size in visible_size. content_size
> is
> > the size of the scrollable area. We'll end up returning content_size -
> > visible_size, i.e. {5,5} - {-13,-13} = {18,18}. We can of course perform
some
> > clamping here instead, but I'm not convinced it's the only place that needs
> it.
> 
> I agree the negative rects are weird.  We should try to avoid having negative
> rects anywhere.  This might entail exposing clipped and unclipped scrollbar
> sizes separately.

So, something like the VerticalScrollbarWidthClampedToContentBox() thing we
currently have, but more of it? It was deemed a temporary hack, more or less, in
https://codereview.chromium.org/2893833004 - but maybe that's what we need,
after all.

Anyway, the fact that auto scrollbars may suddenly affect the size of the border
box (depending on the amount of content that's inside) sounds really horrible,
so I'm closing this now. I wonder how Edge gets away with doing it like I'm
proposing in this CL, though.

[cbiesinger, 2017-06-14 19:39:02]

On 2017/06/14 19:37:21, mstensho wrote:
> On 2017/06/14 15:51:05, skobes wrote:
> > On 2017/06/14 14:55:46, mstensho wrote:
> > > Hm, so the bug title is "Scrollbars are visible even if div has 0 width",
> > which
> > > will indeed become possible again. The problem I was referring to, that
can
> no
> > > longer occur, is to have the scrollbar overflow the border box. That will
no
> > > longer happen, since the border box will be stretched to fit it. I
actually
> > > thought that was the main reason for fixing that bug, but maybe I'm wrong.
I
> > may
> > > have misinterpreted the test.
> > 
> > The point of the test, and of the patch that introduced it
> > (http://crrev.com/361037), is to consistently clip the scrollbars to the
> > declared bounds of the box.  Blink always did this in the common case but
some
> > changes were needed for compositing scenarios.
> > 
> > > That's definitely worth a discussion. Doing this is extremely similar to
how
> > we
> > > already stretch the border box to fit border and padding, though: <div
> > > style="box-sizing:border-box; width:1px; padding-left:5px;"> will give you
a
> > > border box width of 5px, not 1px. This is in accordance with the spec.
> > 
> > I think what you're proposing is different from border/padding stretching,
in
> an
> > important way.  With border/padding stretching the border box is still
> > predictable from the computed style of the element.  But your proposal makes
> the
> > border box depend on scrollbar thickness, which is platform-specific, and on
> > scrollbar existence, which depends in turn on whether there is overflow.
> > 
> > The latter in particular is the most concerning to me: it violates the
> > layout-containment properties currently conveyed by overflow clipping.  If I
> set
> > width/height and overflow-clipping, I'm saying I want my size to be isolated
> > from my contents.  Changes in my descendants, which might add/remove
> scrollbars,
> > should not affect the size I contribute to my surrounding layout.
> > 
> > In fact I suspect this change introduces layout invalidation bugs, since
> > PLSA::UpdateAfterLayout will only relayout the subtree underneath it.  It's
> not
> > prepared to relayout the surrounding document in case the border box got
> bigger
> > when the scrollbar was added.
> > 
> > > Yes, there are indeed other solutions to this problem (if we want to fix
it
> at
> > > all). We already perform such clamping of the content-box though. In the
> test
> > > case I attached to the bug report, when setting scrollLeft to 10000, take
a
> > look
> > > at what goes on in PaintLayerScrollableArea::MaximumScrollOffsetInt()
(where
> > > we'll eventually end up, via LayoutBox::SetScrollLeft()).
> > > MaximumScrollOffsetInt() will call LayoutBox::OverflowClipRect(). There we
> > have
> > > a clip_rect of size {2,2}. ExcludeScrollbars() will subtract the
scrollbars
> > from
> > > that, and we'll end up with a clip_rect of size {-13,-13}! Back in
> > > MaximumScrollOffsetInt() we'll store this size in visible_size.
content_size
> > is
> > > the size of the scrollable area. We'll end up returning content_size -
> > > visible_size, i.e. {5,5} - {-13,-13} = {18,18}. We can of course perform
> some
> > > clamping here instead, but I'm not convinced it's the only place that
needs
> > it.
> > 
> > I agree the negative rects are weird.  We should try to avoid having
negative
> > rects anywhere.  This might entail exposing clipped and unclipped scrollbar
> > sizes separately.
> 
> So, something like the VerticalScrollbarWidthClampedToContentBox() thing we
> currently have, but more of it? It was deemed a temporary hack, more or less,
in
> https://codereview.chromium.org/2893833004 - but maybe that's what we need,
> after all.
> 
> Anyway, the fact that auto scrollbars may suddenly affect the size of the
border
> box (depending on the amount of content that's inside) sounds really horrible,
> so I'm closing this now. I wonder how Edge gets away with doing it like I'm
> proposing in this CL, though.

So we currently have an inconsistency in how we treat border/padding vs how we
treat scrollbars. The former increase the size of a box whereas the latter  does
not. Why is it a good thing to have this inconsistency?

[cbiesinger, 2017-06-14 19:39:58]

Oh, I just caught up to some of the above discussion. I guess the overflow: auto
example is a good point :/

[skobes, 2017-06-15 02:43:27]

On 2017/06/14 19:37:21, mstensho wrote:
> So, something like the VerticalScrollbarWidthClampedToContentBox() thing we
> currently have, but more of it?

Yes that's roughly what I had in mind, though I'd just call it
ClippedVerticalScrollbarWidth.

Even better might be for VerticalScrollbarWidth to always return the clipped
value.  But maybe that breaks other assumptions.