Bug 86517 – text-decoration should not be propagated through absolutely positioned elements to <a> tags

• Component: CSS
• Status: RESOLVED FIXED

[Marc]

Overview:
Bug 18611 solved the issue that text-decoration was propagating to floating and absolutely positioned elements.

However text-decoration is still propagated to <a> tags inside of floating and absolutely positioned elements.

Other elements don't seem effected, though testing was not extensive.


Steps to Reproduce: 
1) View http://dabblet.com/gist/2704820

Actual Results: 
The anchor text in the div is underlined.

Expected Results: 
None of the anchor text is underlined.

Build Date & Platform: 
Lated build tested was Chrome Canary 21.0.1137.1 on Mac, which I believe is WebKit 537.1.

[Shane Stephens]

Created attachment 144459 [details]
Patch

[WebKit Review Bot]

Comment on attachment 144459 [details]
Patch

Attachment 144459 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12852352

New failing tests:
fast/text/trailing-white-space.html
fast/text/trailing-white-space-2.html
fast/ruby/text-decoration-in-descendants-ruby.html
fast/text/decorations-transformed.html

[WebKit Review Bot]

Created attachment 144466 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Darin Adler]

Comment on attachment 144459 [details]
Patch

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

> Source/WebCore/rendering/style/RenderStyle.h:615
> -    ETextDecoration textDecorationsInEffect() const { return static_cast<ETextDecoration>(inherited_flags._text_decorations); }
> +    ETextDecoration textDecorationsInEffect() const
> +    {
> +        if (isFloating() || isPositioned())
> +            return TDNONE;
> +        return static_cast<ETextDecoration>(inherited_flags._text_decorations);
> +    }

This is not conventional layering for the code. All other getters in RenderStyle are pure data accessors. Putting logic here makes this different from the rest of the class in a way that can lead to problems later on.

I’d like to hear from some style experts where they think the right level for this fix is. Should it be in RenderStyle itself at all? Can the whole fix be done in the inheritFrom function?

I’m not sure the logic here is 100% right. This tells us only that the style requests that this be floating or positioned. It does not tell us whether the renderer actually respects that style setting. There are perhaps some elements where the position and float style properties don’t take effect. For those, this may be wrong behavior.

[Shane Stephens]

Created attachment 144699 [details]
Patch

[Shane Stephens]

Thanks for your comments. The new patch takes a different approach which seems to be more oriented towards the way other style inheritance checks are performed.

The new patch does not address your concerns about whether the logic is 100% correct, so I have also asked Tab Atkins to review whether there are circumstances where the behavior could be incorrect.

[Darin Adler]

Comment on attachment 144699 [details]
Patch

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

> Source/WebCore/css/StyleResolver.cpp:2067
>      // Finally update our text decorations in effect, but don't allow text-decoration to percolate through
>      // tables, inline blocks, inline tables, run-ins, or shadow DOM.

The comment no longer matches the code below. That needs to be fixed.

A situation like this shows why WebKit practice is to not write a comment that says the same thing as the code below it. If the comment was instead about why we don’t want text decoration to propagate, it would be more likely to still be correct. A comment that says the same thing as the code has to be updated every time the code is updated.

> Source/WebCore/css/StyleResolver.cpp:2070
>      if (style->display() == TABLE || style->display() == INLINE_TABLE || style->display() == RUN_IN
> -        || style->display() == INLINE_BLOCK || style->display() == INLINE_BOX || isAtShadowBoundary(e))
> +        || style->display() == INLINE_BLOCK || style->display() == INLINE_BOX || isAtShadowBoundary(e)
> +        || style->isFloating() || style->isPositioned())

With this expression getting more and more complicated, it seems clear we should factor this predicate out into a function. The name of that function would help document what’s going on here, and the body of the function might be a cleaner place to write comments about why we need to not do this for various cases.

[Tab Atkins Jr.]

(In reply to comment #7)
> (From update of attachment 144699 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144699&action=review
> 
> > Source/WebCore/css/StyleResolver.cpp:2067
> >      // Finally update our text decorations in effect, but don't allow text-decoration to percolate through
> >      // tables, inline blocks, inline tables, run-ins, or shadow DOM.
> 
> The comment no longer matches the code below. That needs to be fixed.
> 
> A situation like this shows why WebKit practice is to not write a comment that says the same thing as the code below it. If the comment was instead about why we don’t want text decoration to propagate, it would be more likely to still be correct. A comment that says the same thing as the code has to be updated every time the code is updated.
> 
> > Source/WebCore/css/StyleResolver.cpp:2070
> >      if (style->display() == TABLE || style->display() == INLINE_TABLE || style->display() == RUN_IN
> > -        || style->display() == INLINE_BLOCK || style->display() == INLINE_BOX || isAtShadowBoundary(e))
> > +        || style->display() == INLINE_BLOCK || style->display() == INLINE_BOX || isAtShadowBoundary(e)
> > +        || style->isFloating() || style->isPositioned())
> 
> With this expression getting more and more complicated, it seems clear we should factor this predicate out into a function. The name of that function would help document what’s going on here, and the body of the function might be a cleaner place to write comments about why we need to not do this for various cases.

The general rule (unfortunately not stated as such in CSS, but now that I've pointed it out, fantasai agreed to make it explicit) is that text-decorations propagate to descendants in the same inline formatting context.  In other words, solely things that are "display:inline".

(The model this is trying to implement is that text-decorations get set on the *line boxes* of an element, and thus are drawn under any children that share in the element's line boxes.  This makes it so that establishing a decoration once keeps it stable (in thickness and color) regardless of what the children do.)

The 'display' types previously listed all do so, as does floating or absposing (since they force your 'display' to compute to a block-level type).

[Shane Stephens]

Created attachment 144988 [details]
Patch

[Mike Lawther]

Comment on attachment 144988 [details]
Patch

cq+'ing based on darin's earlier r+

[WebKit Review Bot]

Comment on attachment 144988 [details]
Patch

Rejecting attachment 144988 [details] from commit-queue.

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

ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/12862183

[Shane Stephens]

Created attachment 144998 [details]
Patch

[WebKit Review Bot]

Comment on attachment 144998 [details]
Patch

Clearing flags on attachment: 144998

Committed r119065: <http://trac.webkit.org/changeset/119065>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.