Bug 6047

Created attachment 5049 [details] original testcase

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 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 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).

Also no need to say "this->" when you are inside a member function of that class already.

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

Created attachment 16081 [details] Patch (including the testcase and the changelog) Well, i hope the patch finally contains all required files :)

Comment on attachment 16081 [details] Patch (including the testcase and the changelog) Let's get hyatt to look at this.

Comment on attachment 16081 [details] Patch (including the testcase and the changelog) r=me, for landing on the feature branch.

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 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.

It also looks as though possibly other calls to getTextDecorationColors() should have the firstline bool set when called, but I'm not certain.

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 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.

Created attachment 16776 [details] updated test case (needs sanity check)

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.

*** Bug 84988 has been marked as a duplicate of this bug. ***

Created attachment 140554 [details] Patch

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.

(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?

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.

Created attachment 140586 [details] Patch

(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 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.

(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 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?

Created attachment 140599 [details] Patch

Comment on attachment 140599 [details] Patch LGTM.

Comment on attachment 140599 [details] Patch Clearing flags on attachment: 140599 Committed r116373: <http://trac.webkit.org/changeset/116373>

All reviewed patches have been landed. Closing bug.

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).

Sorry, my bad.