Bug 74723

Summary: REGRESSION (r94492): Text is shifted to the right in some buttons in the Mac App Store
Product: WebKit Reporter: mitz
Component: Layout and RenderingAssignee: Robert Hogan <robert>
Status: RESOLVED FIXED    
Severity: Normal CC: ddkilzer, dglazkov, hyatt, robert, webkit.review.bot
Priority: P1 Keywords: InRadar, Regression
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://itunes.apple.com/us/collection/apple-apps/id29520?fcId=489264329&mt=12
Bug Depends on: 75305    
Bug Blocks: 77737    
Attachments:
Description Flags
Patch
none
Test case with collapsible space after a leading positioned object
none
Patch
none
Patch
none
Patch
none
Test case with bidirectional reordering
none
Patch
none
Patch
none
Patch mitz: review+, webkit.review.bot: commit-queue-

Description mitz 2011-12-16 09:34:43 PST
<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.
Comment 1 Robert Hogan 2011-12-16 12:51:07 PST
See https://bugs.webkit.org/show_bug.cgi?id=4860#c21
Comment 2 Robert Hogan 2011-12-18 03:45:11 PST
Created attachment 119757 [details]
Patch
Comment 3 mitz 2011-12-18 08:14:10 PST
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.
Comment 4 mitz 2011-12-18 08:15:36 PST
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.
Comment 5 Robert Hogan 2011-12-18 10:14:19 PST
Created attachment 119767 [details]
Patch
Comment 6 Robert Hogan 2011-12-18 10:15:27 PST
(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.
Comment 7 Robert Hogan 2011-12-20 05:06:40 PST
This is still wrong for some combinations of leading absolute and float elements with whitespace. I'm still looking at it.
Comment 8 Julien Chaffraix 2011-12-20 06:20:21 PST
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?
Comment 9 Robert Hogan 2011-12-21 07:11:02 PST
Created attachment 120178 [details]
Patch
Comment 10 Gyuyoung Kim 2011-12-21 07:15:38 PST
Comment on attachment 120178 [details]
Patch

Attachment 120178 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10993583
Comment 11 Early Warning System Bot 2011-12-21 07:15:47 PST
Comment on attachment 120178 [details]
Patch

Attachment 120178 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10993582
Comment 12 Robert Hogan 2011-12-21 07:31:58 PST
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.
Comment 13 Robert Hogan 2011-12-23 08:26:18 PST
Created attachment 120467 [details]
Patch
Comment 14 Robert Hogan 2011-12-23 08:30:44 PST
(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.
Comment 15 mitz 2011-12-24 16:35:03 PST
Created attachment 120505 [details]
Test case with bidirectional reordering
Comment 16 mitz 2011-12-24 16:40:41 PST
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.
Comment 17 Robert Hogan 2011-12-25 04:07:02 PST
(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.
Comment 18 Robert Hogan 2011-12-26 12:08:41 PST
Created attachment 120558 [details]
Patch
Comment 19 mitz 2011-12-26 13:59:30 PST
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.
Comment 20 Robert Hogan 2011-12-27 10:00:43 PST
Created attachment 120598 [details]
Patch
Comment 21 Robert Hogan 2011-12-27 10:03:07 PST
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 22 mitz 2011-12-27 11:02:32 PST
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.
Comment 23 Robert Hogan 2011-12-28 05:56:07 PST
Created attachment 120660 [details]
Patch
Comment 24 WebKit Review Bot 2011-12-28 06:44:47 PST
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
Comment 25 Robert Hogan 2011-12-28 07:06:11 PST
(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 26 mitz 2011-12-29 14:03:26 PST
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().
Comment 27 Robert Hogan 2011-12-30 12:59:12 PST
Committed r103851: <http://trac.webkit.org/changeset/103851>
Comment 28 David Kilzer (:ddkilzer) 2012-02-03 07:40:26 PST
(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>.