WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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 email)
no flags
Details
complete fix (for original test case)
(7.03 KB, patch)
2007-10-21 17:21 PDT
,
Eric Seidel (no email)
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 email)
no flags
Details
updated test case (needs sanity check)
(1.93 KB, text/html)
2007-10-21 20:24 PDT
,
Eric Seidel (no email)
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
Show Obsolete
(8)
View All
Add attachment
proposed patch, testcase, etc.
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
Created
attachment 140554
[details]
Patch
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
Created
attachment 140586
[details]
Patch
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
Created
attachment 140599
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug