RESOLVED FIXED 6047
:first-line text-decorations are not rendered
https://bugs.webkit.org/show_bug.cgi?id=6047
Summary :first-line text-decorations are not rendered
mitz
Reported 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.
Attachments
original testcase (651 bytes, text/html)
2005-12-12 08:28 PST, mitz
no flags
Patch to check if the RenderObject is a firstLineBlock (2.80 KB, patch)
2007-08-15 13:56 PDT, Vincent Ricard
hyatt: review-
Patch (including the testcase) (5.51 KB, patch)
2007-08-22 12:39 PDT, Vincent Ricard
no flags
Patch (including the testcase and the changelog) (7.43 KB, patch)
2007-08-22 12:57 PDT, Vincent Ricard
eric: review-
screenshot showing test failure after applying patch (20.55 KB, image/png)
2007-10-06 23:42 PDT, Eric Seidel (no email)
no flags
complete fix (for original test case) (7.03 KB, patch)
2007-10-21 17:21 PDT, Eric Seidel (no email)
no flags
updated test case (needs sanity check) (1.93 KB, text/html)
2007-10-21 20:03 PDT, Eric Seidel (no email)
no flags
updated test case (needs sanity check) (1.93 KB, text/html)
2007-10-21 20:24 PDT, Eric Seidel (no email)
no flags
Patch (58.07 KB, patch)
2012-05-07 11:01 PDT, Pravin D
no flags
Patch (115.23 KB, patch)
2012-05-07 14:09 PDT, Pravin D
no flags
Patch (96.27 KB, patch)
2012-05-07 15:07 PDT, Pravin D
no flags
mitz
Comment 1 2005-12-12 08:28:06 PST
Created attachment 5049 [details] original testcase
Vincent Ricard
Comment 2 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...
David Kilzer (:ddkilzer)
Comment 3 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
Dave Hyatt
Comment 4 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).
Dave Hyatt
Comment 5 2007-08-19 21:20:05 PDT
Also no need to say "this->" when you are inside a member function of that class already.
Vincent Ricard
Comment 6 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
Vincent Ricard
Comment 7 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 :)
Adam Roben (:aroben)
Comment 8 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.
Dave Hyatt
Comment 9 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.
Eric Seidel (no email)
Comment 10 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.
Eric Seidel (no email)
Comment 11 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.
Eric Seidel (no email)
Comment 12 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.
Eric Seidel (no email)
Comment 13 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.
Eric Seidel (no email)
Comment 14 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.
Eric Seidel (no email)
Comment 15 2007-10-21 20:03:41 PDT
Created attachment 16776 [details] updated test case (needs sanity check)
Eric Seidel (no email)
Comment 16 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.
Pravin D
Comment 17 2012-05-05 05:45:41 PDT
*** Bug 84988 has been marked as a duplicate of this bug. ***
Pravin D
Comment 18 2012-05-07 11:01:06 PDT
Eric Seidel (no email)
Comment 19 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.
Pravin D
Comment 20 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?
Eric Seidel (no email)
Comment 21 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.
Pravin D
Comment 22 2012-05-07 14:09:32 PDT
Pravin D
Comment 23 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..
Eric Seidel (no email)
Comment 24 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.
Pravin D
Comment 25 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 ?
Eric Seidel (no email)
Comment 26 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?
Pravin D
Comment 27 2012-05-07 15:07:41 PDT
Eric Seidel (no email)
Comment 28 2012-05-07 15:31:31 PDT
Comment on attachment 140599 [details] Patch LGTM.
WebKit Review Bot
Comment 29 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>
WebKit Review Bot
Comment 30 2012-05-07 16:39:04 PDT
All reviewed patches have been landed. Closing bug.
Ojan Vafai
Comment 31 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).
Eric Seidel (no email)
Comment 32 2012-05-07 19:56:42 PDT
Sorry, my bad.
Note You need to log in before you can comment on or make changes to this bug.