WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
77754
REGRESSION (
r94492
): Unstable layout of static block inside text-align: center div
https://bugs.webkit.org/show_bug.cgi?id=77754
Summary
REGRESSION (r94492): Unstable layout of static block inside text-align: cente...
Tim Horton
Reported
2012-02-03 11:15:43 PST
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
>
Attachments
repro
(308 bytes, text/html)
2012-02-03 11:15 PST
,
Tim Horton
no flags
Details
Patch
(5.43 KB, patch)
2012-03-30 11:48 PDT
,
Robert Hogan
no flags
Details
Formatted Diff
Diff
more reduced repro (needs overlay scrollbars)
(153 bytes, text/html)
2012-07-03 16:07 PDT
,
Tim Horton
no flags
Details
Patch
(337.02 KB, patch)
2012-07-10 12:29 PDT
,
Robert Hogan
no flags
Details
Formatted Diff
Diff
Patch
(339.60 KB, patch)
2012-07-11 12:30 PDT
,
Robert Hogan
no flags
Details
Formatted Diff
Diff
Patch
(340.42 KB, patch)
2012-08-10 11:46 PDT
,
Robert Hogan
no flags
Details
Formatted Diff
Diff
Patch
(129.33 KB, patch)
2012-08-28 13:55 PDT
,
Robert Hogan
mitz: review-
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
mitz
Comment 1
2012-02-03 11:50:56 PST
See also
bug 74723
. Seems like some cases are broken even after fixing that.
mitz
Comment 2
2012-02-03 11:53:25 PST
And see
bug 4860 comment #20
, regarding instability. I guess the code that calls logicalWidthForChild() is still in there…
Robert Hogan
Comment 3
2012-02-04 12:23:19 PST
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
Comment 4
2012-02-16 12:00:10 PST
(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
Comment 5
2012-02-20 14:12:57 PST
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
Comment 6
2012-02-20 14:27:51 PST
(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
Comment 7
2012-02-20 15:07:59 PST
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
Comment 8
2012-02-22 13:19:17 PST
Couldn't you just add them to a Vector and walk them after line layout?
Tim Horton
Comment 9
2012-03-21 20:58:17 PDT
Hey, Robert! Have you had a chance to take a look at this again recently?
Robert Hogan
Comment 10
2012-03-22 12:32:29 PDT
(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
Comment 11
2012-03-27 14:05:49 PDT
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
Comment 12
2012-03-30 11:48:15 PDT
Created
attachment 134849
[details]
Patch
Dave Hyatt
Comment 13
2012-04-04 12:27:41 PDT
Comment on
attachment 134849
[details]
Patch r=me
Robert Hogan
Comment 14
2012-04-09 09:36:58 PDT
Committed
r113584
: <
http://trac.webkit.org/changeset/113584
>
Robert Hogan
Comment 15
2012-07-03 11:53:25 PDT
thorton reports this still isn't working! :/
Tim Horton
Comment 16
2012-07-03 16:05:56 PDT
(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
Comment 17
2012-07-03 16:07:17 PDT
Created
attachment 150686
[details]
more reduced repro (needs overlay scrollbars)
WebKit Review Bot
Comment 18
2012-07-03 16:11:30 PDT
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.
mitz
Comment 19
2012-07-03 16:12:16 PDT
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
Comment 20
2012-07-03 17:11:59 PDT
(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
Comment 21
2012-07-05 14:43:43 PDT
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
Comment 22
2012-07-06 10:30:04 PDT
(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
Comment 23
2012-07-06 12:39:50 PDT
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
Comment 24
2012-07-10 12:29:36 PDT
Created
attachment 151501
[details]
Patch
Build Bot
Comment 25
2012-07-10 15:42:23 PDT
Comment on
attachment 151501
[details]
Patch
Attachment 151501
[details]
did not pass mac-ews (mac): Output:
http://queues.webkit.org/results/13201116
Robert Hogan
Comment 26
2012-07-11 12:30:17 PDT
Created
attachment 151751
[details]
Patch
Robert Hogan
Comment 27
2012-07-16 14:28:52 PDT
Hi Dan, Can you r? this? Thanks, Robert
Andy Estes
Comment 28
2012-07-16 17:32:54 PDT
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 29
2012-07-23 13:44:50 PDT
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
Comment 30
2012-07-23 13:46:01 PDT
Ditto for
bug 77754
.
Alexey Proskuryakov
Comment 31
2012-07-23 13:46:34 PDT
(strike that last comment)
Robert Hogan
Comment 32
2012-07-23 14:19:16 PDT
(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 33
2012-08-03 11:52:41 PDT
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 34
2012-08-10 11:13:40 PDT
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
Comment 35
2012-08-10 11:46:53 PDT
Created
attachment 157781
[details]
Patch
Robert Hogan
Comment 36
2012-08-10 12:00:52 PDT
(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 37
2012-08-27 11:53:21 PDT
Comment on
attachment 157781
[details]
Patch r=me
Robert Hogan
Comment 38
2012-08-28 12:20:31 PDT
Committed
r126911
: <
http://trac.webkit.org/changeset/126911
>
Robert Hogan
Comment 39
2012-08-28 13:55:34 PDT
Reopening to attach new patch.
Robert Hogan
Comment 40
2012-08-28 13:55:40 PDT
Created
attachment 161051
[details]
Patch
mitz
Comment 41
2012-08-28 14:09:11 PDT
Comment on
attachment 161051
[details]
Patch This patch is not about this bug.
mitz
Comment 42
2012-08-28 14:10:46 PDT
This was fixed in
r126911
.
Philip Rogers
Comment 43
2012-08-28 15:08:03 PDT
(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
Comment 44
2012-08-28 15:54:39 PDT
(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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug