[webkit-reviews] review denied: [Bug 77754] REGRESSION (r94492): Unstable layout of static block inside text-align: center div : [Attachment 151751] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied Robert Hogan
<robert at webkit.org>'s request for review:
Bug 77754: REGRESSION (r94492): Unstable layout of static block inside
text-align: center div
https://bugs.webkit.org/show_bug.cgi?id=77754

Attachment 151751: Patch
https://bugs.webkit.org/attachment.cgi?id=151751&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=151751&action=review


I found your change super confusing because you don't explain *anything* you
are doing and why this won't open another slew of issues.

> Source/WebCore/ChangeLog:9
> +	   Positioned elements do not align to the center as though they were
text, instead their
> +	   logical offset depending on direction takes the center alignment.

I don't understand this explanation, especially the part after 'instead'

> Source/WebCore/ChangeLog:16
> +	     it depended on the child's width.

Could you mention that you are reverting
http://trac.webkit.org/changeset/113584/trunk/Source/WebCore/rendering/RenderBl
ock.cpp as it didn't help with fixing the issue? I shouldn't have to blame to
understand that *sigh*

> Source/WebCore/ChangeLog:18
> +	   (RenderBlock): Remove child parm from startAlignedOffsetForLine()

Please let's not shorten words: s/parm/parameter/

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2828
> +	   return logicalWidth() - logicalLeft;

Shouldn't you patch updateLogicalWidthForCenterAlignedBlock to return the right
left offset instead?

Btw, I wonder how this bug can be a regression from r94452 considering this
line was added after (r95726)?

>>> LayoutTests/ChangeLog:13
>>> +		 These tests are no longer valid.
>> 
>> Is bug 4860 still considered fixed? What tests are checking that?
> 
> Yes, it's only the center-aligned case that's problematic. The other
alignment issues fixed in 4860 still hold.

Could you justify why bug4860-absolute-inline-child-inherits-alignment.html is
invalid?

align-positioned-object-on-resize.html seems wrong to me if we look at CSS 2.1
as 'text-align' only applies to block containers. You could however make it
valid again by changing the tag to be a <div>