1365162 - stylo: need to be able to match :lang on snapshots

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P2)

Description

[Boris Zbarsky [:bzbarsky]]

Right now we delegate it to the element (and eventually to nsCSSRuleProcessor::StringPseudoMatches), but it depends on the value of the "lang" and "xml:lang" attributes of the element and its ancestors.

This is causing test failures; for example layout/reftests/bugs/631352-1.html fails because we don't properly restyle on "lang" attribute change.

Comment 1

[Bobby Holley (:bholley)]

This is similar to bug 1365160 and bug 1365159.

Comment 2

[Cameron McCormack (:heycam)]

We also need changes to ancestor lang="" and xml:lang="" attributes to cause us to re-selector match the subtree, so that we attempt to pick up the new language on descendants.  (Like nsCSSRuleProcessor::HasAttributeDependentStyle does.)

Comment 3

[Cameron McCormack (:heycam)]

Do we need to match :lang() against snapshots, actually?  It seems like it's hard to avoid posting RestyleSubtree on the element whose lang="" attribute changed.  We can't do anything useful with the old lang="" value in the snapshot when converting it to a restyle hint, because we need to know whether descendants started or stopped matching.

(Well, we could post a "restyle all descendants, not ourselves" when the lang="" attribute changes, and use the dependencies to determine whether to restyle the element itself.  But that doesn't seem worth it if we have to restyle all descendants anyway.)

Comment 4

[Cameron McCormack (:heycam)]

Attachment: Bug 1365162 - Part 1: Restyle entire subtree when lang="" or xml:lang="" changes.

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

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8874310 [details]
Bug 1365162 - Part 1: Restyle entire subtree when lang="" or xml:lang="" changes.

https://reviewboard.mozilla.org/r/145664/#review149676

So, even though this is probably fine, I think we still need to match `:lang` on snapshots. Otherwise, when would we handle selectors like `:lang(foo) + div`?

We can only look at the snapshot lang attribute though (I think), given if any other ancestor's lang attribute changed, we'd restyle all the siblings anyway too.

Comment 6

[Cameron McCormack (:heycam)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #5)
> So, even though this is probably fine, I think we still need to match
> `:lang` on snapshots. Otherwise, when would we handle selectors like
> `:lang(foo) + div`?

Oh, great point.  Luckily I already wrote the code to make :lang match on snapshots, so I will `git stash pop` when I'm in the office tomorrow. ;)

Comment 7

[Boris Zbarsky [:bzbarsky]]

We should add a testcase for the ":lang(foo) + div" case if we don't have one already.

Comment 8

[Bobby Holley (:bholley)]

We could solve this by also posting restyle later siblings when the lang changes, right?

Comment 9

[Emilio Cobos Álvarez (:emilio)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #8)
> We could solve this by also posting restyle later siblings when the lang
> changes, right?

We _could_, but that'd be even more inefficient. Perhaps it's rare enough it doesn't matter though...

Comment 10

[Cameron McCormack (:heycam)]

Attachment: script: Move extended_filtering to the style crate.

We'll need to call it from the style crate in later patches, when matching
:lang() against element snapshots.

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

Comment 11

[Cameron McCormack (:heycam)]

Attachment: Bug 1365162 - Part 2: Factor out lang="" namespace checks.

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

Comment 12

[Cameron McCormack (:heycam)]

Attachment: Bug 1365162 - Part 3: Record on snapshots whether the element supports lang="" attributes.

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

Comment 13

[Cameron McCormack (:heycam)]

Attachment: Bug 1365162 - Part 4: Add FFI functions for getting the relevant lang attribute value for an element or snapshot.

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

Comment 14

[Cameron McCormack (:heycam)]

Attachment: Bug 1365162 - Part 5: Factor out :lang() matching function.

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

Comment 15

[Cameron McCormack (:heycam)]

Attachment: style: Define types for pseudo-class string argument storage.

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

Comment 16

[Cameron McCormack (:heycam)]

Attachment: style: Note :lang() as being sensitive to attributes.

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

Comment 17

[Cameron McCormack (:heycam)]

Attachment: Bug 1365162 - Part 6: Add FFI function for matching :lang().

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

Comment 18

[Cameron McCormack (:heycam)]

Attachment: style: Match :lang() using snapshots correctly.

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

Comment 19

[Cameron McCormack (:heycam)]

Attachment: Bug 1365162 - Part 7: Test for :lang() with a sibling combinator.

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

Comment 20

[Cameron McCormack (:heycam)]

Comment on attachment 8874310 [details]
Bug 1365162 - Part 1: Restyle entire subtree when lang="" or xml:lang="" changes.

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

Comment 21

[Cameron McCormack (:heycam)]

Comment on attachment 8874310 [details]
Bug 1365162 - Part 1: Restyle entire subtree when lang="" or xml:lang="" changes.

(MozReview didn't want to re-open review on this patch...)

Comment 22

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=56fb2f2712112630def0e2d6eea57e5ee3d8b5ff

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8874310 [details]
Bug 1365162 - Part 1: Restyle entire subtree when lang="" or xml:lang="" changes.

https://reviewboard.mozilla.org/r/145664/#review150546

::: layout/base/ServoRestyleManager.cpp:815
(Diff revision 2)
> -  // <td> is affected by the cellpadding on its ancestor table,
> -  // so we should restyle the whole subtree
> -  if (aAttribute == nsGkAtoms::cellpadding && aElement->IsHTMLElement(nsGkAtoms::table)) {
> +
> +  // For some attribute changes we must restyle the whole subtree:
> +  //
> +  // * <td> is affected by the cellpadding on its ancestor table
> +  // * lang="" and xml:lang="" can affect all descendants due to :lang()
> +  if ((aAttribute == nsGkAtoms::cellpadding &&

nit: It'd be nice if these long conditions where into its own function.

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8875123 [details]
script: Move extended_filtering to the style crate.

https://reviewboard.mozilla.org/r/146518/#review150548

I know these are pre-existing, but if you could fix them up while you're here it'd be really nice.

::: servo/components/style/servo/selector_parser.rs:619
(Diff revision 1)
>  }
> +
> +/// Returns whether the language is matched, as defined by
> +/// [RFC 4647](https://tools.ietf.org/html/rfc4647#section-3.3.2).
> +pub fn extended_filtering(tag: &str, range: &str) -> bool {
> +    let lang_ranges: Vec<&str> = range.split(',').collect();

There's no need to allocate here, just use `range.split(',')`.

::: servo/components/style/servo/selector_parser.rs:623
(Diff revision 1)
> +pub fn extended_filtering(tag: &str, range: &str) -> bool {
> +    let lang_ranges: Vec<&str> = range.split(',').collect();
> +
> +    lang_ranges.iter().any(|&lang_range| {
> +        // step 1
> +        let range_subtags: Vec<&str> = lang_range.split('\x2d').collect();

Ditto here, I think

::: servo/components/style/servo/selector_parser.rs:642
(Diff revision 1)
> +        let mut current_tag_subtag = tag_iter.next();
> +
> +        // step 3
> +        for range_subtag in range_iter {
> +            // step 3a
> +            if range_subtag.eq_ignore_ascii_case("*") {

`eq_ignore_ascii_case("*")` is the same as `==`, I guess.

::: servo/components/style/servo/selector_parser.rs:651
(Diff revision 1)
> +                Some(tag_subtag) => {
> +                    // step 3c
> +                    if range_subtag.eq_ignore_ascii_case(tag_subtag) {
> +                        current_tag_subtag = tag_iter.next();
> +                        continue;
> +                    } else {

no need for `else` after `continue`.

::: servo/components/style/servo/selector_parser.rs:656
(Diff revision 1)
> +                    } else {
> +                        // step 3d
> +                        if tag_subtag.len() == 1 {
> +                            return false;
> +                        } else {
> +                            // else step 3e - continue with loop

nor after return.

::: servo/components/style/servo/selector_parser.rs:665
(Diff revision 1)
> +                            }
> +                        }
> +                    }
> +                },
> +                // step 3b
> +                None => { return false; }

Braces here look somewhat odd.

Comment 25

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8875124 [details]
Bug 1365162 - Part 2: Factor out lang="" namespace checks.

https://reviewboard.mozilla.org/r/146520/#review150550

Comment 26

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8875125 [details]
Bug 1365162 - Part 3: Record on snapshots whether the element supports lang="" attributes.

https://reviewboard.mozilla.org/r/146522/#review150552

Comment 27

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8875126 [details]
Bug 1365162 - Part 4: Add FFI functions for getting the relevant lang attribute value for an element or snapshot.

https://reviewboard.mozilla.org/r/146524/#review150556

::: layout/style/ServoBindings.cpp:835
(Diff revision 1)
> +  if (!attr) {
> +    return nullptr;
> +  }
> +
> +  nsString lang;
> +  attr->ToString(lang);

Do we really need to serialize it, then atomize it? Isn't it already stored as an atom if non-empty? There needs to be a way to do this more efficiently I think.

Comment 28

[Cameron McCormack (:heycam)]

Comment on attachment 8875126 [details]
Bug 1365162 - Part 4: Add FFI functions for getting the relevant lang attribute value for an element or snapshot.

https://reviewboard.mozilla.org/r/146524/#review150556

> Do we really need to serialize it, then atomize it? Isn't it already stored as an atom if non-empty? There needs to be a way to do this more efficiently I think.

It didn't seem so.  I got assertions when calling GetAtomValue() because it was stored as a string.  Perhaps we should make lang="" always be stored as an atom, though I'm not sure where to do that.

Comment 29

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8875127 [details]
Bug 1365162 - Part 5: Factor out :lang() matching function.

https://reviewboard.mozilla.org/r/146526/#review150560

::: layout/style/nsCSSRuleProcessor.cpp:1681
(Diff revision 1)
>  }
>  
>  /* static */ bool
> +nsCSSRuleProcessor::LangPseudoMatches(const mozilla::dom::Element* aElement,
> +                                      const nsAString* aOverrideLang,
> +                                      bool aHasOverrideLang,

If it's only called from one place, and it always has `nullptr`, `false` as `aOverrideLang` and `aHasOverrideLang`, perhaps we could simplify this removing these two arguments?

Comment 30

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8875128 [details]
style: Define types for pseudo-class string argument storage.

https://reviewboard.mozilla.org/r/146528/#review150562

Comment 31

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8875129 [details]
style: Note :lang() as being sensitive to attributes.

https://reviewboard.mozilla.org/r/146530/#review150564

Comment 32

[Cameron McCormack (:heycam)]

Comment on attachment 8875127 [details]
Bug 1365162 - Part 5: Factor out :lang() matching function.

https://reviewboard.mozilla.org/r/146526/#review150560

> If it's only called from one place, and it always has `nullptr`, `false` as `aOverrideLang` and `aHasOverrideLang`, perhaps we could simplify this removing these two arguments?

Later patches will pass different things to those arguments.

Comment 33

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8875130 [details]
Bug 1365162 - Part 6: Add FFI function for matching :lang().

https://reviewboard.mozilla.org/r/146532/#review150570

Comment 34

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8875131 [details]
style: Match :lang() using snapshots correctly.

https://reviewboard.mozilla.org/r/146534/#review150568

::: servo/components/style/restyle_hints.rs:617
(Diff revision 1)
> +
> +    /// Returns the value of the `xml:lang=""` (or, if appropriate, `lang=""`)
> +    /// attribute from this element's snapshot or the closest ancestor
> +    /// element snapshot with the attribute specified.
> +    fn get_lang(&self) -> Option<AttrValue> {
> +        let mut e = self.clone();

Perhaps `current` is slightly more descriptive than `e`?

Comment 35

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8875132 [details]
Bug 1365162 - Part 7: Test for :lang() with a sibling combinator.

https://reviewboard.mozilla.org/r/146536/#review150572

::: layout/reftests/bugs/reftest.list:2012
(Diff revision 1)
>  == 1366144.html 1366144-ref.html
>  == 1369584-1a.html 1369584-1-ref.html
>  == 1369584-1b.html 1369584-1-ref.html
>  == 1367592-1.html 1367592-1-ref.html
>  == 1365159-1.html 1365159-1-ref.html
> +fails-if(!stylo&&!styloVsGecko) == 1365162-1.html 1365162-1-ref.html

Oh, really, does this fail on Gecko? That's unfortunate.

Comment 36

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8875126 [details]
Bug 1365162 - Part 4: Add FFI functions for getting the relevant lang attribute value for an element or snapshot.

https://reviewboard.mozilla.org/r/146524/#review150574

::: layout/style/ServoBindings.cpp:835
(Diff revision 1)
> +  if (!attr) {
> +    return nullptr;
> +  }
> +
> +  nsString lang;
> +  attr->ToString(lang);

It seems that for IDs and classes it's done in Element::ParseAttribute[1].

We may be able to do the same for lang... But perhaps it's fine as a followup, could you file?

[1]: http://searchfox.org/mozilla-central/rev/20963d7269b1b14d455f47bc0260d0653015bf84/dom/base/Element.cpp#2638

Comment 37

[Cameron McCormack (:heycam)]

Comment on attachment 8875132 [details]
Bug 1365162 - Part 7: Test for :lang() with a sibling combinator.

https://reviewboard.mozilla.org/r/146536/#review150572

> Oh, really, does this fail on Gecko? That's unfortunate.

Yeah.  I could do the "post a LaterSiblings hint too" thing that Bobby suggests just for Gecko.  Just wasn't sure it was worth the bother.

Comment 38

[Cameron McCormack (:heycam)]

Filed bug 1370802 for the atom thing.

Comment 39

[Cameron McCormack (:heycam)]

Comment on attachment 8874310 [details]
Bug 1365162 - Part 1: Restyle entire subtree when lang="" or xml:lang="" changes.

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

Comment 40

[Cameron McCormack (:heycam)]

Comment on attachment 8875124 [details]
Bug 1365162 - Part 2: Factor out lang="" namespace checks.

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

Comment 41

[Cameron McCormack (:heycam)]

Comment on attachment 8875125 [details]
Bug 1365162 - Part 3: Record on snapshots whether the element supports lang="" attributes.

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

Comment 42

[Cameron McCormack (:heycam)]

Comment on attachment 8875126 [details]
Bug 1365162 - Part 4: Add FFI functions for getting the relevant lang attribute value for an element or snapshot.

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

Comment 43

[Cameron McCormack (:heycam)]

Comment on attachment 8875127 [details]
Bug 1365162 - Part 5: Factor out :lang() matching function.

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

Comment 44

[Cameron McCormack (:heycam)]

Comment on attachment 8875130 [details]
Bug 1365162 - Part 6: Add FFI function for matching :lang().

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

Comment 45

[Cameron McCormack (:heycam)]

Comment on attachment 8875132 [details]
Bug 1365162 - Part 7: Test for :lang() with a sibling combinator.

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

Comment 46

[Boris Zbarsky [:bzbarsky]]

> I could do the "post a LaterSiblings hint too" thing that Bobby suggests just for Gecko. 

Or you could add selectors involving :lang to aCascade via AttributeListFor(nsGkAtoms::lang) in AddSelector.

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/53c186e01b12
Part 1: Restyle entire subtree when lang="" or xml:lang="" changes. r=emilio
https://hg.mozilla.org/integration/autoland/rev/6751fa7c5261
Part 2: Factor out lang="" namespace checks. r=emilio
https://hg.mozilla.org/integration/autoland/rev/e10c7dba0349
Part 3: Record on snapshots whether the element supports lang="" attributes. r=emilio
https://hg.mozilla.org/integration/autoland/rev/c1cac2541d86
Part 4: Add FFI functions for getting the relevant lang attribute value for an element or snapshot. r=emilio
https://hg.mozilla.org/integration/autoland/rev/383f5061c80e
Part 5: Factor out :lang() matching function. r=emilio
https://hg.mozilla.org/integration/autoland/rev/bc0e5e592ff4
Part 6: Add FFI function for matching :lang(). r=emilio
https://hg.mozilla.org/integration/autoland/rev/d27eb08d5cff
Part 7: Test for :lang() with a sibling combinator. r=emilio

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2d432523af9f
Followup: fix reftest annotation.

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f223e1fd2044
Followup: fix reftest annotation.

Comment 50

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/53c186e01b12
https://hg.mozilla.org/mozilla-central/rev/6751fa7c5261
https://hg.mozilla.org/mozilla-central/rev/e10c7dba0349
https://hg.mozilla.org/mozilla-central/rev/c1cac2541d86
https://hg.mozilla.org/mozilla-central/rev/383f5061c80e
https://hg.mozilla.org/mozilla-central/rev/bc0e5e592ff4
https://hg.mozilla.org/mozilla-central/rev/d27eb08d5cff
https://hg.mozilla.org/mozilla-central/rev/2d432523af9f
https://hg.mozilla.org/mozilla-central/rev/f223e1fd2044