Bug 99194 - Regression r130057: incorrect block pref width for alternating InlineFlow and inline Replaced
Summary: Regression r130057: incorrect block pref width for alternating InlineFlow and...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Levi Weintraub
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-12 11:48 PDT by Levi Weintraub
Modified: 2012-10-15 14:29 PDT (History)
11 users (show)

See Also:


Attachments
Patch (4.54 KB, patch)
2012-10-12 13:12 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Testcase.html (527 bytes, text/html)
2012-10-13 07:52 PDT, Arpita Bahuguna
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Levi Weintraub 2012-10-12 11:48:31 PDT
http://trac.webkit.org/changeset/130057 fixed block pref width calculation, but introduced a regression where alternating InlineFlows and inline Replaced elements will result in the prefs widths calculation never breaking the line.
Comment 1 Levi Weintraub 2012-10-12 13:12:24 PDT
Created attachment 168466 [details]
Patch
Comment 2 Levi Weintraub 2012-10-12 15:42:51 PDT
Upstream chromium bug http://code.google.com/p/chromium/issues/detail?id=154797
Comment 3 Arpita Bahuguna 2012-10-13 07:52:57 PDT
Created attachment 168552 [details]
Testcase.html

Apologize for not having verified all the scenarios and for causing this regression.
I tried to come up with similar combinations which might regress the previous fix and in the process found one more issue.

The case is when a RenderInline containing some text follows another RenderInline wrapping a RenderReplaced. For this case we are not breaking the line after the RenderInline containing the text. This causes it to display extra space after the line containing the RenderReplaced.
Have uploaded the testcase containing two variations of the same issue.
FF too does not seem to handle the second variation in the uploaded testcase.

This issue occurs even after applying this patch. I think I may have a fix for the same. Should this case too be included as part of this regression or shall I raise another bug for it?
Comment 4 Arpita Bahuguna 2012-10-15 08:00:16 PDT
Hey Levi,

I was trying to fix the issue mentioned in my previous comment (#3) which resulted in handling of some other failing scenarios (arising out of the fix for #3 issue) such as :

<div style="width: 50px">
<span style="float: left; border: 1px solid black;">
<span>FF doesn't handle this case.</span>Some text outside of span<span><img src="50x50.gif"/></span>
</span>
</div>

Wherein a text followed by another text and then a RenderReplaced (enclosed within a RenderInline) causes the text to overflow.

However, all these issues are independent of your patch.
 
Am trying to fix all these scenarios and if it's okay with you, will try to submit a patch for them by tomorrow.
Comment 5 Levi Weintraub 2012-10-15 10:04:05 PDT
(In reply to comment #4)
> Am trying to fix all these scenarios and if it's okay with you, will try to submit a patch for them by tomorrow.

Of course I welcome your patch. In the meantime, I'd love to get this patch reviewed or the original reverted as this is causing painful regressions.
Comment 6 Levi Weintraub 2012-10-15 14:11:07 PDT
Anyone? This is a definite improvement from current behavior. We didn't cover all the cases prior to r130057, and won't with this fix, but at least we'll correct this regression.
Comment 7 Eric Seidel (no email) 2012-10-15 14:16:33 PDT
Comment on attachment 168466 [details]
Patch

OK.
Comment 8 Levi Weintraub 2012-10-15 14:17:17 PDT
Comment on attachment 168466 [details]
Patch

Thanks Eric.
Comment 9 Levi Weintraub 2012-10-15 14:18:21 PDT
Arpita, please open a new bug and add your test case (and patch when you have it) to it.
Comment 10 WebKit Review Bot 2012-10-15 14:29:33 PDT
Comment on attachment 168466 [details]
Patch

Clearing flags on attachment: 168466

Committed r131359: <http://trac.webkit.org/changeset/131359>
Comment 11 WebKit Review Bot 2012-10-15 14:29:37 PDT
All reviewed patches have been landed.  Closing bug.