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…
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.
(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?
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?
(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.
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.
Couldn't you just add them to a Vector and walk them after line layout?
Hey, Robert! Have you had a chance to take a look at this again recently?
(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.
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?
Created attachment 134849 [details] Patch
Comment on attachment 134849 [details] Patch r=me
Committed r113584: <http://trac.webkit.org/changeset/113584>
thorton reports this still isn't working! :/
(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.
Created attachment 150686 [details] more reduced repro (needs overlay scrollbars)
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
(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.
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).
(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()?
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.
Created attachment 151501 [details] Patch
Comment on attachment 151501 [details] Patch Attachment 151501 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13201116
Created attachment 151751 [details] Patch
Hi Dan, Can you r? this? Thanks, Robert
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.
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?
Ditto for bug 77754.
(strike that last comment)
(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.
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>
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.
Created attachment 157781 [details] Patch
(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.
Comment on attachment 157781 [details] Patch r=me
Committed r126911: <http://trac.webkit.org/changeset/126911>
Reopening to attach new patch.
Created attachment 161051 [details] Patch
Comment on attachment 161051 [details] Patch This patch is not about this bug.
This was fixed in r126911.
(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
(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.