<rdar://problem/10238920> To reproduce: open the URL in the Mac App Store. The price buttons in the rightmost column have their text shifted to the right and clipped out. This was caused by <http://trac.webkit.org/r94492>. See bug 4860 comment #20.
See https://bugs.webkit.org/show_bug.cgi?id=4860#c21
Created attachment 119757 [details] Patch
Comment on attachment 119757 [details] Patch This change is incorrect. It will fail to skip space after a positioned object. I am going to attach a test case that shows this.
Created attachment 119764 [details] Test case with collapsible space after a leading positioned object This is an example of a case that renders incorrectly with attachment 119757 [details] applied.
Created attachment 119767 [details] Patch
(In reply to comment #3) > (From update of attachment 119757 [details]) > This change is incorrect. It will fail to skip space after a positioned object. I am going to attach a test case that shows this. Thanks for the quick review. You're right. I've updated accordingly with a few more test cases to show that leading whitespace is discarded properly.
This is still wrong for some combinations of leading absolute and float elements with whitespace. I'm still looking at it.
Comment on attachment 119767 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119767&action=review I really can't say anything about the change so I looked at the tests instead. > LayoutTests/fast/css/absolute-inline-alignment-expected.html:1 > + Unneeded leading empty line. > LayoutTests/fast/css/absolute-inline-alignment.html:1 > + Ditto. > LayoutTests/fast/css/absolute-inline-alignment.html:29 > +<div>​<span></span> Lorem ipsum dolor sit amet</div> Those 2 lines are exactly the same between the test and its reference. Is it intended?
Created attachment 120178 [details] Patch
Comment on attachment 120178 [details] Patch Attachment 120178 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10993583
Comment on attachment 120178 [details] Patch Attachment 120178 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10993582
Comment on attachment 120178 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120178&action=review I gave up on trying to add positioned elements in the leading whitespace into the inline run. Turns out the complexity of handling subsequent whitespace was the good good reason they weren't there in the first place. The alternative to adding inline positioned elements to the positioned objects list is to create and use a separate positioned objects list for them, but I used the existing list for now. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1890 > + if (object->style()->isOriginalDisplayInlineType() && !object->isRenderFullScreen()) The reference to isRenderFullScreen is necessary to pass fullscreen/full-screen-zIndex.html. Full screen elements seem to be positioned inlines I guess. Obviously this check needs to be something else but I need advice on what it should be. > LayoutTests/fast/css/absolute-inline-alignment-expected.html:29 > +<div id="fixed" style="top:0px"></div> > +<div>Lorem ipsum dolor sit amet</div> I haven't used the unicode zero-width whitespace in the reference result because it actually adds one pixel of width to the run. This puts a reference result that uses them out by one pixel.
Created attachment 120467 [details] Patch
(In reply to comment #13) > Created an attachment (id=120467) [details] > Patch Fixed the build for ports without full screen API enabled. RenderFullScreen is a fixed position renderer with an original inline display style so the specific exclusion seems necessary given the way full screen is expected to behave. I'd be happier using a separate hashset for leading inline positioned objects rather than the existing one. The reason I haven't is because I don't want to add a few bytes to every block for a fairly rare use case.
Created attachment 120505 [details] Test case with bidirectional reordering
Comment on attachment 120467 [details] Patch Using RenderBlock::m_positioned for this seems unnecessarily complex and dangerous. Moreover, this patch doesn’t handle bidirectional reordering correctly. I think this bug can be fixed simply and correctly by just patching skipLeadingWhitespace() to create and add a BidiRun for each formerly-inline positioned object it encounters.
(In reply to comment #16) > (From update of attachment 120467 [details]) > Using RenderBlock::m_positioned for this seems unnecessarily complex and dangerous. Moreover, this patch doesn’t handle bidirectional reordering correctly. I think this bug can be fixed simply and correctly by just patching skipLeadingWhitespace() to create and add a BidiRun for each formerly-inline positioned object it encounters. That works extremely well. Thanks! Revised patch to follow.
Created attachment 120558 [details] Patch
Comment on attachment 120558 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120558&action=review r- because of the regression in the case of lines with only positioned objects and a <br> in quirks mode. I think you can fix it by adding to LineInfo a counter of “positioned object runs from leading whitespace”, incrementing it as needed in skipLeadingWhitespace, then subtracting it from bidiRuns.runCount() when computing isOnlyRun in constructLine(). > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1881 > + appendRunsForObject(bidiRuns, resolver.position().offset(), object->length(), object, resolver); appendRunsForObject() contains a lot of logic that’s irrelevant here; also, since you know object is a box, you don’t need to call length() (not that the length matters here). I would just do it like this: resolver.runs().addRun(createRun(0, 1, object, resolver); > LayoutTests/ChangeLog:21 > + * platform/chromium-linux/fast/inline/styledEmptyInlinesWithBRs-expected.png: > + * platform/chromium-linux/fast/replaced/absolute-position-percentage-height-expected.png: > + * platform/chromium-win/fast/inline/styledEmptyInlinesWithBRs-expected.txt: > + * platform/chromium-win/fast/replaced/absolute-position-percentage-height-expected.txt: > + * platform/chromium-win/media/audio-controls-rendering-expected.txt: > + With this patch, WebKit now treats the inline positioned <img> as an element in the inline run, so the > + first subsequent <br> does not contribute any height. Before, the inline positioned element was not in the run > + so the first <br> was treated as free-standing, contributing 20px of height - hence the smaller height in > + the updated results. Neither Firefox nor Opera allow the <br>'s to contribute any height to the rendering of the <img> element. These changes show that the patch is incorrect. The presence of a positioned object on the line should not affect how the <br> element behaves. The problem is that now that it contributes a run, it causes isOnlyRun to be false in constructLine(), and consequently causes the line to have no text descendants (for the purposes of the quirks mode behavior) and thus collapse.
Created attachment 120598 [details] Patch
Comment on attachment 120598 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120598&action=review > Source/WebCore/rendering/InlineFlowBox.h:200 > + void setHasTextDescendants() > + { > + m_hasTextDescendants = true; > + if (parent()) > + parent()->setHasTextDescendants(); > + } This part was necessary to ensure that when the positioned object was added first, the root inline box was still aware of any subsequent text descendants such as the BR added as children to its first child.
Comment on attachment 120598 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120598&action=review r- because of the incomplete change log and because the fix for the hasTextDescendants() bug should be done separately and more efficiently. > Source/WebCore/ChangeLog:16 > + * rendering/RenderBlockLineLayout.cpp: > + (WebCore::RenderBlock::LineBreaker::skipLeadingWhitespace): > + The change log is incomplete: it doesn’t list the InlineFlowBox changes, the LineInfo changes, and the constructLine change. You should include detailed comments about each change. >> Source/WebCore/rendering/InlineFlowBox.h:200 >> + } > > This part was necessary to ensure that when the positioned object was added first, the root inline box was still aware of any subsequent text descendants such as the BR added as children to its first child. Good catch here, but this is a pre-existing issue and I think you should file a bug about it and fix it separately before proceeding with this bug. Here is a test case showing the bug: <span style="outline: 1px dotted blue;"> <span style="margin: 0 10px;"></span> text</span> When you fix that bug, you should do it more efficiently. First, use a loop instead of recursion. Secondly, check if the parent is already marked as having text descendants (the overwhelmingly common case), and if so, stop. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:205 > + void setPositionedObjectsInLeadingWhitespace(unsigned count) { m_positionedObjectsInLeadingWhitespace = count; } Since you’re only ever incrementing this by one, I’d define a function called incrementPositionedObjectsInLeadingWhitespace() that bumps the counter by one. No need for the caller to keep track of the count. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:488 > + int runCount = bidiRuns.runCount() - lineInfo.positionedObjectsInLeadingWhitespace(); You should hoist this line out of the loop. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1881 > + lineInfo.setPositionedObjectsInLeadingWhitespace(positionedObjects); Is it necessary to reset this here? See my earlier comments about how to deal with this counter.
Created attachment 120660 [details] Patch
Comment on attachment 120660 [details] Patch Attachment 120660 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11038590 New failing tests: fast/replaced/absolute-position-percentage-height.html fast/inline/styledEmptyInlinesWithBRs.html
(In reply to comment #24) > (From update of attachment 120660 [details]) > Attachment 120660 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/11038590 > > New failing tests: > fast/replaced/absolute-position-percentage-height.html > fast/inline/styledEmptyInlinesWithBRs.html This patch depends on the one at 75305 to pass these two.
Comment on attachment 120660 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120660&action=review This is getting really close so I am going to say r+ but I think you can improve it a little. > Source/WebCore/ChangeLog:17 > + (WebCore::LineInfo::positionedObjectsInLeadingWhitespace): On further thought, this is not the best name. It’s both inaccurate (positioned objects that were not originally inline aren’t counted) and indirect. A better name would be “runsFromLeadingWhitespace”. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1881 > + lineInfo.resetPositionedObjectsInLeadingWhitespace(); I think it makes more sense for this to be called by layoutRunsAndFloatsInRange() just after calling setEmpty().
Committed r103851: <http://trac.webkit.org/changeset/103851>
(In reply to comment #27) > Committed r103851: <http://trac.webkit.org/changeset/103851> This fix caused <https://bugs.webkit.org/show_bug.cgi?id=77737>.