Bug 89515 – REGRESSION(r113885): Margin not properly applied to elements with align=center

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Levi Weintraub]

Created attachment 148429 [details]
Partial reduction

The rendering of the "Character Sheet" on http://evegate.com (and the attached reduction by eseidel) diverged from that of Gecko with http://trac.webkit.org/changeset/113885/.

Upstream Chromium bug: http://code.google.com/p/chromium/issues/detail?id=130069

[Eric Seidel (no email)]

https://gate.eveonline.com/Profile/etouchqa is an example, btw.  No login should be needed.

[Eric Seidel (no email)]

I also just tested Opera's (amazingly slow?) rendering of the site.  Opera matches FF.

I suspect another few minutes of working on the reduction would let us know what's the actual behavior change.  It appears to be doing some sort of text-align: center calculation with a bunch of relative-position/floated/negative-margin objects. :(

[Julien Chaffraix]

Created attachment 148441 [details]
Further reduction - combination of fixed size, align=center and margin-left (positive)

[Levi Weintraub]

(In reply to comment #3)
> Created an attachment (id=148441) [details]
> Further reduction - combination of fixed size, align=center and margin-left (positive)

Well done!! Looks like this should be retitled.

[Eric Seidel (no email)]

Thank you very much for the excellent reduction Julien!

[Eric Seidel (no email)]

Can we use text-align: center instead of align="center"?

[Julien Chaffraix]

(In reply to comment #6)
> Can we use text-align: center instead of align="center"?

You were close: you need to use text-align: -webkit-center (for some reason, we translate align="center" to that for <div>). The test case works with text-align: center FWIW.

[Eric Seidel (no email)]

Yeah, I remember that align="center" behaves somehow differently from text-align: center, but I don't remember the details.  Leaving the reduction using non-prefixed properties is of course best (as you've done). :)

[Julien Chaffraix]

Thanks Eric for the question that narrowed down the issue.

The core issue was uncovered by r113885 but it's likely it has been around for some time: RenderBox::computeInlineDirectionMargins doesn't account for margins set in CSS in the case of align=center. This seems to differ from other browsers (FF, Opera and IE9).

[Eric Seidel (no email)]

Does that mean we have an even further reduction? :)  Sounds like we're close to a patch in either case!  Thanks again for jumping in on this.

[Julien Chaffraix]

(In reply to comment #10)
> Does that mean we have an even further reduction? :) 

It is possible to reduce it a bit (I have managed to remove the text). I have a patch that needs proper testing before I submit it.

I think I understand the issue but it's concerning that the original change made us ignore margins not only for align="center" but also for align="right" and "left". We have only heard about align="center" but I fear it's a matter of time before we get people complaining about the other cases.

[Alexey Proskuryakov]

<rdar://problem/11709988>

[Julien Chaffraix]

Created attachment 148618 [details]
Proposed change v1: patch computeInlineDirectionMargins.

[Levi Weintraub]

Comment on attachment 148618 [details]
Proposed change v1: patch computeInlineDirectionMargins.

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

> Source/WebCore/rendering/RenderBox.cpp:1840
> -        containingBlock->setMarginStartForChild(this, max<LayoutUnit>(0, (containerWidth - childWidth) / 2));
> -        containingBlock->setMarginEndForChild(this, containerWidth - childWidth - containingBlock->marginStartForChild(this));
> +        // Other browsers center the margin box for align=center elements so we match them here.
> +        LayoutUnit marginStartWidth = marginStartLength.value();
> +        LayoutUnit marginEndWidth = marginEndLength.value();
> +        LayoutUnit centeredMarginBoxStart = max<LayoutUnit>(0, (containerWidth - childWidth - marginStartWidth - marginEndWidth) / 2);
> +        containingBlock->setMarginStartForChild(this, centeredMarginBoxStart + marginStartWidth);
> +        containingBlock->setMarginEndForChild(this, containerWidth - childWidth - containingBlock->marginStartForChild(this) + marginEndWidth);

Seems like the right fix to me!

[Eric Seidel (no email)]

Comment on attachment 148618 [details]
Proposed change v1: patch computeInlineDirectionMargins.

Ref tests are sooooo verbose. :(

[Tony Chang]

(In reply to comment #11)
> I think I understand the issue but it's concerning that the original change made us ignore margins not only for align="center" but also for align="right" and "left". We have only heard about align="center" but I fear it's a matter of time before we get people complaining about the other cases.

What happens with align="right" or align="left"?  Do we have some tests for this case already?  If not, should we add some?

[Julien Chaffraix]

(In reply to comment #16)
> (In reply to comment #11)
> > I think I understand the issue but it's concerning that the original change made us ignore margins not only for align="center" but also for align="right" and "left". We have only heard about align="center" but I fear it's a matter of time before we get people complaining about the other cases.
> 
> What happens with align="right" or align="left"?  Do we have some tests for this case already?  If not, should we add some?

I haven't looked deeper but it's likely like align="center", there was very little testing. As mentioned in the ChangeLog, I will investigate and fix / test those cases in follow-up bugs as I haven't found any filed bugs related to them so far.

[Levi Weintraub]

Comment on attachment 148618 [details]
Proposed change v1: patch computeInlineDirectionMargins.

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

>> Source/WebCore/rendering/RenderBox.cpp:1840
>> +        containingBlock->setMarginEndForChild(this, containerWidth - childWidth - containingBlock->marginStartForChild(this) + marginEndWidth);
> 
> Seems like the right fix to me!

Sounds good!

[Julien Chaffraix]

Fixed landed in http://trac.webkit.org/changeset/120859

[Julien Chaffraix]

(In reply to comment #19)
> Fixed landed in http://trac.webkit.org/changeset/120859

This fix doesn't cover non fixed length, opened bug 89626 to correct that.