1382786 - stylo: implement restrictions on which properties apply to ::first-letter, ::first-line, ::placeholder

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Boris Zbarsky [:bzbarsky]]

Per CSS spec, only certain properties apply to first-line, first-letter, placeholder pseudo-elements.

The way Gecko implements this is as follows:

1) Style contexts with those pseudos are forced into separate branches of the ruletree via "restriction rules".  This is needed to make caching in the ruletree work ok; stylo doesn't need this part.

2) When determining the specified style, once the walk through the ruletree is done and we've mapped everything into the ruledata, if we're resolving one of those pseudos we go through and reset all the specified values to "not specified" for properties that don't have the relevant "allowed for this pseudo" bits set.

We need to do something like this for servo.  A quick experiment:

  <style>
    input::placeholder { border-top-color: red; background: green; }
  </style>
  <script>
    var d = document.createElement("input");
    var s = getComputedStyle(d, "::placeholder");
    alert(s.borderTopColor);
    alert(s.backgroundColor);
  </script>

shows we already do this for ::placeholder.  But we need to do it for ::first-letter and ::first-line too.  Testcase:

  <style>
    div::first-line { border-top-color: red; }
    div::first-letter { display: table }
  </style>
  <script>
    var d = document.createElement("div");
    var s = getComputedStyle(d, "::first-line");
    alert(s.borderTopColor);
    s = getComputedStyle(d, "::first-letter");
    alert(s.display);
  </script>

which should alert "rgb(0, 0, 0)" and "inline".

Comment 1

[Boris Zbarsky [:bzbarsky]]

> shows we already do this for ::placeholder

Nope.  It just looks like that because of !important rules in ua.css.  Here's a failing testcase for ::placeholder:

Comment 2

[Boris Zbarsky [:bzbarsky]]

  <style>
    input::placeholder { float: right; }
  </style>
  <script>
    var d = document.createElement("input");
    var s = getComputedStyle(d, "::placeholder");
    alert(s.cssFloat);
  </script>

this should alert "none" but servo alerts "right".

Comment 3

[Emilio Cobos Álvarez [:emilio]]

So this is the kind of thing that Gecko implements with the different rule implementations in the rule tree, right?

I think in Servo it'd be nice to follow the pattern we have for `:visited` styles, and filter on the cascade() function instead, but happy to hear other options.

I think it should be somewhat easier now than it was not long ago, I added information of which pseudo we're cascading to StyleBuilder. And it should be just one branch in the "no pseudo" case.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> So this is the kind of thing that Gecko implements with the different rule implementations in the rule tree

No, it's actually not, except for the restriction rule bits to force not-sharing of cached data between rulenodes that have had this thing applied to them and once that haven't.  When we last talked about this, I'd mis-remembered what the restriction rules were for.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Add property flags for which properties apply to certain pseudo-elements.

These flags can be used to flag properties that apply to ::first-letter, ::first-line, and ::placeholder

Part 1 of the fix for Gecko bug 1382786 <https://bugzilla.mozilla.org/show_bug.cgi?id=1382786>.

Review commit: https://reviewboard.mozilla.org/r/159588/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/159588/

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Flag properties that apply to ::first-letter/::first-line/::placeholder.

Part 2 of the fix for Gecko bug 1382786 <https://bugzilla.mozilla.org/show_bug.cgi?id=1382786>.

Review commit: https://reviewboard.mozilla.org/r/159590/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/159590/

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Filter out non-applying properties when cascading style for ::first-letter/::first-line/::placeholder.

Part 3 of the fix for Gecko bug 1382786 <https://bugzilla.mozilla.org/show_bug.cgi?id=1382786>.

Review commit: https://reviewboard.mozilla.org/r/159592/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/159592/

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1382786 part 4. Update test expectations.

Review commit: https://reviewboard.mozilla.org/r/159594/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/159594/

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1382786 part 5. Add some tests for which properties are properties are allowed on ::first-letter, ::first-line, and ::placeholder.

Review commit: https://reviewboard.mozilla.org/r/159596/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/159596/

Comment 10

[Boris Zbarsky [:bzbarsky]]

I'm really glad I wrote part 5, by the way: it turned out when I was done, that I had in fact managed to get some of the flags wrong for Stylo.  But at this point Gecko and Stylo both pass the new tests, which is a good sign.  ;)

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8888599 [details]
Bug 1382786 part 4.  Update test expectations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159594/diff/1-2/

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8888600 [details]
Bug 1382786 part 5.  Add some tests for which properties are properties are allowed on ::first-letter, ::first-line, and ::placeholder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159596/diff/1-2/

Comment 13

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8888596 [details]
Add property flags for which properties apply to certain pseudo-elements.

https://reviewboard.mozilla.org/r/159588/#review165074

Comment 14

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8888597 [details]
Flag properties that apply to ::first-letter/::first-line/::placeholder.

https://reviewboard.mozilla.org/r/159590/#review165076

::: servo/components/style/properties/longhand/background.mako.rs:42
(Diff revision 1)
> +        flags="APPLIES_TO_FIRST_LETTER APPLIES_TO_FIRST_LINE APPLIES_TO_PLACEHOLDER",
>      )}
>  % endfor
>  
>  <%helpers:vector_longhand name="background-repeat" animation_value_type="discrete"
> -                          spec="https://drafts.csswg.org/css-backgrounds/#the-background-repeat">
> +                          spec="https://drafts.csswg.org/css-backgrounds/#the-background-repeat" 

nit: trailing whitespace.

::: servo/components/style/properties/longhand/box.mako.rs:15
(Diff revision 1)
>                           gecko_name="Display") %>
>  
>  // TODO(SimonSapin): don't parse `inline-table`, since we don't support it
> +//
> +// We allow "display" to apply to placeholders because we need to make the
> +// placeholder pseudo-element an inline-block in the UA stylesheet in Gecko.

Oh, I was assuming we wouldn't check this for rules in UA sheets. But this is fine too I guess.

::: servo/components/style/properties/longhand/box.mako.rs:1635
(Diff revision 1)
>  // http://dev.w3.org/csswg/css-ui
>  // FIXME support logical values `block` and `inline` (https://drafts.csswg.org/css-logical-props/#resize)
> +//
> +// This is APPLIES_TO_PLACEHOLDER so we can override, in the UA sheet, the
> +// 'resize' property we'd inherit from textarea otherwise.  Basically, just
> +// makes the UA rules easier to write.

Ditto, let me know if that'd be more preferrable or not.

::: servo/components/style/properties/longhand/inherited_text.mako.rs:732
(Diff revision 1)
>      "-webkit-text-fill-color", "Color",
>      "computed_value::T::currentcolor()",
>      products="gecko", animation_value_type="IntermediateColor",
>      need_clone=True, ignored_when_colors_disabled=True,
> +    flags="APPLIES_TO_FIRST_LETTER APPLIES_TO_FIRST_LINE APPLIES_TO_PLACEHOLDER",
> +    

nit: Trailing whitespace (though I guess this line can go away entirely)

::: servo/components/style/properties/longhand/padding.mako.rs:10
(Diff revision 1)
>  <%namespace name="helpers" file="/helpers.mako.rs" />
>  <% from data import ALL_SIDES, maybe_moz_logical_alias %>
>  <% data.new_style_struct("Padding", inherited=False) %>
>  
> +// APPLIES_TO_PLACEHOLDER so we can set it in UA  stylesheets.  But we use a
> +// !important value there, so pages can't set it.    

nit: Trailing whitespace.

Comment 15

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8888598 [details]
Filter out non-applying properties when cascading style for ::first-letter/::first-line/::placeholder.

https://reviewboard.mozilla.org/r/159592/#review165082

