Bug 74723 – REGRESSION (r94492): Text is shifted to the right in some buttons in the Mac App Store

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

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

[Robert Hogan]

See https://bugs.webkit.org/show_bug.cgi?id=4860#c21

[Robert Hogan]

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.

[Robert Hogan]

Created attachment 119767 [details]
Patch

[Robert Hogan]

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

[Robert Hogan]

This is still wrong for some combinations of leading absolute and float elements with whitespace. I'm still looking at it.

[Julien Chaffraix]

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>&#x200b;<span></span> Lorem ipsum dolor sit amet</div>

Those 2 lines are exactly the same between the test and its reference. Is it intended?

[Robert Hogan]

Created attachment 120178 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 120178 [details]
Patch

Attachment 120178 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10993583

[Early Warning System Bot]

Comment on attachment 120178 [details]
Patch

Attachment 120178 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10993582

[Robert Hogan]

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.

[Robert Hogan]

Created attachment 120467 [details]
Patch

[Robert Hogan]

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

[Robert Hogan]

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

[Robert Hogan]

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.

[Robert Hogan]

Created attachment 120598 [details]
Patch

[Robert Hogan]

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.

[Robert Hogan]

Created attachment 120660 [details]
Patch

[WebKit Review Bot]

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

[Robert Hogan]

(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().

[Robert Hogan]

Committed r103851: <http://trac.webkit.org/changeset/103851>

[David Kilzer (:ddkilzer)]

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