Bug 18926

Created attachment 21002 [details] screenshot

Created attachment 21003 [details] reduction

<rdar://problem/5938743>

Using 33571, the results in boost.org and the reduction match Safari 3.1.1.

This is just what's now left of bug 7276 (see attachment 21018 [details]): WebKit allows a line break at the boundary between inlines where it most likely should not.

Created attachment 47395 [details] Suppress line breaks at pseudo element boundaries, i.e., after :before and before :after.

Hi, reviewers, Can you review the patch above? Yuzo

Comment on attachment 47395 [details] Suppress line breaks at pseudo element boundaries, i.e., after :before and before :after. Looks sane to me. This is going to cause test failures on the other bots. You will need to either provide results for those bots, or skip the test on those platforms with a note that the test just needs results landed.

Comment on attachment 47395 [details] Suppress line breaks at pseudo element boundaries, i.e., after :before and before :after. I don’t think this is correct. What makes before and after content need special treatment? Why is a change to findNextLineBreak not matched by a change to calcInlinePrefWidths()? Have you looked at bug 7276?

Created attachment 47822 [details] Do not assume that text is broken at pseudo/inline element boundaries, i.e., after :before, before :after, and before/after inline elements.

Hi, Eric, mitz, Thank you for the review. > What makes before and after content need special treatment? > Have you looked at bug 7276? I've updated the patch such that inline elements are treated in the same way as pseudo elements. > Why is a change to findNextLineBreak not matched by a change to calcInlinePrefWidths()? Please correct me if wrong, but RenderBlock::calcInlinePrefWidths() doesn't seem to treat pseudo/inline element boundaries breakable (unless whitespace is around there). I'll rebase or correct tests broken by this patch once this patch itself becomes good enough. Yuzo

Comment on attachment 47822 [details] Do not assume that text is broken at pseudo/inline element boundaries, i.e., after :before, before :after, and before/after inline elements. > + if (last != o && !last->isListMarker() && last->style()->styleType() != BEFORE && o->style()->styleType() != AFTER && !last->style()->isDisplayInlineType() && !o->style()->isDisplayInlineType()) { > + // better to break between object boundaries than in the middle of a word, > + // except for list markers, :before pseudo elements, :after pseudo elements, and inline elements. I don’t see why you have to single-out :before and :after at all. Presumably they are inlines too. But the more I think about this, the less I think this “break between object boundaries” case is ever the right thing to do. I think the handling of inline and replaced elements in the main loop now correctly identifies breaking opportunities, so this branch should never be taken.

I would like Hyatt to take a look and say whether the “break between objects” case is ever needed.

Comment on attachment 47822 [details] Do not assume that text is broken at pseudo/inline element boundaries, i.e., after :before, before :after, and before/after inline elements. I don't think this code is even needed at all.

Created attachment 48200 [details] Do not assume that text is broken at pseudo/inline element boundaries, i.e., after :before, before :after, and before/after inline elements.

Comment on attachment 48200 [details] Do not assume that text is broken at pseudo/inline element boundaries, i.e., after :before, before :after, and before/after inline elements. > - if (last != o && !last->isListMarker()) { > - // better to break between object boundaries than in the middle of a word (except for list markers) > + if (last != o && !last->isListMarker() && !last->style()->isDisplayInlineType() && !o->style()->isDisplayInlineType()) { > + // better to break between object boundaries than in the middle of a word, > + // except for list markers and inline elements including :before and :after pseudo elements. > lBreak.obj = o; > lBreak.pos = 0; > lBreak.nextBreakablePosition = -1; Have you tried just removing this entire branch? Hyatt and I think it’s completely unnecessary.

Created attachment 48313 [details] Do not assume that text is broken at pseudo/inline element boundaries, i.e., after :before, before :after, and before/after inline elements.

Hi, mitz, Can you take another look? I've removed the if branch and it works. I've rebaselined three tests which I believe necessary. Please see the change log for more detail. Yuzo

Comment on attachment 48313 [details] Do not assume that text is broken at pseudo/inline element boundaries, i.e., after :before, before :after, and before/after inline elements. > --- a/WebCore/ChangeLog > +++ b/WebCore/ChangeLog > @@ -1,3 +1,20 @@ > +2010-02-07 Yuzo Fujishima <yuzo@google.com> > + > + Reviewed by NOBODY (OOPS!). > + > + Do not assume that text is broken at pseudo/inline element boundaries, i.e., after :before, before :after, and before/after inline elements. > + Tests css2.1/t0803-c5504-imrgn-l-02-b-ag-expected.html and css2.1/t0804-c5509-ipadn-l-02-b-ag-expected.html have been rebaselined because the original expected values are incorrect, judging from the test description and how IE 8.0, Firefox 3.6, and Opera 10.10 render the pages. > + Test fast/dom/clone-node-dynamic-style-expected.html have been also rebaselined because the original expected values assumes a line break. > + Expected values under platform/qt need not be updated because the tests are in the Skipped file for the platform. > + https://bugs.webkit.org/show_bug.cgi?id=18926 > + https://bugs.webkit.org/show_bug.cgi?id=7276 > + > + Tests: fast/css/inline-element-line-break.html > + fast/css/pseudo-element-line-break.html > + > + * rendering/RenderBlockLineLayout.cpp: > + (WebCore::RenderBlock::findNextLineBreak): > + The code change looks fine, but I have to say that this is not a good change log entry. There is no need to mention the test result changes in the WebCore change log. It is wrong to call out pseudoelements, :before and :after—there is nothing in the bug or in the patch that is specific to those. I also don’t understand “do not assume that text is broken”—it doesn’t look like the old code was assuming that. Perhaps a better way to write this change log would be to start with the titles and URLs of the bugs you’re fixing, then list the tests you’ve added. Then you can explain what the root cause of the bug was: findNextLineBreak() unconditionally allowed lines to break between elements when no other line breaking opportunity had been found, but that was unnecessary and led to incorrect layout, and you changed the code to not allow those line breaks unconditionally. You can put that explanation before the changed methods list or as a description of the change to findNextLineBreak(). Marking r- because of the change log.

Created attachment 48378 [details] Disallow line breaks at inline element boundaries.

Hi, mitz, Thank you for the review. I've updated the change log as suggested. Can you take yet another look? Yuzo

Committed r54536: <http://trac.webkit.org/changeset/54536>