Summary: | REGRESSION(r113885): Margin not properly applied to elements with align=center | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Levi Weintraub <leviw> | ||||||||
Component: | Layout and Rendering | Assignee: | Julien Chaffraix <jchaffraix> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | eric, hyatt, jchaffraix, leviw, mitz, ojan, tony | ||||||||
Priority: | P1 | Keywords: | HasReduction, InRadar | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
URL: | https://gate.eveonline.com/Profile/etouchqa | ||||||||||
Bug Depends on: | 83614 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Levi Weintraub
2012-06-19 14:34:38 PDT
https://gate.eveonline.com/Profile/etouchqa is an example, btw. No login should be needed. 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. :( Created attachment 148441 [details]
Further reduction - combination of fixed size, align=center and margin-left (positive)
(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. Thank you very much for the excellent reduction Julien! Can we use text-align: center instead of align="center"? (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. 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). :) 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). 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. (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. Created attachment 148618 [details]
Proposed change v1: patch computeInlineDirectionMargins.
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 on attachment 148618 [details]
Proposed change v1: patch computeInlineDirectionMargins.
Ref tests are sooooo verbose. :(
(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? (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 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! Fixed landed in http://trac.webkit.org/changeset/120859 (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. |