Bug 6047 - :first-line text-decorations are not rendered
Summary: :first-line text-decorations are not rendered
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 420+
Hardware: All All
: P2 Normal
Assignee: Pravin D
URL:
Keywords: HasReduction
: 84988 (view as bug list)
Depends on:
Blocks: 34753
  Show dependency treegraph
 
Reported: 2005-12-12 08:26 PST by mitz
Modified: 2012-05-23 11:02 PDT (History)
8 users (show)

See Also:


Attachments
original testcase (651 bytes, text/html)
2005-12-12 08:28 PST, mitz
no flags Details
Patch to check if the RenderObject is a firstLineBlock (2.80 KB, patch)
2007-08-15 13:56 PDT, Vincent Ricard
hyatt: review-
Details | Formatted Diff | Diff
Patch (including the testcase) (5.51 KB, patch)
2007-08-22 12:39 PDT, Vincent Ricard
no flags Details | Formatted Diff | Diff
Patch (including the testcase and the changelog) (7.43 KB, patch)
2007-08-22 12:57 PDT, Vincent Ricard
eric: review-
Details | Formatted Diff | Diff
screenshot showing test failure after applying patch (20.55 KB, image/png)
2007-10-06 23:42 PDT, Eric Seidel
no flags Details
complete fix (for original test case) (7.03 KB, patch)
2007-10-21 17:21 PDT, Eric Seidel
no flags Details | Formatted Diff | Diff
updated test case (needs sanity check) (1.93 KB, text/html)
2007-10-21 20:03 PDT, Eric Seidel
no flags Details
updated test case (needs sanity check) (1.93 KB, text/html)
2007-10-21 20:24 PDT, Eric Seidel
no flags Details
Patch (58.07 KB, patch)
2012-05-07 11:01 PDT, Pravin D
no flags Details | Formatted Diff | Diff
Patch (115.23 KB, patch)
2012-05-07 14:09 PDT, Pravin D
no flags Details | Formatted Diff | Diff
Patch (96.27 KB, patch)
2012-05-07 15:07 PDT, Pravin D
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2005-12-12 08:26:57 PST
Text-decorations specified for :first-line are not rendered. Instead, the first line is rendered with the text-
decorations of the latter lines (if they have any).

To reproduce: open the testcase.

Expected result: described in the testcase, based on Firefox 1.5 and MacIE(!).

Actual result: The first green line has no underline, the second green line has a black underline. The third 
green line renders as expected, with a black underline.

Perhaps getTextDecorationColors() should take a firstLine flag.
Comment 1 mitz 2005-12-12 08:28:06 PST
Created attachment 5049 [details]
original testcase
Comment 2 Vincent Ricard 2007-08-15 13:56:22 PDT
Created attachment 15988 [details]
Patch to check if the RenderObject is a firstLineBlock

RenderObject::getTextDecorationColors does not check if the current RenderObject stands for a firstLineBlock.
If my patch is ok, maybe the do/while loop should be convert into a normal while loop since 'decorations' has already been modified before the loop.
I failed to run the tests on my linux box, so if someone can do it for me...
Comment 3 David Kilzer (:ddkilzer) 2007-08-16 06:40:31 PDT
Comment on attachment 15988 [details]
Patch to check if the RenderObject is a firstLineBlock

Thanks for the patch, Vincent!

Please set the "review?" flag on patches in the future so that they may be found and reviewed by Bugzilla queries.

I am not qualified to review the correctness of this patch, but you will need to include a ChangeLog and a layout test with this patch before it lands.  Details are here:

http://webkit.org/coding/contributing.html
Comment 4 Dave Hyatt 2007-08-19 21:05:55 PDT
Comment on attachment 15988 [details]
Patch to check if the RenderObject is a firstLineBlock

Need a test case that illustrates the problem as part of the patch.

Rename "firstMode" to "firstLine".

You don't need to duplicate all the bit checking.  Just grab the right style once at the top using style(firstLine).
Comment 5 Dave Hyatt 2007-08-19 21:20:05 PDT
Also no need to say "this->" when you are inside a member function of that class already.
Comment 6 Vincent Ricard 2007-08-22 12:39:20 PDT
Created attachment 16076 [details]
Patch (including the testcase)

This patch replaces the previous one:
- i added the testcase (and the corresponding "expected" file).
- i applied the corrections asked by Dave
Comment 7 Vincent Ricard 2007-08-22 12:57:10 PDT
Created attachment 16081 [details]
Patch (including the testcase and the changelog)

Well, i hope the patch finally contains all required files :)
Comment 8 Adam Roben (:aroben) 2007-09-28 12:18:09 PDT
Comment on attachment 16081 [details]
Patch (including the testcase and the changelog)

Let's get hyatt to look at this.
Comment 9 Dave Hyatt 2007-10-01 14:47:48 PDT
Comment on attachment 16081 [details]
Patch (including the testcase and the changelog)

r=me, for landing on the feature branch.
Comment 10 Eric Seidel 2007-10-06 23:42:12 PDT
Created attachment 16571 [details]
screenshot showing test failure after applying patch

I would land this, except your test case seems to fail for me when I apply the patch and view your test.
Comment 11 Eric Seidel 2007-10-06 23:42:42 PDT
Comment on attachment 16081 [details]
Patch (including the testcase and the changelog)

Marking r- since the attached test case seems to fail for me (on feature branch) with this patch applied.
Comment 12 Eric Seidel 2007-10-06 23:51:42 PDT
It also looks as though possibly other calls to getTextDecorationColors() should have the firstline bool set when called, but I'm not certain.
Comment 13 Eric Seidel 2007-10-21 17:21:58 PDT
Created attachment 16774 [details]
complete fix (for original test case)

Ok, so the previous patch didn't work correctly on the last test case (inheriting text decoration color from non-first-line style).  I also added two test cases to the attached patch (one which doesn't function as I expected in either FF or Safari).  Hopefully this is fully correct now.
Comment 14 Eric Seidel 2007-10-21 18:38:02 PDT
Comment on attachment 16774 [details]
complete fix (for original test case)

The test case has a bug, and I think this patch can be made more efficient. I'll post a new one in a bit.
Comment 15 Eric Seidel 2007-10-21 20:03:41 PDT
Created attachment 16776 [details]
updated test case (needs sanity check)
Comment 16 Eric Seidel 2007-10-21 20:24:54 PDT
Created attachment 16778 [details]
updated test case (needs sanity check)

Mitz found a typo in the last test case.  I'm still not quite clear on how text-decoration should "inherit" in two of these tests.

The spec says:
http://www.w3.org/TR/REC-CSS2/text.html#lining-striking-props
This property is not inherited, but descendant boxes of a block box should be formatted with the same decoration (e.g., they should all be underlined). The color of decorations should remain the same even if descendant elements have different 'color' values.
Comment 17 Pravin D 2012-05-05 05:45:41 PDT
*** Bug 84988 has been marked as a duplicate of this bug. ***
Comment 18 Pravin D 2012-05-07 11:01:06 PDT
Created attachment 140554 [details]
Patch
Comment 19 Eric Seidel 2012-05-07 11:15:17 PDT
Comment on attachment 140554 [details]
Patch

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

This looks good.  I think it could use one more test though.