::: servo/components/style/properties/properties.mako.rs:2783
(Diff revision 1)
>              };
>              let node_importance = node.importance();
> +
> +            let required_flag = match pseudo {
> +                #[cfg(feature = "gecko")]
> +                Some(&PseudoElement::FirstLetter) => Some(APPLIES_TO_FIRST_LETTER),

nit: Not sure if this'd be more elegant with an explicit method in `PseudoElement`. Also, perhaps a slightly more descriptive name would be something like `property_restrictions`? It'd look something like:

```
let property_restrictions = pseudo.and_then(|p| p.property_restrictions());
```

::: servo/components/style/properties/properties.mako.rs:2796
(Diff revision 1)
>              declarations
>                  .iter()
>                  // Yield declarations later in source order (with more precedence) first.
>                  .rev()
>                  .filter_map(move |&(ref declaration, declaration_importance)| {
> +                    if let Some(required_flag) = required_flag {

If you think it's better to just skip UA rules as I suggested, you can avoid looking at `cascade_level` here.

::: servo/components/style/properties/properties.mako.rs:2797
(Diff revision 1)
>                  .iter()
>                  // Yield declarations later in source order (with more precedence) first.
>                  .rev()
>                  .filter_map(move |&(ref declaration, declaration_importance)| {
> +                    if let Some(required_flag) = required_flag {
> +                        // Only allow longhands if they have the flag set

nit: Not sure the comment is supper-helpful...

Comment 16

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8888599 [details]
Bug 1382786 part 4.  Update test expectations.

https://reviewboard.mozilla.org/r/159594/#review165084

Comment 17

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8888600 [details]
Bug 1382786 part 5.  Add some tests for which properties are properties are allowed on ::first-letter, ::first-line, and ::placeholder.

https://reviewboard.mozilla.org/r/159596/#review165086

::: layout/style/test/test_first_line_restrictions.html:39
(Diff revision 2)
> +  if (info.prerequisites) {
> +    for (let name in info.prerequisites) {
> +      prereqs += `${name}: ${info.prerequisites[name]}; `;
> +    }
> +  }
> +  $("s").textContent = `

I bet this is how you found bug 1382927 ;)

Comment 18

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8888598 [details]
Filter out non-applying properties when cascading style for ::first-letter/::first-line/::placeholder.

https://reviewboard.mozilla.org/r/159592/#review165082

> If you think it's better to just skip UA rules as I suggested, you can avoid looking at `cascade_level` here.

Err, I meant you can avoid looking at the flags after looking at cascade_level.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8888597 [details]
Flag properties that apply to ::first-letter/::first-line/::placeholder.

https://reviewboard.mozilla.org/r/159590/#review165076

> Oh, I was assuming we wouldn't check this for rules in UA sheets. But this is fine too I guess.

Hmm.  I basically duplicated Gecko behavior (albeit with more overhead, because Gecko does this only once all the stuff is cascaded, and with less checks in the inner hot loop, but I couldn't figure out how to do that here).

We could think about allowing this in UA sheets in general, but that's pretty dangerous: it's too easy to create styles that will assert or crash.  For now I'd prefer keeping the Gecko behavior.

Comment 20

[Boris Zbarsky [:bzbarsky]]

> I bet this is how you found bug 1382927 ;)

Yes, it is.  I was running the tests in Stylo and Gecko and as I was adding stuff to property_database to make them green, and noticed that the Stylo version took a lot longer to run.  ;)

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8888598 [details]
Filter out non-applying properties when cascading style for ::first-letter/::first-line/::placeholder.

https://reviewboard.mozilla.org/r/159592/#review165082

> nit: Not sure if this'd be more elegant with an explicit method in `PseudoElement`. Also, perhaps a slightly more descriptive name would be something like `property_restrictions`? It'd look something like:
> 
> ```
> let property_restrictions = pseudo.and_then(|p| p.property_restrictions());
> ```

Good idea.

> nit: Not sure the comment is supper-helpful...

I'll rephase it to be clearer.  The point is that custom properties are always allowed.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8888597 [details]
Flag properties that apply to ::first-letter/::first-line/::placeholder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159590/diff/1-2/

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8888598 [details]
Filter out non-applying properties when cascading style for ::first-letter/::first-line/::placeholder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159592/diff/1-2/

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8888599 [details]
Bug 1382786 part 4.  Update test expectations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159594/diff/2-3/

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8888600 [details]
Bug 1382786 part 5.  Add some tests for which properties are properties are allowed on ::first-letter, ::first-line, and ::placeholder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159596/diff/2-3/

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8888599 [details]
Bug 1382786 part 4.  Update test expectations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159594/diff/3-4/

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8888600 [details]
Bug 1382786 part 5.  Add some tests for which properties are properties are allowed on ::first-letter, ::first-line, and ::placeholder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159596/diff/3-4/

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fb19f23bb69a
part 4.  Update test expectations.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/cc9fbf7eabe7
part 5.  Add some tests for which properties are properties are allowed on ::first-letter, ::first-line, and ::placeholder.  r=emilio

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f9ddd2f1395
followup.  Mark some more tests as passing.

Comment 30

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/fb19f23bb69a
https://hg.mozilla.org/mozilla-central/rev/cc9fbf7eabe7
https://hg.mozilla.org/mozilla-central/rev/6f9ddd2f1395

Comment 31

[Chris Nardi]

Is there any specific reason why shape-outside was decided to be a property ::first-letter could have? I can't see any note in https://drafts.csswg.org/css-pseudo-4/#first-letter-styling or https://drafts.csswg.org/css-shapes/#shape-outside-property about shape-outside and ::first-letter.

Comment 32

[Emilio Cobos Álvarez [:emilio]]

(In reply to Chris Nardi from comment #31)
> Is there any specific reason why shape-outside was decided to be a property
> ::first-letter could have? I can't see any note in
> https://drafts.csswg.org/css-pseudo-4/#first-letter-styling or
> https://drafts.csswg.org/css-shapes/#shape-outside-property about
> shape-outside and ::first-letter.

I guess per the spec you mention it shouldn't, since https://drafts.csswg.org/css-pseudo-4/#first-letter-styling doesn't mention it explicitly, but says:

> any other properties defined to apply to ::first-letter by their respective specifications 

And shape-outside is not defined to apply to ::first-letter explicitly.

If you think it should or what not please do file a different bug for it, this bug was only implementing in the new style system what the old style system was doing, effectively (bug 469227 implemented the latest mechanism, and such).

Comment 33

[Chris Nardi]

I have no opinion on the matter; I was just trying to match up Chrome's implementation of what properties could apply to ::first-letter to the spec and was using Stylo as a guide.

Comment 34

[Emilio Cobos Álvarez [:emilio]]

(In reply to Chris Nardi from comment #33)
> I have no opinion on the matter; I was just trying to match up Chrome's
> implementation of what properties could apply to ::first-letter to the spec
> and was using Stylo as a guide.

Ah, awesome, thanks!

Comment 35

[Emilio Cobos Álvarez [:emilio]]

Also, let me know if there's anything I can help with :)

Comment 36

[Chris Nardi]

Thank you, I'll be sure to ask if I have any other questions. :)