Clamp shape-margin to zero

Clamp shape-margin to zero

The shape-margin is supposed to be non-negative, this
is hinted at by the spec:
http://dev.w3.org/csswg/css-shapes/#shape-margin-property

BUG=450598
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194638

[rwlbuis, 2015-04-07 20:56:00]

rob.buis@samsung.com changed reviewers:
+ bemjb@chromium.org

[rwlbuis, 2015-04-07 20:56:00]

PTAL. I discussed it with astearns and this seems the way to go.

[rwlbuis, 2015-04-07 20:56:32]

rob.buis@samsung.com changed reviewers:
+ bjonesbe@adobe.com

[Zoltan, 2015-04-13 20:33:08]

zoltan@adobe.com changed reviewers:
+ zoltan@adobe.com

[Zoltan, 2015-04-13 20:33:08]

https://codereview.chromium.org/1036653002/diff/20001/Source/core/layout/shap...
File Source/core/layout/shapes/ShapeOutsideInfo.cpp (right):

https://codereview.chromium.org/1036653002/diff/20001/Source/core/layout/shap...
Source/core/layout/shapes/ShapeOutsideInfo.cpp:156: LayoutUnit maximumValue =
m_renderer.containingBlock() ?
std::max(m_renderer.containingBlock()->contentWidth(), LayoutUnit()) :
LayoutUnit();
I think it would be more readable if you just do something like

LayoutUnit maximumValue;
if (m_renderer.containingBlock() && m_renderer.containingBlock()->contentWidth()
> 0)
    maximumValue = m_renderer.containingBlock()->contentWidth();

[Zoltan, 2015-04-14 17:39:07]

lgtm

[rwlbuis, 2015-04-14 21:03:35]

rob.buis@samsung.com changed reviewers:
+ leviw@chromium.org

[rwlbuis, 2015-04-14 21:03:35]

@leviw PTAL

[leviw_travelin_and_unemployed, 2015-04-14 21:12:31]

https://codereview.chromium.org/1036653002/diff/40001/Source/core/layout/shap...
File Source/core/layout/shapes/ShapeOutsideInfo.cpp (right):

https://codereview.chromium.org/1036653002/diff/40001/Source/core/layout/shap...
Source/core/layout/shapes/ShapeOutsideInfo.cpp:157: if
(m_renderer.containingBlock() && m_renderer.containingBlock()->contentWidth() >
0)
The contentWidth() is negative? That doesn't seem right... Why is this
happening? This seems like the wrong place for this fix.

[rwlbuis, 2015-04-16 21:31:22]

On 2015/04/14 21:12:31, leviw wrote:
>
https://codereview.chromium.org/1036653002/diff/40001/Source/core/layout/shap...
> File Source/core/layout/shapes/ShapeOutsideInfo.cpp (right):
> 
>
https://codereview.chromium.org/1036653002/diff/40001/Source/core/layout/shap...
> Source/core/layout/shapes/ShapeOutsideInfo.cpp:157: if
> (m_renderer.containingBlock() && m_renderer.containingBlock()->contentWidth()
>
> 0)
> The contentWidth() is negative? That doesn't seem right... Why is this
> happening? This seems like the wrong place for this fix.

Note that the (only) reason it becomes negative in the test is because of the
overflow-y: scroll. Remove it and it becomes positive again.

From playing around with the FireFox inspector, there the content box shown in
"Box Model" does not take the overflow scrollbar into account. I played around
with making contentWidth not take into account the overflow scrollbar width, but
I am sure we rely on the old behavior and many things will break, I stopped at
fast/css/nested-rounded-corners.html since even for that one I am not sure what
call site of contentWidth to fix.

In my latest patch set I just introduce a contentWidthExcludingScrollbar, using
that the problem is fixed.

Let me know if I am on the right track, if so maybe the best way is to
transition to the new behavior contentWidth, but that seems like a potentially
huge issue.

[leviw_travelin_and_unemployed, 2015-04-21 22:58:42]

https://codereview.chromium.org/1036653002/diff/80001/Source/core/css/StylePr...
File Source/core/css/StylePropertySet.h (right):

https://codereview.chromium.org/1036653002/diff/80001/Source/core/css/StylePr...
Source/core/css/StylePropertySet.h:118: bool forceOverride() const { return
m_forceOverride; }
Do you mean to include this stuff? Is it necessary?

[rwlbuis, 2015-04-22 00:52:01]

