Able to reproduce the issue on Windows-7, Mac-10.12.4 and Linux Ubuntu-14.04 using chrome stable version 57.0.2987.133 and canary 59.0.3066.0 with the steps mentioned in comment#0.
This is Non-regression issue,observed from M40 and marking it as Untriaged to get more inputs from dev team.
Please find the attached screen cast for reference.

Thanks.

Deleted: 708751.mp4 690 KB

	708751.mp4 690 KB View Download

Which element do you inspect? Where is the wrong number shown for you? Computed styles pane shows all margins as zeros for all of the elements in your example html.

The "p" element in the HTML I provided in comment 0.

lushni… via monorail <monorail+v2.108612539@chromium.org>于2017年4月10日周一
17:15写道：

Right, thanks! I see it now.

Eric, could you please take a look?

Thanks for the report.  It looks like this is not a bug in DevTools, but rather the ComputedStyleCSSValueMapping::get could be fixed.

Firefox 52 reports marginRight: 0px, while Safari 10 and IE 11 give -106px.

Hi,

I have discovered more.

The computeMarginsForDirection Method have caculate the margin right as 0 right.

However, there is a following calculation recaculate the margin agian in LayoutBox::computeLogicalWidth  (LayoutBox.cpp:1934)

    // Margin calculations.
    computeMarginsForDirection(InlineDirection, cb, containerLogicalWidth, computedValues.m_extent, computedValues.m_margins.m_start,
        computedValues.m_margins.m_end, style()->marginStart(), style()->marginEnd());
    if (!hasPerpendicularContainingBlock && containerLogicalWidth && containerLogicalWidth != (computedValues.m_extent + computedValues.m_margins.m_start + computedValues.m_margins.m_end)
        && !isFloating() && !isInline() && !cb->isFlexibleBoxIncludingDeprecated() && !cb->isLayoutGrid()) {
        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;
    }


This part actually rewrite computedValues.m_margins.m_end = newMargin;

In this case it should be 400-506-10 = -116 

This most likely because the  cb->marginStartForChild(*this) gives 0 instead of 10

anyway, this is not right.

Above is not right, I guess the purpose of this part is when the child have smaller width than parent, we will set the following margin to make the it fill the parent's width.

