Bug 99442 – Regression r130057: Improper preferred width calculation when an inline replaced object, wrapped in an inline flow, follows some text.

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

[Arpita Bahuguna]

For scenarios such as:
1. InlineReplaced object followed by a Text object (wrapped in an InlineFlow object)
2. Text (wrapped in InlineFlow) followed by InlineReplaced objects
our preferred logical width is not being computed correctly (in a RenderBlock).

Uploading three test pages containing various scenarios that highlight the inconsistency in our block width's calculation.

1. InlineReplaced-Text.html - This contains various combinations for InlineReplaced objects followed by text objects (wrapped in InlineFlow).

2. Text-InlineReplaced.html - This is for Text objects followed by InlineReplaced objects and their various combinations.

3. InlineReplaced-InlineReplaced.html - For cases where InlineReplaced objects (wrapped in InlineFlow) are followed by other InlineReplaced objects.


FF handles most of these cases correctly, except for:

1. InlineReplaced followed by InlineReplaced wrapped in InlineFlow object.
2. Text wrapped in InlineFlow followed by InlineReplaced also wrapped in InlineFlow.
3. InlineReplaced (wrapped in InlineFlow) followed by Text.

Also, see https://bugs.webkit.org/show_bug.cgi?id=99194#c3 and https://bugs.webkit.org/show_bug.cgi?id=99194#c4.

[Arpita Bahuguna]

Created attachment 168900 [details]
InlineReplaced-InlineReplaced.html

[Arpita Bahuguna]

Created attachment 168901 [details]
InlineReplaced-Text.html

[Arpita Bahuguna]

Created attachment 168902 [details]
Text-InlineReplaced.html

[Arpita Bahuguna]

Created attachment 168911 [details]
Testfiles with the Image

[Arpita Bahuguna]

Created attachment 168941 [details]
Proposed patch

[Arpita Bahuguna]

Created attachment 168943 [details]
Proposed patch

[Arpita Bahuguna]

Hey Levi,

Have uploaded a patch for fixing the issue of the extra width occurring for the following scenario (and other similar scenarios):

<div style="width: 50px">
<span style="float: left; border: 1px solid black;">
<span>This is some text</span>
<span><img src="50x50.gif"/></span>
</span>
</div>

Previously, extra width was being carried forward from the Text object (endMin), via the inlineMin, onto the next line containing the InlineReplaced object.

For handling the same have added another flag (isPrevChildText) which keeps a track of whether the previous object was a Text object or not.
When this flag is set, the extra width carried forward (maintained in inlineMin) is reset and the line is broken at that point.

This now makes our rendering similar to that of FFs.

Albeit, there is one scenario for which even though we behave similar to other browsers (FF), it still seems incorrect in its rendering.
This is the scenario when we have an InlineReplaced object following a Text object.
<div style="width: 50px">
<span style="float: left; border: 1px solid black;">
<span>This is some text</span><span><img src="50x50.gif"/></span>
</span>
</div>

For the above case you will notice that the image overflows the parent block (after applying this patch).

Am unsure on how to proceed for this particular scenario; whether FF is right in its rendering approach or whether we should expand our parent block to contain the images as well.
Also, not sure about what the specification says for handling this scenario.

The uploaded patch does not have any layout testcases as I was not sure about the rendering behavior that we should follow for the above case.

Wish to get your opinion on the same.

[Arpita Bahuguna]

Modifying the title to reflect the precise issue.

[Arpita Bahuguna]

Created attachment 170838 [details]
Patch

[Build Bot]

Comment on attachment 170838 [details]
Patch

Attachment 170838 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14563961

New failing tests:
fast/block/block-with-inline-replaced-child-following-text.html

[Arpita Bahuguna]

Created attachment 171234 [details]
Patch

[Levi Weintraub]

Comment on attachment 171234 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171234&action=review

> Source/WebCore/ChangeLog:25
> +        Introduced another flag (isPrevChildText) which keeps a track of the
> +        previous object being a Text object or not. This flag is set when our current
> +        Text line (object) does not end in a whitespace, thereby implying that
> +        there could be further continuous text for which the end width needs to
> +        be carried forward onto the next line.

This variable name doesn't mean what its name suggests, which can lead to misunderstanding and misuse. Please try and come up with a name that better identifies its use. You actually contradict its meaning in this paragraph. r- for this, but otherwise the fix seems reasonable.

> LayoutTests/fast/block/block-with-inline-replaced-child-following-text.html:19
> +    description("Improper preferred logical width is computed for blocks with an inline replaced object following some text. The end width from the text object is carried forward to the next line containing the replaced obejct, thereby increasing its width.");

Typo: "obejct"

> LayoutTests/fast/block/block-with-inline-replaced-child-following-text.html:21
> +    shouldBe("getWidth('span1')", "'94px'");

I'm a little bit confused... where did the 94px size come from? Does this depend on font measurement?

> LayoutTests/fast/block/block-with-inline-replaced-child-following-text.html:33
> +.test {
> +    float: left;
> +}

Is this float part necessary?

[Arpita Bahuguna]

Created attachment 171600 [details]
Patch

[Arpita Bahuguna]

