Bug 93829 – Text decorations specified on the containing block are not properly applied when ::first-line is present.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Arpita Bahuguna]

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.

[Arpita Bahuguna]

Created attachment 157986 [details]
Patch

[Arpita Bahuguna]

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.

[Elliott Sprehn]

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)

[Abhishek Arya]

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.

[Arpita Bahuguna]

Created attachment 167526 [details]
Patch

[Arpita Bahuguna]

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

[Arpita Bahuguna]

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

[Abhishek Arya]

Comment on attachment 167526 [details]
Patch

Thanks for the explanation. r=me.

[WebKit Review Bot]

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

[WebKit Review Bot]

Comment on attachment 167526 [details]
Patch

Clearing flags on attachment: 167526

Committed r130750: <http://trac.webkit.org/changeset/130750>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Arpita Bahuguna]

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

[Arpita Bahuguna]

Patch has been landed.