Bug 93829 - Text decorations specified on the containing block are not properly applied when ::first-line is present.
Summary: Text decorations specified on the containing block are not properly applied w...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Arpita Bahuguna
URL:
Keywords:
Depends on: 88580
Blocks: 91638
  Show dependency treegraph
 
Reported: 2012-08-13 05:31 PDT by Arpita Bahuguna
Modified: 2012-10-09 05:37 PDT (History)
8 users (show)

See Also:


Attachments
Testcase (886 bytes, text/html)
2012-08-13 05:31 PDT, Arpita Bahuguna
no flags Details
Patch (6.48 KB, patch)
2012-08-13 06:41 PDT, Arpita Bahuguna
no flags Details | Formatted Diff | Diff
Patch (6.86 KB, patch)
2012-10-08 05:28 PDT, Arpita Bahuguna
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Arpita Bahuguna 2012-08-13 05:31:38 PDT
Created attachment 157971 [details]
Testcase

Issue:
When a pseudo ::first-line is present, the text-decoration of it's containing block is not applied properly on the first-line content.

Several scenarios can occur, namely:

1. pseudo ::first-line has no text-decoration specified, only the containing block has. In such a case the containing block's text-decoration should be applied.
2. pseudo ::first-line too has text-decoration specified along with the containing block; but the styles are non-conflicting. In this case, both the styles should be applied.
3. pseudo ::first-line and the containing block both have the same text-decoration specified. In this case the text-decoration of the pseudo selector should be given precedence.

Currently, WebKit's behavior is not proper for scenarios 1 and 2 mentioned above.

Attached is a test-case for verifying the above scenarios.
Comment 1 Arpita Bahuguna 2012-08-13 06:41:20 PDT
Created attachment 157986 [details]
Patch
Comment 2 Arpita Bahuguna 2012-08-16 07:44:32 PDT
The uploaded patch intends to first compute the text decoration colors for the containing box, followed by that of the first-line (if specified).

This thus avoids the condition wherein our containing box's text-decorations were not getting applied at all since initially we were only computing for the first-line style.
Comment 3 Elliott Sprehn 2012-10-01 18:40:15 PDT
Comment on attachment 157986 [details]
Patch

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

This looks fine, but it'd be nice to have a better explanation. You'll also need someone like eseidel to r+ it.

> Source/WebCore/rendering/InlineTextBox.cpp:928
> +        renderer()->getTextDecorationColors(deco, underline, overline, linethrough, true, true);

Can you explain in the changelog what was really going on here? It's not clear what this does, or why we need to call it twice now (without and with first line)
Comment 4 Abhishek Arya 2012-10-01 18:50:20 PDT
Comment on attachment 157986 [details]
Patch

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

>> Source/WebCore/rendering/InlineTextBox.cpp:928
>> +        renderer()->getTextDecorationColors(deco, underline, overline, linethrough, true, true);
> 
> Can you explain in the changelog what was really going on here? It's not clear what this does, or why we need to call it twice now (without and with first line)

This looks like the only call site for getTextDecorationColors and you pass always true for its argument. This seems wrong to me. you should probably fix up getTextDecorationColors function itself and make it so that its style is taken into account and not just :first-line style. I think this part of the code is wrong since it only looks at first-line style
do {
        styleToUse = curr->style(firstlineStyle);
        int currDecs = styleToUse->textDecoration();
        if (currDecs) {


Your changelog description is just a copy of the title, so it needs more explanation.
Comment 5 Arpita Bahuguna 2012-10-08 05:28:26 PDT
Created attachment 167526 [details]
Patch
Comment 6 Arpita Bahuguna 2012-10-08 05:41:40 PDT
(In reply to comment #4)
> (From update of attachment 157986 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157986&action=review
> 
Thanks for the review Abhishek.

> This looks like the only call site for getTextDecorationColors and you pass always true for its argument. This seems wrong to me. you should probably fix up getTextDecorationColors function itself and make it so that its style is taken into account and not just :first-line style. I think this part of the code is wrong since it only looks at first-line style
> do {
>         styleToUse = curr->style(firstlineStyle);
>         int currDecs = styleToUse->textDecoration();
>         if (currDecs) {
> 

Let me explain a little about the fix with the below example:

<style>
div { text-decoration: underline;}
div:first-line { color: green; text-decoration: overline;} 
</style>

<div>First Line. First Line. First Line. <br/> Second Line. Second Line.</div>

The issue is that for the first-line in the above <div>, text-decoration: underline was not being applied.

The intention of the previous fix was that we first apply the <div>'s text decorations onto our line box, followed by the text decorations specified in the <div>'s first line style (if any).

getTextDecorationColors() has a default last argument - firstLineStyle = false.
In the above fix, the first call made is without the last argument and is thus meant for retrieving the <div>'s (container box's) text decorations.

On the other hand the second call is made to retrieve the first-line text decorations if and only if the line box is part of a first-line. In our case this would be the style text-decoration: overline for "first line... first line." text.

With this current approach, for the first call to getTextDecorationColors() the last argument (firstLineStyle) can perhaps be explicitly passed as false.

I tried to integrate both the functionalities into getTextDecorationColors() but it seemed to make the function messy and compromised on its readability.

Also, as part of cleanup, should the argument - quirksMode be removed altogether since it is always true.

Please let me know your opinion on the above and also on how to proceed with this further.

> 
> Your changelog description is just a copy of the title, so it needs more explanation.
Have uploaded another patch with a detailed changelog.
Comment 7 Arpita Bahuguna 2012-10-08 05:44:07 PDT
(In reply to comment #3)
> (From update of attachment 157986 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157986&action=review
> 
> This looks fine, but it'd be nice to have a better explanation. You'll also need someone like eseidel to r+ it.
> 
> > Source/WebCore/rendering/InlineTextBox.cpp:928
> > +        renderer()->getTextDecorationColors(deco, underline, overline, linethrough, true, true);
> 
> Can you explain in the changelog what was really going on here? It's not clear what this does, or why we need to call it twice now (without and with first line)
Thanks for reviewing Elliott.
Have uploaded another patch with a detailed explanation.
Comment 8 Abhishek Arya 2012-10-08 08:58:39 PDT
Comment on attachment 167526 [details]
Patch

Thanks for the explanation. r=me.
Comment 9 WebKit Review Bot 2012-10-08 17:52:06 PDT
Comment on attachment 167526 [details]
Patch

Rejecting attachment 167526 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
_party/angle --revision 1289 --non-interactive --force --accept theirs-conflict --ignore-externals returned non-zero exit status 1 in /mnt/git/webkit-commit-queue/Source/WebKit/chromium
Error: 'depot_tools/gclient sync --force --reset --delete_unversioned_trees' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 96.
Re-trying 'depot_tools/gclient sync --force --reset --delete_unversioned_trees'
Died at /mnt/git/webkit-commit-queue/Tools/Scripts/webkitdirs.pm line 2456.

Full output: http://queues.webkit.org/results/14229336
Comment 10 WebKit Review Bot 2012-10-09 04:42:06 PDT
Comment on attachment 167526 [details]
Patch

Clearing flags on attachment: 167526

Committed r130750: <http://trac.webkit.org/changeset/130750>
Comment 11 WebKit Review Bot 2012-10-09 04:42:10 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Arpita Bahuguna 2012-10-09 05:31:43 PDT
(In reply to comment #8)
> (From update of attachment 167526 [details])
> Thanks for the explanation. r=me.

Thankyou for your time for reviewing the patch Abhishek.
Comment 13 Arpita Bahuguna 2012-10-09 05:37:21 PDT
Patch has been landed.