Therefore, the condition 

 if (!hasPerpendicularContainingBlock && containerLogicalWidth && containerLogicalWidth != (computedValues.m_extent + computedValues.m_margins.m_start + computedValues.m_margins.m_end)

should be changed to

 if (!hasPerpendicularContainingBlock && containerLogicalWidth && containerLogicalWidth > (computedValues.m_extent + computedValues.m_margins.m_start + computedValues.m_margins.m_end)

Maybe you guys can assign it to me, I will go for the commit step when I home.

https://codereview.chromium.org/2841823006

cc'ing a few people with ownership and activity in LayoutBox.cpp who may be able to better review.  @chrishtr, @wangxianzhu, could you please take a look, or forward to someone who can?

I think this bug is invalid, and that Blink and Edge already behave correctly.

https://www.w3.org/TR/2011/REC-CSS2-20110607/visudet.html#blockwidth first says:

<quote>
The following constraints must hold among the used values of the other properties:
'margin-left' + 'border-left-width' + 'padding-left' + 'width' + 'padding-right' + 'border-right-width' + 'margin-right' = width of containing block
</quote>

Example:
<div id="cb" style="width:400px;">
    <div id="child" style="width:500px;"></div>
</div>

Here, margin-left, border-left-width, padding-left, padding-right, border-right-width and margin-right of #child are all 0, so we get:

0 + 0 + 0 + 500px + 0 + 0 + 0 = 500px, while the width of the containing block (#parent) is 400px.

We have an over-constrained situation, and the spec says:

<quote>
If all of the above have a computed value other than 'auto', the values are said to be "over-constrained" and one of the used values will have to be different from its computed value. If the 'direction' property of the containing block has the value 'ltr', the specified value of 'margin-right' is ignored and the value is calculated so as to make the equality true. If the value of 'direction' is 'rtl', this happens to 'margin-left' instead.
</quote>

I.e. we override the computed value of margin-right (which was 0) to satisfy the constraint, so we set it to -100px, since 500px - 100px == 400px.

With auto margin-right:
<div id="cb" style="width:400px;">
    <div id="child" style="width:500px; margin-right:auto;"></div>
</div>

That shouldn't make much of a difference, since the spec says:

<quote>
If 'width' is not 'auto' and 'border-left-width' + 'padding-left' + 'width' + 'padding-right' + 'border-right-width' (plus any of 'margin-left' or 'margin-right' that are not 'auto') is larger than the width of the containing block, then any 'auto' values for 'margin-left' or 'margin-right' are, for the following rules, treated as zero.
</quote>

So we treat margin-right as 0, and again we have an over-constrained situation, and margin-right must be overridden to -100px.

Hi,
Give the test case here again:

<head>
    <style>
        div {
            width: 400px; border: 3px solid black;
        }
        p {
            margin-left: 10px; width: 500px; border: 3px solid gray; margin-right: auto;
        }
        body {
            margin: 0;
        }

    </style>
</head>
<body>
    <div>
        <p>A paragraph</p>
    </div>
</body>


<quote>
If <b>all</b> of the above have a computed value other than <b>'auto'</b>,
<qoute>

This only happened when the marign-right is not set to auto, in this test case it is set to auto, so it should be zero, and should not go to the "over-constrained".

I guess I need modify my change a little bit more here.



And Even we choose to override this margin-right value.

In this test case it should be  400-500-10(margin-left)-6(border) = -116

But we got -106 instead, you can see comment 2 gives out the result, I am still invesigating the reason.


Above all:
Let me give out a new testcase

<html>
<head>
</head>
<body>
    <div id="target-1" style="width: 400px; border: 3px solid black;">
        <div style="margin-left: 10px; width: 500px; border: 3px solid gray; margin-right: auto;">Margin right should be 0</div>
    </div>
    <div id="target-2" style="width: 400px; border: 3px solid black;">
        <div style="margin-left: 10px; width: 500px; border: 3px solid gray; margin-right: 10%;">Margine right should be -116</div>
    </div>
</body>
</html>

The target-1 block have the child set margin-right auto, so at last the margin-right should be 0; which I got -106.

The target-2 block have the child set margin-right a computed value 10% not auto, based on the "over-constrained" sistuation， the specified value of 'margin-right' is ignored, and should replaced to -116, which I got 40;

<quote>
'margin-right', 'margin-left'
Computed value:  	the percentage as specified or the absolute length
</quote>

I guess something here is totally wrong.

And yes, the looking is the same, only a value is not right, and in most cases it is not harmful.

However, when a guy try to learn the CSS standard, this makes him confused, I recently received a question about this, let's make this right.

The spec says that if stuff gets wider than the containing block, auto margins are resolved to 0.

If width is non-auto, and all of the relevant border-width, padding and margin values are non-auto, we have an over-constrained situation.

If auto margins have been resolved to 0, because stuff was too wide, well, then we have an over-constrained situation (since nothing is auto anymore), and one of the margin sides (margin-right in the case of direction:ltr) has to be overridden to satisfy the constraint (margin box width == containing block width).

Do you disagree?

Oh, Ok, I understand now.

However we still have the issue.

<html>
<head>
</head>
<body>
    <div id="target-1" style="width: 400px; border: 3px solid black;">
        <div style="margin-left: 10px; width: 500px; border: 3px solid gray; margin-right: auto;">Margin right should be 0</div>
    </div>
    <div id="target-2" style="width: 400px; border: 3px solid black;">
        <div style="margin-left: 10px; width: 500px; border: 3px solid gray; margin-right: 10%;">Margine right should be -116</div>
    </div>
</body>
</html>

then the target-1 and target-2 should have same margin right -116

For　target-1 we resovled it as 0, and then we have over-constrained,  we should computed margin right out as 500-400-10-6 = -116

For target-2 we firstly resovled it as 40, and then we have over-constrained, we should computed margin right out as 500-400-10-6 = -116

however I got -106 and 40 instead.

Thanks.

Hi,
Accoring to the following code for fixed and percentage margin, we will show the style's value instead of the computed value.
So the dev tool actually not showing the exactly marign right for target 2, do we need a improve here ?


    case CSSPropertyMarginRight: {
      Length margin_right = style.MarginRight();
      if (margin_right.IsFixed() || !layout_object || !layout_object->IsBox())
        return ZoomAdjustedPixelValueForLength(margin_right, style);
      float value;
      if (margin_right.IsPercentOrCalc()) {
        // LayoutBox gives a marginRight() that is the distance between the
        // right-edge of the child box and the right-edge of the containing box,
        // when display == EDisplay::kBlock. Let's calculate the absolute value
        // of the specified margin-right % instead of relying on LayoutBox's
        // marginRight() value.
        value = MinimumValueForLength(
                    margin_right, ToLayoutBox(layout_object)
                                      ->ContainingBlockLogicalWidthForContent())
                    .ToFloat();
      } else {
        value = ToLayoutBox(layout_object)->MarginRight().ToFloat();
      }
      return ZoomAdjustedPixelValue(value, style);
    }


I will try to dig more why we have -106 instead of -116 for the auto.

Thanks !

Your analysis here seems correct. *This* looks like a bug, and hopefully one we don't share with Edge (I cannot test on Windows right now). So, shall we let this bug report be about that, then? Then we need a different fix, (and a different bug report summary; margin-right shouldn't necessarily become 0px - it should become whatever satisfies the "margin box width == containing block width" constraint).

(my previous response was in response to message #21)

Sure thing, I will fix it, I am not very familiar with Monrail, it seems I need create a new issue, correct ?

You don't NEED to. It's up to you. You can either re-use this one and modify the summary, or close this as WontFix and create a new one. If you don't have the necessary permissions, I can do it for you.

It seems like I cannot modify the summary, thanks if you can help me to do so.

Done.

So, in over-constrained situations, we always have to adjust the margin side that has no impact on visual layout, so that margin box width == containing block width. In LTR layout, that means margin-right. In RTL layout, it means margin-left.

Fixing this will have no direct impact on layout, but since we expose the margin values via getComputedStyle() (and it's also exposed by devtools), this is worth fixing.

LayoutUnit new_margin = container_logical_width - computed_values.extent_ -
                            cb->MarginStartForChild(*this);

This line is wrong, when doing so cb->MarginStartForChild(*this) actually return the old margin, we need to use the new computed ones.

case CSSPropertyMarginRight: {
      Length margin_right = style.MarginRight();
      if (margin_right.IsFixed() || !layout_object || !layout_object->IsBox())
        return ZoomAdjustedPixelValueForLength(margin_right, style);
      float value;
      if (margin_right.IsPercentOrCalc()) {
        // LayoutBox gives a marginRight() that is the distance between the
        // right-edge of the child box and the right-edge of the containing box,
        // when display == EDisplay::kBlock. Let's calculate the absolute value
        // of the specified margin-right % instead of relying on LayoutBox's
        // marginRight() value.
        value = MinimumValueForLength(
                    margin_right, ToLayoutBox(layout_object)
                                      ->ContainingBlockLogicalWidthForContent())
                    .ToFloat();
      } else {
        value = ToLayoutBox(layout_object)->MarginRight().ToFloat();
      }
      return ZoomAdjustedPixelValue(value, style);
    }


About this, for dev tool, for over-constrained situations, for fix and percentage do we show the real margin, or we just show the resolved value?

I saw firefox still show the resolved value.

I don't know what's more natural to do for devtools.

As for what getComputedStyle() is supposed to return, it's the "resolved value". It's specified here: https://drafts.csswg.org/cssom/#resolved-values

In the case of margin-right, it should be the used value (i.e. the value you end up after resolving auto, percentages and over-constrainedness). So Chrome is doing the right thing. It's just that the final value is wrong, because of this bug.

Let's keep the dev tool computed style for now. Since the dev tool draw the margin box uses the real value. Maybe we can start a new thread for it.

I will figure out how to add a LayoutTest today.

Thanks!

https://codereview.chromium.org/2841823006

added the test requested.

Anyone? Outstanding code review.

Servo is compliant in this respect, and we can download and test it: https://download.servo.org. 

It should be the WebKit  bug 13343 [1], which caused the wrong margin calculation.

[1]https://bugs.webkit.org/show_bug.cgi?id=13343

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/34d81295269809c64d9e12a5921db3ada416a043

commit 34d81295269809c64d9e12a5921db3ada416a043
Author: facetothefate <facetothefate@gmail.com>
Date: Thu May 18 01:20:29 2017

Resolve inline margins in over-constrained situations correctly.

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

BUG= 708751 

Review-Url: https://codereview.chromium.org/2841823006
Cr-Commit-Position: refs/heads/master@{#472619}

[modify] https://crrev.com/34d81295269809c64d9e12a5921db3ada416a043/AUTHORS
[add] https://crrev.com/34d81295269809c64d9e12a5921db3ada416a043/third_party/WebKit/LayoutTests/fast/block/over-constrained-auto-margin.html
[modify] https://crrev.com/34d81295269809c64d9e12a5921db3ada416a043/third_party/WebKit/Source/core/layout/LayoutBox.cpp