In regular block layout, the width of a child's margin box should always be equal to that of its containing block

[Zhifei Fang]

Hi,

Orignally, I have filed a bug in blink here:
https://bugs.chromium.org/p/chromium/issues/detail?id=708751

Today I find I repoduce it on my safari.


After checking the code, it looks we have same root cause here

        LayoutUnit newMargin = containerLogicalWidth - computedValues.m_extent - cb.marginStartForChild(*this);
        bool hasInvertedDirection = cb.style().isLeftToRightDirection() != style().isLeftToRightDirection();
        if (hasInvertedDirection)
            computedValues.m_margins.m_start = newMargin;
        else
            computedValues.m_margins.m_end = newMargin;


Simply because at this time marginStartForChild have not been set yet.

if you saw here:

void RenderBox::updateLogicalWidth()
{
    LogicalExtentComputedValues computedValues;
    computeLogicalWidthInRegion(computedValues);

    setLogicalWidth(computedValues.m_extent);
    setLogicalLeft(computedValues.m_position);
    setMarginStart(computedValues.m_margins.m_start);
    setMarginEnd(computedValues.m_margins.m_end);
}

we are calling computeLogicalWidthInRegion first, then use the result to update the margin.

Therefore below path will fix it as I did for blink:

        LayoutUnit newMarginTotal = containerLogicalWidth - computedValues.m_extent;
        bool hasInvertedDirection = cb.style().isLeftToRightDirection() != style().isLeftToRightDirection();
        if (hasInvertedDirection)
            computedValues.m_margins.m_start = 
                 newMarginTotal - computedValues..m_margins.m_end;
        else
            computedValues.m_margins.m_end = 
                 newMarginTotal - computedValues..m_margins.m_start;

[Zhifei Fang]

Please confirm, then I can start the commit process.

[Alexey Proskuryakov]

Please see <https://webkit.org/contributing-code/>. It is unlikely that anyone will have a lot of comments about a code snippet, so the best way to proceed is to make a patch as described in this document, and once all tests pass, someone can review.

[Zhifei Fang]

Woring on it.

[Zhifei Fang]

(In reply to Alexey Proskuryakov from comment #2)
> Please see <https://webkit.org/contributing-code/>. It is unlikely that
> anyone will have a lot of comments about a code snippet, so the best way to
> proceed is to make a patch as described in this document, and once all tests
> pass, someone can review.

I just wonder should this bug firstly assign to me, not sure if it is not assign to me I can upload the code or not.

Thanks!

[Alexey Proskuryakov]

You can attach patches without having the bug assigned to you.

[Zhifei Fang]

Created attachment 321174 [details]
Patch

[Zhifei Fang]

Need a reviewer

[Dave Hyatt]

Comment on attachment 321174 [details]
Patch

I see that this is a port of a Blink patch. It might be good to reference that, since this is actually a Blink merge and not an original patch.

[Simon Fraser (smfr)]

Comment on attachment 321174 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=321174&action=review

> Source/WebCore/rendering/RenderBox.cpp:2453
> +            computedValues.m_margins.m_start = 
> +                newMarginTotal - computedValues.m_margins.m_end;

Don't wrap these lines.

> Source/WebCore/rendering/RenderBox.cpp:2456
> +            computedValues.m_margins.m_end = 
> +                newMarginTotal - computedValues.m_margins.m_start;

Ditto.

> LayoutTests/fast/block/over-constrained-auto-margin.html:21
> +        <script>

I think this test could just dump the render tree, and not run any script.

[zalan]

Comment on attachment 321174 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=321174&action=review

> Source/WebCore/rendering/RenderBox.cpp:2448
> +        // We should not use the cb.marginStartForChild here, it has not been set yet

This comment is confusing as it refers to something that's not there anymore. It should rather be part of the Changlog entry.

[Zhifei Fang]

Comment on attachment 321174 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=321174&action=review

>> LayoutTests/fast/block/over-constrained-auto-margin.html:21
>> +        <script>
> 
> I think this test could just dump the render tree, and not run any script.

I am not sure about "dump the render tree", do we dump it as a picture? if so, the margin right will not been show in the picture, the error happened with the computed style it is should be -116px instead of -106px, we cannot tell it on the picture.

[Zhifei Fang]

(In reply to Dave Hyatt from comment #8)
> Comment on attachment 321174 [details]
> Patch
> 
> I see that this is a port of a Blink patch. It might be good to reference
> that, since this is actually a Blink merge and not an original patch.

Add it in the change log ?

[zalan]

Comment on attachment 321174 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=321174&action=review

>>> LayoutTests/fast/block/over-constrained-auto-margin.html:21
>>> +        <script>
>> 
>> I think this test could just dump the render tree, and not run any script.
> 
> I am not sure about "dump the render tree", do we dump it as a picture? if so, the margin right will not been show in the picture, the error happened with the computed style it is should be -116px instead of -106px, we cannot tell it on the picture.

Dumping the render tree means something like this ->
layer at (0,0) size 943x600
  RenderView at (0,0) size 800x600
layer at (0,0) size 800x528
  RenderBlock {HTML} at (0,0) size 800x528
    RenderBody {BODY} at (8,8) size 784x0
      RenderBlock {DIV} at (0,0) size 935x0
        RenderBlock (floating) {DIV} at (0,0) size 935x500 [bgcolor=#EEEEEE]
          RenderBlock {DIV} at (0,0) size 935x500 [bgcolor=#ADD8E6]
        RenderBlock (floating) {DIV} at (0,0) size 146x300 [bgcolor=#A9A9A9]
        RenderBlock (floating) {DIV} at (635,0) size 300x300 [bgcolor=#A9A9A9]

But I think running script is fine, it's just I don't see why you remove the child at the end. Shouldn't it be something like instead?
document.body.offsetHeight; 
shouldBe("getComputedStyle(target1).marginRight", "'-116px'");
shouldBe("getComputedStyle(target2).marginLeft", "'-116px'");
(no need to do getElementById)

[zalan]

(In reply to Zhifei Fang from comment #12)
> (In reply to Dave Hyatt from comment #8)
> > Comment on attachment 321174 [details]
> > Patch
> > 
> > I see that this is a port of a Blink patch. It might be good to reference
> > that, since this is actually a Blink merge and not an original patch.
> 
> Add it in the change log ?
Yes, please.

[Zhifei Fang]

(In reply to zalan from comment #13)
> Comment on attachment 321174 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=321174&action=review
> 
> >>> LayoutTests/fast/block/over-constrained-auto-margin.html:21
> >>> +        <script>
> >> 
> >> I think this test could just dump the render tree, and not run any script.
> > 
> > I am not sure about "dump the render tree", do we dump it as a picture? if so, the margin right will not been show in the picture, the error happened with the computed style it is should be -116px instead of -106px, we cannot tell it on the picture.
> 
> Dumping the render tree means something like this ->
> layer at (0,0) size 943x600
>   RenderView at (0,0) size 800x600
> layer at (0,0) size 800x528
>   RenderBlock {HTML} at (0,0) size 800x528
>     RenderBody {BODY} at (8,8) size 784x0
>       RenderBlock {DIV} at (0,0) size 935x0
>         RenderBlock (floating) {DIV} at (0,0) size 935x500 [bgcolor=#EEEEEE]
>           RenderBlock {DIV} at (0,0) size 935x500 [bgcolor=#ADD8E6]
>         RenderBlock (floating) {DIV} at (0,0) size 146x300 [bgcolor=#A9A9A9]
>         RenderBlock (floating) {DIV} at (635,0) size 300x300
> [bgcolor=#A9A9A9]
> 
> But I think running script is fine, it's just I don't see why you remove the
> child at the end. Shouldn't it be something like instead?
> document.body.offsetHeight; 
> shouldBe("getComputedStyle(target1).marginRight", "'-116px'");
> shouldBe("getComputedStyle(target2).marginLeft", "'-116px'");
> (no need to do getElementById)

Got it, thanks !

[Zhifei Fang]

(In reply to zalan from comment #14)
> (In reply to Zhifei Fang from comment #12)
> > (In reply to Dave Hyatt from comment #8)
> > > Comment on attachment 321174 [details]
> > > Patch
> > > 
> > > I see that this is a port of a Blink patch. It might be good to reference
> > > that, since this is actually a Blink merge and not an original patch.
> > 
> > Add it in the change log ?
> Yes, please.

No problem, will do it.

[Dave Hyatt]

Comment on attachment 321174 [details]
Patch

r=me, just throw the link to the crbug in the changeling, so that all the history/discussion can be easily seen. Thanks.

[Dave Hyatt]

ChangeLog. Stupid autocomplete. :)

[Zhifei Fang]

(In reply to Dave Hyatt from comment #18)
> ChangeLog. Stupid autocomplete. :)

sure thing, will do

[Zhifei Fang]

Created attachment 321408 [details]
Patch

[Zhifei Fang]

(In reply to Zhifei Fang from comment #20)
> Created attachment 321408 [details]
> Patch

New patch!

[WebKit Commit Bot]

Comment on attachment 321408 [details]
Patch

Clearing flags on attachment: 321408

Committed r222321: <http://trac.webkit.org/changeset/222321>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34693454>