Bug 226724 - `text-decoration: underline` is not applied to web component
Summary: `text-decoration: underline` is not applied to web component
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 148695
  Show dependency treegraph
 
Reported: 2021-06-07 04:34 PDT by Jeroen Zwartepoorte
Modified: 2021-06-08 05:11 PDT (History)
4 users (show)

See Also:


Attachments
patch (5.62 KB, patch)
2021-06-08 01:46 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (5.65 KB, patch)
2021-06-08 01:49 PDT, Antti Koivisto
rniwa: review+
Details | Formatted Diff | Diff
patch (5.76 KB, patch)
2021-06-08 02:36 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (5.76 KB, patch)
2021-06-08 02:57 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeroen Zwartepoorte 2021-06-07 04:34:04 PDT
See https://codepen.io/jpzwarte/pen/VwpdPqJ?editors=1010

This rule is not working in Safari (14.1.1 and STP125):

      :host([fill]:hover) {
        text-decoration: underline;
      }

It works fine in Chrome and FF.
Comment 1 Jeroen Zwartepoorte 2021-06-07 04:55:06 PDT
Another example, even more simple: https://codepen.io/Westbrook/pen/dyvKvXP?editors=1010
Comment 2 Radar WebKit Bug Importer 2021-06-08 00:08:23 PDT
<rdar://problem/78987286>
Comment 3 Antti Koivisto 2021-06-08 00:27:13 PDT
Looks like effective text decoration don't pass correctly to shadow tree.
Comment 4 Antti Koivisto 2021-06-08 01:46:07 PDT
Created attachment 430818 [details]
patch
Comment 5 Antti Koivisto 2021-06-08 01:49:52 PDT
Created attachment 430819 [details]
patch
Comment 6 Ryosuke Niwa 2021-06-08 02:20:00 PDT
Comment on attachment 430819 [details]
patch

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

> Source/WebCore/ChangeLog:12
> +        Test case by Jeroen Zwartepoorte.

Maybe put this in the change log for the layout test?

> Source/WebCore/style/StyleAdjuster.cpp:154
> +    auto isAtUserAgentShadowBoundary = [&] {
> +        if (!element)
> +            return false;
> +        auto* parentNode = element->parentNode();
> +        return parentNode && parentNode->isUserAgentShadowRoot();
> +    }();

Hm... I feel like we should be checking the slot assignment here
but I suppose we don't let a node inside a UA shadow root assigned to an author defined shadow tree,
and we don't really set text decoration within UA shadow tree so it's probably okay.
It might be still good to add a comment in the code explaining why this is okay though.

> Source/WebCore/style/StyleAdjuster.cpp:166
> +    default:
> +        return true;

I think putting this return true outside switch would read better.
Comment 7 Antti Koivisto 2021-06-08 02:31:03 PDT
> Hm... I feel like we should be checking the slot assignment here
> but I suppose we don't let a node inside a UA shadow root assigned to an
> author defined shadow tree,
> and we don't really set text decoration within UA shadow tree so it's
> probably okay.
> It might be still good to add a comment in the code explaining why this is
> okay though.

I don't think we should really have this test at all. UA shadow trees that don't want to inherit the effective text-decoration should simply set it themselves. That is a riskier change though and we probably don't have good test coverage.
Comment 8 Ryosuke Niwa 2021-06-08 02:34:15 PDT
(In reply to Antti Koivisto from comment #7)
> > Hm... I feel like we should be checking the slot assignment here
> > but I suppose we don't let a node inside a UA shadow root assigned to an
> > author defined shadow tree,
> > and we don't really set text decoration within UA shadow tree so it's
> > probably okay.
> > It might be still good to add a comment in the code explaining why this is
> > okay though.
> 
> I don't think we should really have this test at all. UA shadow trees that
> don't want to inherit the effective text-decoration should simply set it
> themselves. That is a riskier change though and we probably don't have good
> test coverage.

What do you mean by "simply set it"? It's not possible to get rid of text decorations, right? -webkit-text-decorations-in-effect is a readonly CSS property.
Comment 9 Antti Koivisto 2021-06-08 02:36:00 PDT
Created attachment 430822 [details]
patch
Comment 10 Antti Koivisto 2021-06-08 02:52:13 PDT
> What do you mean by "simply set it"? It's not possible to get rid of text
> decorations, right? -webkit-text-decorations-in-effect is a readonly CSS
> property.

Yeah, I suppose it is bit hard to get rid of it without an extension that makes the property directly available in UA style.
Comment 11 Antti Koivisto 2021-06-08 02:57:51 PDT
Created attachment 430825 [details]
patch
Comment 12 EWS 2021-06-08 05:11:38 PDT
Committed r278602 (238591@main): <https://commits.webkit.org/238591@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430825 [details].