> Source/WebCore/rendering/RenderObject.cpp:2592
>      do {
> -        int currDecs = curr->style()->textDecoration();
> +        styleToUse = curr->style(firstlineStyle);

It seems we need a test for this loop.  This loop looks like it's crawling up through the parent chain.  You've made it always ask for first-line colors for that chain, and we just want to test to amke sure we are doing that irhgt.
Comment 20 Pravin D 2012-05-07 11:30:04 PDT
(In reply to comment #19)
> (From update of attachment 140554 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140554&action=review
> 
> This looks good.  I think it could use one more test though.
> 
> > Source/WebCore/rendering/RenderObject.cpp:2592
> >      do {
> > -        int currDecs = curr->style()->textDecoration();
> > +        styleToUse = curr->style(firstlineStyle);
> 
>  You've made it always ask for first-line colors for that chain, and we just want to test to amke sure we are doing that irhgt.

I'm sorry, not able to get properly comprehend on what you mean by "make sure we are doing that right". If you don't mind , can kindly explain?
Comment 21 Eric Seidel 2012-05-07 11:33:23 PDT
I would just like to see a test which demonstrates that you understand what that code does, and that we didn't break it. :)

My understanding is that the code is attempting to inherit colors from parent elements of the styled text.  Since those colors could be different for first-line vs. not, seems we've fixed a "second" bug there, and we should test that bug.
Comment 22 Pravin D 2012-05-07 14:09:32 PDT
Created attachment 140586 [details]
Patch
Comment 23 Pravin D 2012-05-07 14:16:46 PDT
(In reply to comment #21)
> I would just like to see a test which demonstrates that you understand what that code does, and that we didn't break it. :)
> 
> My understanding is that the code is attempting to inherit colors from parent elements of the styled text.  Since those colors could be different for first-line vs. not, seems we've fixed a "second" bug there, and we should test that bug.

I have added another test case. Maybe it requires more verbose in it , but thought to keep the verbose out as it was just a test case. 
The new test case does the following
<div>
<p>
1st line Some text -- 1st line  ---- this text has deco and Color from 1st-line div selector.
Some more text -- second line -- this text has no deco and Color from <P>
</p>
Some div text -- this text has no deco and Color from <div> selector
<div>

So test case tests that the decoration color is not the same color as the last line (i.e Parent style vs parent 1st-line style).
I hope my understand is right about the test case req..
Comment 24 Eric Seidel 2012-05-07 14:17:36 PDT
Comment on attachment 140586 [details]
Patch

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

> LayoutTests/fast/css/first-line-text-decoration-inherited-from-parent.html:9
> +                font: 24px sans-serif;
> +                color: gray;
> +                letter-spacing: 5px;

We could share a bunch fo these styles using a class no?

> LayoutTests/fast/css/first-line-text-decoration-inherited-from-parent.html:58
> +        <p> Text-decoration UNDERLINE : Only the first line must have an underline.

Looks like this text is now wrong in this new test.
Comment 25 Pravin D 2012-05-07 14:21:09 PDT
(In reply to comment #24)
> (From update of attachment 140586 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140586&action=review
> 
> > LayoutTests/fast/css/first-line-text-decoration-inherited-from-parent.html:9
> > +                font: 24px sans-serif;
> > +                color: gray;
> > +                letter-spacing: 5px;
> 
> We could share a bunch fo these styles using a class no?
> 
> > LayoutTests/fast/css/first-line-text-decoration-inherited-from-parent.html:58
> > +        <p> Text-decoration UNDERLINE : Only the first line must have an underline.
> 
> Looks like this text is now wrong in this new test.

I guess :( ... I'll do the changes n upload another patch... but apart from these, is there any other changes that is req ?
Comment 26 Eric Seidel 2012-05-07 14:27:31 PDT
Comment on attachment 140586 [details]
Patch

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

Otherwise things look great.  Thanks for the tests.

> LayoutTests/fast/css/first-line-text-decoration-inherited-from-parent.html:52
> +            
> +            
> +            

Nit: lots of extra whitespace?
Comment 27 Pravin D 2012-05-07 15:07:41 PDT
Created attachment 140599 [details]
Patch
Comment 28 Eric Seidel 2012-05-07 15:31:31 PDT
Comment on attachment 140599 [details]
Patch

LGTM.
Comment 29 WebKit Review Bot 2012-05-07 16:38:55 PDT
Comment on attachment 140599 [details]
Patch

Clearing flags on attachment: 140599

Committed r116373: <http://trac.webkit.org/changeset/116373>
Comment 30 WebKit Review Bot 2012-05-07 16:39:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 31 Ojan Vafai 2012-05-07 19:55:15 PDT
For future reference, this test would have been better written as a reftest (and could still be rewritten :)). As it is, each platform needs it's own expectations due to platform-specific text rendering (e.g. I just committed http://trac.webkit.org/changeset/116386 with 6 results just for the Chromium ports).
Comment 32 Eric Seidel 2012-05-07 19:56:42 PDT
Sorry, my bad.