Bug 89515 - REGRESSION(r113885): Margin not properly applied to elements with align=center
Summary: REGRESSION(r113885): Margin not properly applied to elements with align=center
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Julien Chaffraix
URL: https://gate.eveonline.com/Profile/et...
Keywords: HasReduction, InRadar
Depends on: 83614
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-19 14:34 PDT by Levi Weintraub
Modified: 2012-06-20 18:27 PDT (History)
7 users (show)

See Also:


Attachments
Partial reduction (1.35 KB, text/html)
2012-06-19 14:34 PDT, Levi Weintraub
no flags Details
Further reduction - combination of fixed size, align=center and margin-left (positive) (308 bytes, text/html)
2012-06-19 15:28 PDT, Julien Chaffraix
no flags Details
Proposed change v1: patch computeInlineDirectionMargins. (20.51 KB, patch)
2012-06-20 11:56 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Levi Weintraub 2012-06-19 14:34:38 PDT
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
Comment 1 Eric Seidel (no email) 2012-06-19 14:50:51 PDT
https://gate.eveonline.com/Profile/etouchqa is an example, btw.  No login should be needed.
Comment 2 Eric Seidel (no email) 2012-06-19 14:53:13 PDT
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. :(
Comment 3 Julien Chaffraix 2012-06-19 15:28:12 PDT
Created attachment 148441 [details]
Further reduction - combination of fixed size, align=center and margin-left (positive)
Comment 4 Levi Weintraub 2012-06-19 15:30:08 PDT
(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.
Comment 5 Eric Seidel (no email) 2012-06-19 16:18:18 PDT
Thank you very much for the excellent reduction Julien!
Comment 6 Eric Seidel (no email) 2012-06-19 16:20:18 PDT
Can we use text-align: center instead of align="center"?
Comment 7 Julien Chaffraix 2012-06-19 16:27:57 PDT
(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.
Comment 8 Eric Seidel (no email) 2012-06-19 16:37:20 PDT
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). :)
Comment 9 Julien Chaffraix 2012-06-19 17:52:58 PDT
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).
Comment 10 Eric Seidel (no email) 2012-06-19 19:59:12 PDT
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.
Comment 11 Julien Chaffraix 2012-06-20 10:17:01 PDT
(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.
Comment 12 Alexey Proskuryakov 2012-06-20 10:38:53 PDT
<rdar://problem/11709988>
Comment 13 Julien Chaffraix 2012-06-20 11:56:34 PDT
Created attachment 148618 [details]
Proposed change v1: patch computeInlineDirectionMargins.
Comment 14 Levi Weintraub 2012-06-20 12:04:43 PDT
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!
Comment 15 Eric Seidel (no email) 2012-06-20 12:39:13 PDT
Comment on attachment 148618 [details]
Proposed change v1: patch computeInlineDirectionMargins.

Ref tests are sooooo verbose. :(
Comment 16 Tony Chang 2012-06-20 13:24:07 PDT
(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?
Comment 17 Julien Chaffraix 2012-06-20 13:34:23 PDT
(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.
Comment 18 Levi Weintraub 2012-06-20 13:57:21 PDT
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!
Comment 19 Julien Chaffraix 2012-06-20 14:07:26 PDT
Fixed landed in http://trac.webkit.org/changeset/120859
Comment 20 Julien Chaffraix 2012-06-20 18:27:54 PDT
(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.