Bug 84624 – RenderBlock incorrectly calculates pref width when a replaced object follows a RenderInline with width

• Status: RESOLVED
• Resolution: FIXED
• Component: Tables

Created attachment 138408 [details]
Minimal example

See attached example.
In the current nightly build: The img from the first cell overlap into the second cell.

[Arpita Bahuguna]

Created attachment 154859 [details]
Patch

[Allan Sandfeld Jensen]

Comment on attachment 154859 [details]
Patch

As non-reviewer review, this looks good to me, and I can confirm this matches the result in other browsers.
Julien, what do you think?

[Julien Chaffraix]

Comment on attachment 154859 [details]
Patch

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

> Julien, what do you think?

This patch will impact any RenderBlocks and not just RenderTableCells. The ChangeLog seems to suggest that this is table cell specific so it should be clarified.

> Source/WebCore/rendering/RenderBlock.cpp:5872
> +                isPrevChildInlineFlow = false;

Interestingly, we don't set isPrevChildInlineFlow in the else branch below which looks suspicious. Ideally you want to update |isPrevChildInlineFlow| for every iteration so it should probably be pushed to the end of the while loop (outside of any branch to garantee that it is updated)

[Julien Chaffraix]

Leviw knows this code better and may have some comments.

[Arpita Bahuguna]

Created attachment 165738 [details]
Patch

[Arpita Bahuguna]

(In reply to comment #3)
> (From update of attachment 154859 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154859&action=review
>
Thanks Julien for reviewing the patch. Have made changes as per your comments.
I agree, it would be safer to update |isPrevChildInlineFlow| at the end of the while loop. Have modified the changelog as well.

Shall ping Leviw for further reviews.

> > Julien, what do you think?
> 
> This patch will impact any RenderBlocks and not just RenderTableCells. The ChangeLog seems to suggest that this is table cell specific so it should be clarified.
> 
> > Source/WebCore/rendering/RenderBlock.cpp:5872
> > +                isPrevChildInlineFlow = false;
> 
> Interestingly, we don't set isPrevChildInlineFlow in the else branch below which looks suspicious. Ideally you want to update |isPrevChildInlineFlow| for every iteration so it should probably be pushed to the end of the while loop (outside of any branch to garantee that it is updated)

[Levi Weintraub]

Updating the title since this isn't table-specific. The fix LGTM. The test case really belongs in fast/block (and not using tables).

[Arpita Bahuguna]

Created attachment 166200 [details]
Patch

[Arpita Bahuguna]

(In reply to comment #7)
> Updating the title since this isn't table-specific. The fix LGTM. The test case really belongs in fast/block (and not using tables).

Thanks for the review Levi. Have moved the testcase to fast/block and removed the dependency on tables as well.

[Levi Weintraub]

Comment on attachment 166200 [details]
Patch

Thank you for the fix! LGTM.

[Levi Weintraub]

Comment on attachment 166200 [details]
Patch

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

> LayoutTests/ChangeLog:8
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

Whoops... missed this :(

[Arpita Bahuguna]

Created attachment 166358 [details]
Patch

[Build Bot]

Comment on attachment 166358 [details]
Patch

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

New failing tests:
http/tests/workers/terminate-during-sync-operation.html

[Arpita Bahuguna]

(In reply to comment #11)
> (From update of attachment 166200 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166200&action=review
> 
> > LayoutTests/ChangeLog:8
> > +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).
> 
> Whoops... missed this :(

Thanks for the review Levi and apologize for missing out on removing that line from the changelog.

Have uploaded another patch now, albeit it fails for mac port this time. :(
The failing test case though seems to be unrelated to my changes; it verifies web workers.

[WebKit Review Bot]

Comment on attachment 166358 [details]
Patch

Clearing flags on attachment: 166358

Committed r130057: <http://trac.webkit.org/changeset/130057>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.