Bug 84624

Summary: RenderBlock incorrectly calculates pref width when a replaced object follows a RenderInline with width
Product: WebKit Reporter: tbsmark86
Component: TablesAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: arpitabahuguna, eric, inferno, jchaffraix, leviw, robert, vijayan.bits, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows XP   
Attachments:
Description Flags
Minimal example
none
Patch
none
Patch
none
Patch
none
Patch none

Description tbsmark86 2012-04-23 13:16:31 PDT
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.
Comment 1 Arpita Bahuguna 2012-07-27 00:13:50 PDT
Created attachment 154859 [details]
Patch
Comment 2 Allan Sandfeld Jensen 2012-08-27 05:25:59 PDT
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?
Comment 3 Julien Chaffraix 2012-09-24 11:52:31 PDT
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)
Comment 4 Julien Chaffraix 2012-09-24 11:53:02 PDT
Leviw knows this code better and may have some comments.
Comment 5 Arpita Bahuguna 2012-09-25 23:53:34 PDT
Created attachment 165738 [details]
Patch
Comment 6 Arpita Bahuguna 2012-09-26 04:56:28 PDT
(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)
Comment 7 Levi Weintraub 2012-09-26 13:01:43 PDT
Updating the title since this isn't table-specific. The fix LGTM. The test case really belongs in fast/block (and not using tables).
Comment 8 Arpita Bahuguna 2012-09-28 04:08:38 PDT
Created attachment 166200 [details]
Patch
Comment 9 Arpita Bahuguna 2012-09-28 05:59:28 PDT
(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.
Comment 10 Levi Weintraub 2012-09-28 10:13:42 PDT
Comment on attachment 166200 [details]
Patch

Thank you for the fix! LGTM.
Comment 11 Levi Weintraub 2012-09-28 10:14:08 PDT
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 :(
Comment 12 Arpita Bahuguna 2012-09-29 04:16:40 PDT
Created attachment 166358 [details]
Patch
Comment 13 Build Bot 2012-09-29 04:42:52 PDT
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
Comment 14 Arpita Bahuguna 2012-09-29 05:07:16 PDT
(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.
Comment 15 WebKit Review Bot 2012-10-01 11:16:02 PDT
Comment on attachment 166358 [details]
Patch

Clearing flags on attachment: 166358

Committed r130057: <http://trac.webkit.org/changeset/130057>
Comment 16 WebKit Review Bot 2012-10-01 11:16:07 PDT
All reviewed patches have been landed.  Closing bug.