Bug 77754 – REGRESSION (r94492): Unstable layout of static block inside text-align: center div

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Tim Horton]

Created attachment 125367 [details]
repro

Steps to Reproduce:

1. Open the attached repro in ToT WebKit.
2. Resize the window.

After 1) you have an <input> which hangs off the right edge of the screen, and starts in the middle.
After 2) you have an <input> which is centered nicely, like intended.

This regressed in http://trac.webkit.org/changeset/94492/trunk

<rdar://problem/10701179>

See also bug 74723. Seems like some cases are broken even after fixing that.

And see bug 4860 comment #20, regarding instability. I guess the code that calls logicalWidthForChild() is still in there…

[Robert Hogan]

Even when it's right it's wrong - the position is always a bit off centre in the test case because the child's width is always stale. I don't see much alternative to re-computing the child's width each time in RenderBlock::startAlignedOffsetForLine() at the moment.

[Robert Hogan]

(In reply to comment #3)
> Even when it's right it's wrong - the position is always a bit off centre in the test case because the child's width is always stale. I don't see much alternative to re-computing the child's width each time in RenderBlock::startAlignedOffsetForLine() at the moment.

Mitz, what do you think of this suggestion? Should I proceed with it?

[Robert Hogan]

So the issue is that a block that has central alignment (the absolute positioned span in this case) currently calculates the inline position of any inline positioned children before the width of the child (which is necessary determine that position) has been computed in the current layout. This happens in in setStaticPositions(), specifically the call to setStaticInlinePositionForChild() using startAlignedOffsetForLine().

Central alignment is the only case where this is a problem because it's the only one that relies on the width of the child to be known before the child has been through layout.

So the options I see are:

- Compute the width of the child early in startAlignedOffsetForLine() if the block has central alignment. This will duplicate the subsequent computeLogicalWidth() in the child when it goes through layout.

- Do something like this:

--- a/Source/WebCore/rendering/RenderBox.cpp
+++ b/Source/WebCore/rendering/RenderBox.cpp
@@ -1646,6 +1646,7 @@ void RenderBox::computeLogicalWidthInRegion(RenderRegion* region, LayoutUnit off
         // FIXME: This calculation is not patched for block-flow yet.
         // https://bugs.webkit.org/show_bug.cgi?id=46500
         computePositionedLogicalWidth(region, offsetFromLogicalTopOfFirstPage);
+        layer()->setStaticInlinePosition(toRenderBlock(parent())->startAlignedOffsetForLine(this, toRenderBlock(parent())->logicalHeight(), false));
         return;
     }

The second one seems better, and might allow me to remove the inline child case from setStaticPositions() altogether. 

What do you think?

[Robert Hogan]

(In reply to comment #5)
> 
> The second one seems better, and might allow me to remove the inline child case from setStaticPositions() altogether. 
> 
> What do you think?

<dhyatt> hmmm
<dhyatt> i mean the other idea would be to just defer the setting of the positions until after line layout
<dhyatt> like don't do it right as you encounter positioned objects
<dhyatt> but wait for the line to be finished and then do it

me: Which is the ideal way - I'll give it a try.

[Robert Hogan]

Positioned objects with centred alignment in leading white space really are a special case. Unlike positioned objects preceded by some text they don't get their inline position updated after line layout. So it only happens when they're first encountered in setStaticPositions(). This is easy enough to cater for and allow their width to get computed in just the problem case, but it seems any attempt to prevent a second call to computeLogicalWidth() when the positioned object gets laid out later might get quite intrusive.

[Dave Hyatt]

Couldn't you just add them to a Vector and walk them after line layout?

[Tim Horton]

Hey, Robert! Have you had a chance to take a look at this again recently?

[Robert Hogan]

(In reply to comment #9)
> Hey, Robert! Have you had a chance to take a look at this again recently?

Was planning to pick it up again in the next day or two. So yes, it's still with me.

[Robert Hogan]

I had another look and am not sure I can improve on my initial suggestion, which is to make sure the width of the child is up to date before using it in startAlignedOffsetForLine().

I have not found a good place to set the static position of the positioned child once its width has been calculated in the normal course of events. I can't do it just after line layout, because it hasn't been calculated there - the line is empty so line layout exits early. 

This only leaves layoutPositionedObjects(), just after the positioned object has been laid out. Does that sound OK?

[Robert Hogan]

Created attachment 134849 [details]
Patch

[Dave Hyatt]

Comment on attachment 134849 [details]
Patch

r=me

[Robert Hogan]

Committed r113584: <http://trac.webkit.org/changeset/113584>

[Robert Hogan]

thorton reports this still isn't working! :/

[Tim Horton]

(In reply to comment #15)
> thorton reports this still isn't working! :/

Dan and I debugged and he pointed out that there's an additional layout for you, caused by the scrollbars. Since we have overlay scrollbars, we don't do that extra layout. And, indeed, I don't see the problem if I turn legacy scrollbars on.

So, this is still an unstable layout problem, just more subtle than before.

We also significantly reduced the test case to the point where the code added in the fix for this bug is no longer hit, so the fix was too narrow in scope. Attaching that in a moment.

[Tim Horton]

Created attachment 150686 [details]
more reduced repro (needs overlay scrollbars)

[WebKit Review Bot]

Attachment 150686 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1
Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 150686 [details]
more reduced repro (needs overlay scrollbars)

You can avoid the need for overlay scrollbars by setting the height of the body element to something large

[Tim Horton]

(In reply to comment #19)
> You can avoid the need for overlay scrollbars by setting the height of the body element to something large

^ This doesn't really work, the problem is severely minimized by this for some reason.

[Tim Horton]

Robert's fix happens way too late in layout; we never get back to any code that propagates the position down into the RenderBlock (unless we end up doing layout again, which Mac doesn't).

[Robert Hogan]

(In reply to comment #21)
> Robert's fix happens way too late in layout; we never get back to any code that propagates the position down into the RenderBlock (unless we end up doing layout again, which Mac doesn't).

The thing that prevents me from agreeing with you is that if layoutPositionedObjects() has been called then the child has had layout, its width is therefore known, so it should be safe to set the static position of its parent.

Can a positioned object to get layout, and therefore change its width, outside layoutPositionedObjects()?

[Tim Horton]

Ergh, sorry, Robert. After a long discussion with hyatt and mitz and smfr, it seems that the initial layout is actually the correct one, and that subsequent layouts (where it's actually centered) are wrong...

It seems like the out-of-flow static block shouldn't affect the line width, and its x position alone should be determined by the text-align of its parent (thus, putting the left edge of the block at 50%).

Firefox gets this right. Opera doesn't. We get it right in the first layout, and then wrong in subsequent layouts.

[Robert Hogan]

Created attachment 151501 [details]
Patch

[Build Bot]

Comment on attachment 151501 [details]
Patch

Attachment 151501 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13201116

[Robert Hogan]

Created attachment 151751 [details]
Patch

[Robert Hogan]

Hi Dan,

Can you r? this?

Thanks,
Robert

[Andy Estes]

r113584 caused a regression on Apple's Store locator site. I filed <https://bugs.webkit.org/show_bug.cgi?id=91452>. I suspect that the patch currently up for review might resolve this, in which case feel free to dupe that bug to this one.

[Alexey Proskuryakov]

Comment on attachment 151751 [details]
Patch

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

> LayoutTests/ChangeLog:13
> +        * fast/css/align-positioned-object-on-resize-expected.txt: Removed.
> +        * fast/css/align-positioned-object-on-resize.html: Removed.
> +        * fast/css/bug4860-absolute-inline-child-inherits-alignment-expected.png: Removed.
> +        * fast/css/bug4860-absolute-inline-child-inherits-alignment-expected.txt: Removed.
> +        * fast/css/bug4860-absolute-inline-child-inherits-alignment.html: Removed.
> +            These tests are no longer valid.

Is bug 4860 still considered fixed? What tests are checking that?

[Alexey Proskuryakov]

Ditto for bug 77754.

[Alexey Proskuryakov]

(strike that last comment)

[Robert Hogan]

(In reply to comment #29)
> (From update of attachment 151751 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151751&action=review
> 
> > LayoutTests/ChangeLog:13
> > +        * fast/css/align-positioned-object-on-resize-expected.txt: Removed.
> > +        * fast/css/align-positioned-object-on-resize.html: Removed.
> > +        * fast/css/bug4860-absolute-inline-child-inherits-alignment-expected.png: Removed.
> > +        * fast/css/bug4860-absolute-inline-child-inherits-alignment-expected.txt: Removed.
> > +        * fast/css/bug4860-absolute-inline-child-inherits-alignment.html: Removed.
> > +            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.

[Julien Chaffraix]

Comment on attachment 151751 [details]
Patch

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/RenderBlock.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>

[Robert Hogan]

Comment on attachment 151751 [details]
Patch

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

>> 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)?

When this line was added it was getting the invalid logicalLeft value introduced by r94452, and had to use totalLogicalWidth to keep the alignment consistent. So this line didn't introduce a bug in r95726 but it did have to put up with the bogus value of logicalLeft from r94452.

re patching updateLogicalWidthForCenterAlignedBlock() - startAlignedOffsetForLine() is specifically for out-of-flow elements so the RTL adjustment performed here isn't suitable for in-flow elements.

>>>> 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>

Will do.

[Robert Hogan]

Created attachment 157781 [details]
Patch

[Robert Hogan]

(In reply to comment #34)
> > 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>
> 
> Will do.

I didn't reinstate this test as the width of the input element is rendered incorrectly in WebKit. Opened bug https://bugs.webkit.org/show_bug.cgi?id=93735 to look at it in more detail.

[Dave Hyatt]

Comment on attachment 157781 [details]
Patch

r=me

[Robert Hogan]

Committed r126911: <http://trac.webkit.org/changeset/126911>

[Robert Hogan]

Reopening to attach new patch.

[Robert Hogan]

Created attachment 161051 [details]
Patch

Comment on attachment 161051 [details]
Patch

This patch is not about this bug.

This was fixed in r126911.

[Philip Rogers]

(In reply to comment #42)
> This was fixed in r126911.

I think this may have broken several tests:
fast/css/absolute-child-with-percent-height-inside-relative-parent.html
fast/css/center-align-absolute-position-inline-block.html
fast/css/center-align-absolute-position.html
fast/inline/absolute-positioned-inline-in-centred-block.html
fast/inline/left-right-center-inline-alignment-in-ltr-and-rtl-blocks.html

[Julien Chaffraix]

(In reply to comment #43)
> (In reply to comment #42)
> > This was fixed in r126911.
> 
> I think this may have broken several tests:
> fast/css/absolute-child-with-percent-height-inside-relative-parent.html
> fast/css/center-align-absolute-position-inline-block.html
> fast/css/center-align-absolute-position.html
> fast/inline/absolute-positioned-inline-in-centred-block.html
> fast/inline/left-right-center-inline-alignment-in-ltr-and-rtl-blocks.html

This is a false alarm. I did rebaseline some of the tests and that's why they showed up. Rebaselined the rest in http://trac.webkit.org/changeset/126934. Sorry for the noise.