Bug 18926 - dt:after generate a line break. Boost documentation page renders poorly
Summary: dt:after generate a line break. Boost documentation page renders poorly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 525.x (Safari 3.1)
Hardware: PC Windows XP
: P2 Normal
Assignee: Nobody
URL: http://www.boost.org/doc/libs/1_35_0
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2008-05-07 11:15 PDT by jasneet
Modified: 2010-02-09 01:16 PST (History)
6 users (show)

See Also:


Attachments
screenshot (201.56 KB, image/jpeg)
2008-05-07 11:16 PDT, jasneet
no flags Details
reduction (413 bytes, text/html)
2008-05-07 11:16 PDT, jasneet
no flags Details
Suppress line breaks at pseudo element boundaries, i.e., after :before and before :after. (21.37 KB, patch)
2010-01-26 03:17 PST, Yuzo Fujishima
mitz: review-
eric: commit-queue-
Details | Formatted Diff | Diff
Do not assume that text is broken at pseudo/inline element boundaries, i.e., after :before, before :after, and before/after inline elements. (41.31 KB, patch)
2010-02-01 02:37 PST, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Do not assume that text is broken at pseudo/inline element boundaries, i.e., after :before, before :after, and before/after inline elements. (41.23 KB, patch)
2010-02-04 22:34 PST, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Do not assume that text is broken at pseudo/inline element boundaries, i.e., after :before, before :after, and before/after inline elements. (162.49 KB, patch)
2010-02-07 21:17 PST, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Disallow line breaks at inline element boundaries. (162.17 KB, patch)
2010-02-08 18:07 PST, Yuzo Fujishima
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jasneet 2008-05-07 11:15:48 PDT
I Steps:
Go to 
http://www.boost.org/doc/libs/1_35_0

II Issue:
The fields "Author(s), First Release, Standard, Build & Link" are not appearing in the correct place for each library.

III Conclusion:
Safari generates a line break for 'dt:after'. Hence the misalignment.

IV Other browsers:
IE7: not ok (not honoring 'dt:after')
FF3: ok
Opera9.24: ok

V Nightly tested: 32871
Comment 1 jasneet 2008-05-07 11:16:12 PDT
Created attachment 21002 [details]
screenshot
Comment 2 jasneet 2008-05-07 11:16:43 PDT
Created attachment 21003 [details]
reduction
Comment 3 mitz 2008-05-16 14:03:39 PDT
<rdar://problem/5938743>
Comment 4 David Harrison 2008-05-19 09:17:42 PDT
Using 33571, the results in boost.org and the reduction match Safari 3.1.1.
Comment 5 mitz 2008-05-25 19:49:35 PDT
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.
Comment 6 Yuzo Fujishima 2010-01-26 03:17:29 PST
Created attachment 47395 [details]
Suppress line breaks at pseudo element boundaries, i.e., after :before and before :after.
Comment 7 Yuzo Fujishima 2010-01-26 03:18:36 PST
Hi, reviewers,

Can you review the patch above?

Yuzo
Comment 8 Eric Seidel (no email) 2010-01-26 14:18:22 PST
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 9 mitz 2010-01-26 14:20:57 PST
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?
Comment 10 Yuzo Fujishima 2010-02-01 02:37:54 PST
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.
Comment 11 Yuzo Fujishima 2010-02-01 02:58:25 PST
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 12 mitz 2010-02-04 12:04:10 PST
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.
Comment 13 mitz 2010-02-04 12:05:14 PST
I would like Hyatt to take a look and say whether the “break between objects” case is ever needed.
Comment 14 Dave Hyatt 2010-02-04 12:11:03 PST
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.
Comment 15 Yuzo Fujishima 2010-02-04 22:34:51 PST
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 16 mitz 2010-02-04 22:37:18 PST
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.
Comment 17 Yuzo Fujishima 2010-02-07 21:17:06 PST
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.
Comment 18 Yuzo Fujishima 2010-02-07 21:19:59 PST
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 19 mitz 2010-02-08 10:08:34 PST
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.
Comment 20 Yuzo Fujishima 2010-02-08 18:07:02 PST
Created attachment 48378 [details]
Disallow line breaks at inline element boundaries.
Comment 21 Yuzo Fujishima 2010-02-08 18:09:00 PST
Hi, mitz,

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

Yuzo
Comment 22 Yuzo Fujishima 2010-02-09 01:16:55 PST
Committed r54536: <http://trac.webkit.org/changeset/54536>