Use correct container width as base for percentage margins.

Use correct container width as base for percentage margins.

Blocks establishing a new BFC need to avoid floats in its parent BFC. Also,
computing used values for auto margins for such blocks is affected by the
available line width constrained by these floats. However, percentage sized
margins must be calculated based on the full containing block width, not the
available line width.

R=jchaffraix@chromium.org
BUG=348927
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169030

[Julien - ping for review, 2014-03-06 23:05:05]

https://codereview.chromium.org/185723005/diff/1/LayoutTests/fast/css/bfc-per...
File LayoutTests/fast/css/bfc-percentage-margin-expected.html (right):

https://codereview.chromium.org/185723005/diff/1/LayoutTests/fast/css/bfc-per...
LayoutTests/fast/css/bfc-percentage-margin-expected.html:15: <div class="square
right"></div>
It's weird that the offset is 128px when the left-most square is not even offset
by 8px (I assume this is due to the body's margin).

https://codereview.chromium.org/185723005/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/185723005/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:2548: LayoutUnit containerLogicalWidth =
containingBlockLogicalWidthForContent();
I seems weird that we always use containingBlockLogicalWidthForContent() here
and don't check for floats and use containingBlockAvailableLineWidth().

Because of that, I can't suggest to avoid the new parameter and always pass the
availableWidth and do the float check at the beginning of
computeInlineDirectionMargins.

[rune, 2014-03-07 14:20:23]

https://codereview.chromium.org/185723005/diff/1/LayoutTests/fast/css/bfc-per...
File LayoutTests/fast/css/bfc-percentage-margin-expected.html (right):

https://codereview.chromium.org/185723005/diff/1/LayoutTests/fast/css/bfc-per...
LayoutTests/fast/css/bfc-percentage-margin-expected.html:15: <div class="square
right"></div>
On 2014/03/06 23:05:05, Julien Chaffraix - PST wrote:
> It's weird that the offset is 128px when the left-most square is not even
offset
> by 8px (I assume this is due to the body's margin).

Yes, I've modified the reference html now to add a relative container to be able
to use 120px for left instead.

https://codereview.chromium.org/185723005/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/185723005/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:2548: LayoutUnit containerLogicalWidth =
containingBlockLogicalWidthForContent();
On 2014/03/06 23:05:05, Julien Chaffraix - PST wrote:
> I seems weird that we always use containingBlockLogicalWidthForContent() here
> and don't check for floats and use containingBlockAvailableLineWidth().
> 
> Because of that, I can't suggest to avoid the new parameter and always pass
the
> availableWidth and do the float check at the beginning of
> computeInlineDirectionMargins.

Done that now. I also added a layout test showing how a float in a horizontal
writing mode container would now be taken into account for computing auto
margins for a bfc child with a vertical writing mode. I tried to read the
writing mode spec and twist my mind around combinations of perpendicular blocks
and floats going in all sorts of directions. I cannot say I fully succeeded, but
I think what the new layout test shows with this code change makes sense.

[Julien - ping for review, 2014-03-10 20:27:01]

The code change looks great.

I think we should add a test for the table + orthogonal writing mode case (see
RenderBox::computeLogicalHeight that calls computeInlineDirectionMargins).
Checking percentage margins in this case should be enough as they will be
computed based on the total containing block size now.

https://codereview.chromium.org/185723005/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/185723005/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:2445: LayoutUnit availableWidth =
containerWidth;
Everyone is passing containingBlockLogicalWidthForContent() after this change so
could we just remove the containerWidth argument and call
containingBlockLogicalWidthForContent() here instead?

[rune, 2014-03-12 12:21:36]

Added table test now.

https://codereview.chromium.org/185723005/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/185723005/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:2445: LayoutUnit availableWidth =
containerWidth;
On 2014/03/10 20:27:01, Julien Chaffraix - PST wrote:
> Everyone is passing containingBlockLogicalWidthForContent() after this change
so
> could we just remove the containerWidth argument and call
> containingBlockLogicalWidthForContent() here instead?

containingBlockLogicalWidthForContent() is a virtual call, and it calls
RenderObject::containingBlock() which does not seem to be very cheap either.
Since the result of containgBlockLogicalWidthForContent() is used several times
in the relevant places (apart from one), I think it would be good to keep that
optimization.

[Julien - ping for review, 2014-03-12 15:09:06]

lgtm

[commit-bot: I haz the power, 2014-03-12 15:09:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rune@opera.com/185723005/40001

[commit-bot: I haz the power, 2014-03-12 16:25:54]

Change committed as 169030