On 2015/04/21 22:58:42, leviw wrote:
>
https://codereview.chromium.org/1036653002/diff/80001/Source/core/css/StylePr...
> File Source/core/css/StylePropertySet.h (right):
> 
>
https://codereview.chromium.org/1036653002/diff/80001/Source/core/css/StylePr...
> Source/core/css/StylePropertySet.h:118: bool forceOverride() const { return
> m_forceOverride; }
> Do you mean to include this stuff? Is it necessary?

Sorry, two patches were squashed :( Patch set 6 should have just the relevant
changes!

[leviw_travelin_and_unemployed, 2015-04-22 01:02:16]

https://codereview.chromium.org/1036653002/diff/100001/Source/core/layout/sha...
File Source/core/layout/shapes/ShapeOutsideInfo.cpp (right):

https://codereview.chromium.org/1036653002/diff/100001/Source/core/layout/sha...
Source/core/layout/shapes/ShapeOutsideInfo.cpp:156: LayoutUnit maximumValue =
m_renderer.containingBlock() ?
m_renderer.containingBlock()->contentWidthExcludingScrollbar() : LayoutUnit();
Will this still yield the right answer when we have a scrollbar and a
non-negative content width?

[rwlbuis, 2015-04-22 21:01:11]

On 2015/04/22 01:02:16, leviw wrote:
>
https://codereview.chromium.org/1036653002/diff/100001/Source/core/layout/sha...
> File Source/core/layout/shapes/ShapeOutsideInfo.cpp (right):
> 
>
https://codereview.chromium.org/1036653002/diff/100001/Source/core/layout/sha...
> Source/core/layout/shapes/ShapeOutsideInfo.cpp:156: LayoutUnit maximumValue =
> m_renderer.containingBlock() ?
> m_renderer.containingBlock()->contentWidthExcludingScrollbar() : LayoutUnit();
> Will this still yield the right answer when we have a scrollbar and a
> non-negative content width?

I added a test for that in the testcase, it looks good to me.
Note that Firefox seems to not show the scrollbar when there is not enough
room/content to show it, which may automatically fix their internal
calculations.

[rwlbuis, 2015-04-28 21:13:07]

On 2015/04/22 21:01:11, rwlbuis wrote:
> On 2015/04/22 01:02:16, leviw wrote:
> >
>
https://codereview.chromium.org/1036653002/diff/100001/Source/core/layout/sha...
> > File Source/core/layout/shapes/ShapeOutsideInfo.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1036653002/diff/100001/Source/core/layout/sha...
> > Source/core/layout/shapes/ShapeOutsideInfo.cpp:156: LayoutUnit maximumValue
=
> > m_renderer.containingBlock() ?
> > m_renderer.containingBlock()->contentWidthExcludingScrollbar() :
LayoutUnit();
> > Will this still yield the right answer when we have a scrollbar and a
> > non-negative content width?
> 
> I added a test for that in the testcase, it looks good to me.
> Note that Firefox seems to not show the scrollbar when there is not enough
> room/content to show it, which may automatically fix their internal
> calculations.

ping

[leviw_travelin_and_unemployed, 2015-04-28 21:31:51]

Sorry for all the noise on this (and the slow review)!

I actually think LayoutUnit maximumValue = max(LayoutUnit(),
m_renderer.containingBlock()->contentWidth()); is the way to go. Is that alright
with you? A comment about what's going on would be good, too.

[rwlbuis, 2015-04-28 23:32:37]

On 2015/04/28 21:31:51, leviw wrote:
> Sorry for all the noise on this (and the slow review)!
> 
> I actually think LayoutUnit maximumValue = max(LayoutUnit(),
> m_renderer.containingBlock()->contentWidth()); is the way to go. Is that
alright
> with you? A comment about what's going on would be good, too.

Yes, that is fine by me. I updated the patch :)

[leviw_travelin_and_unemployed, 2015-04-28 23:34:01]

The CQ bit was checked by leviw@chromium.org

[leviw_travelin_and_unemployed, 2015-04-28 23:34:02]

lgtm

Sorry for all the runaround!

[leviw_travelin_and_unemployed, 2015-04-28 23:34:02]

The patchset sent to the CQ was uploaded after l-g-t-m from zoltan@adobe.com
Link to the patchset: https://codereview.chromium.org/1036653002/#ps140001
(title: "Address review comments")

[commit-bot: I haz the power, 2015-04-28 23:34:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1036653002/140001

[rwlbuis, 2015-04-29 00:17:22]

On 2015/04/28 23:34:02, leviw wrote:
> lgtm
> 
> Sorry for all the runaround!

No worries!

[commit-bot: I haz the power, 2015-04-29 00:46:59]

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194638