(In reply to comment #12)
> (From update of attachment 171234 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171234&action=review
> 
Thanks for the review Levi. Have uploaded another patch with the changes.

> > Source/WebCore/ChangeLog:25
> > +        Introduced another flag (isPrevChildText) which keeps a track of the
> > +        previous object being a Text object or not. This flag is set when our current
> > +        Text line (object) does not end in a whitespace, thereby implying that
> > +        there could be further continuous text for which the end width needs to
> > +        be carried forward onto the next line.
> 
> This variable name doesn't mean what its name suggests, which can lead to misunderstanding and misuse. Please try and come up with a name that better identifies its use. You actually contradict its meaning in this paragraph. r- for this, but otherwise the fix seems reasonable.
> 
Have changed the name of the variable to shouldBreakLineAfterText which should perhaps be more indicative of its usage. It would be set only when our text line doesn't end in a whitespace.

> > LayoutTests/fast/block/block-with-inline-replaced-child-following-text.html:19
> > +    description("Improper preferred logical width is computed for blocks with an inline replaced object following some text. The end width from the text object is carried forward to the next line containing the replaced obejct, thereby increasing its width.");
> 
> Typo: "obejct"
> 
Rectified

> > LayoutTests/fast/block/block-with-inline-replaced-child-following-text.html:21
> > +    shouldBe("getWidth('span1')", "'94px'");
> 
> I'm a little bit confused... where did the 94px size come from? Does this depend on font measurement?
> 
Yes, this is the size of our text which depends on the font. Have used ahem font to make it independent of platforms.
For my previous patch I'd tried to use a ref test, but that failed for mac showing a difference of 1 space character width. I need to investigate that further, it being an issue specific only to mac. On windows or Linux (checked for Qt and Chromium ports) I got same results.

> > LayoutTests/fast/block/block-with-inline-replaced-child-following-text.html:33
> > +.test {
> > +    float: left;
> > +}
> 
> Is this float part necessary?
It was in order to make our containing box (<span> element with class="test") block level. Have changed the same to a less cryptic, display: inline-block.
Since the specified width for this element is auto, it would thus compute its width making use of the preferred logical widths.

[Levi Weintraub]

Comment on attachment 171600 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171600&action=review

I'm happy to CQ+ this with a comment showing the math to get this 94px. Better still would be determining the correct width programmatically, as this is less prone to failure and "shows the math."

> Source/WebCore/rendering/RenderBlock.cpp:5795
> +    bool shouldBreakLineAfterText = false;

This is a much better name, thank you.

> LayoutTests/fast/block/block-with-inline-replaced-child-following-text.html:19
> +    description("Improper preferred logical width is computed for blocks with an inline replaced object following some text. The end width from the text object is carried forward to the next line containing the replaced object, thereby increasing its width.");

Nit: super long line.

> LayoutTests/fast/block/block-with-inline-replaced-child-following-text.html:23
> +    shouldBe("getWidth('div1')", "'94px'");
> +    shouldBe("getWidth('div2')", "'94px'");
> +    shouldBe("getWidth('div3')", "'94px'");

I'd like a comment about where this 94px comes from so someone looking at a test failure can quickly figure it out.

> LayoutTests/fast/block/block-with-inline-replaced-child-following-text.html:46
> +<div>Test for Bugzilla bug:<a href="https://bugs.webkit.org/show_bug.cgi?id=99442"> 99442</a> Regression r130057: Improper preferred width calculation when an inline replaced object, wrapped in an inline flow, follows some text.</div>

Nit: super long line.

[Eric Seidel (no email)]

This sounds a lot like bug 100943.

[Levi Weintraub]

(In reply to comment #16)
> This sounds a lot like bug 100943.


Thanks, you're totally right :)

[Levi Weintraub]

*** Bug 100943 has been marked as a duplicate of this bug. ***

[Arpita Bahuguna]

Created attachment 172016 [details]
Patch

[Arpita Bahuguna]

(In reply to comment #15)
> (From update of attachment 171600 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171600&action=review
> 
Thanks for the review Levi.

> > LayoutTests/fast/block/block-with-inline-replaced-child-following-text.html:19
> > +    description("Improper preferred logical width is computed for blocks with an inline replaced object following some text. The end width from the text object is carried forward to the next line containing the replaced object, thereby increasing its width.");
> 
> Nit: super long line.
> 
Have modified this description to only reflect (and that too briefly) on how we arrive at the 94px width.

> > LayoutTests/fast/block/block-with-inline-replaced-child-following-text.html:23
> > +    shouldBe("getWidth('div1')", "'94px'");
> > +    shouldBe("getWidth('div2')", "'94px'");
> > +    shouldBe("getWidth('div3')", "'94px'");
> 
> I'd like a comment about where this 94px comes from so someone looking at a test failure can quickly figure it out.
> 
The page itself contains a very brief comment (added in the description part) on how we get the 94px width. A more detailed description of the same has been added in the changelog though.

> > LayoutTests/fast/block/block-with-inline-replaced-child-following-text.html:46
> > +<div>Test for Bugzilla bug:<a href="https://bugs.webkit.org/show_bug.cgi?id=99442"> 99442</a> Regression r130057: Improper preferred width calculation when an inline replaced object, wrapped in an inline flow, follows some text.</div>
> 
> Nit: super long line.
This was the title of the bug. :( So have let it be as is. But if required, can perhaps change the title (along with this).

[Arpita Bahuguna]

(In reply to comment #18)
> *** Bug 100943 has been marked as a duplicate of this bug. ***

Hi Levi, Eric,
The issue (100943) is actually not really related to this particular fix.

The problem (in 100943) is not due to the extra width (from the text) being carried onto the next line, as is the case here, but because of our breaking the line when we encounter that one single space character, since it treats it as a beginWhiteSpace/endWhiteSpace, thereby causing our line to break.

I will try to add a more detailed description of the actual issue on the bug itself, after further study.

[Levi Weintraub]

Comment on attachment 172016 [details]
Patch

Ok.

[WebKit Review Bot]

Comment on attachment 172016 [details]
Patch

Clearing flags on attachment: 172016

Committed r133292: <http://trac.webkit.org/changeset/133292>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.