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
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.
<rdar://problem/11709988>
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.