Closed Bug 1365162 Opened 2 years ago Closed 2 years ago

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

Categories

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

53 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: bzbarsky, Assigned: heycam)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 4 obsolete files)

59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
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.
This is similar to bug 1365160 and bug 1365159.
Priority: -- → P2
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.)
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 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.
Attachment #8874310 - Flags: review?(emilio+bugs) → review-
(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. ;)
We should add a testcase for the ":lang(foo) + div" case if we don't have one already.
We could solve this by also posting restyle later siblings when the lang changes, right?
(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...
Attachment #8874310 - Flags: review?(emilio+bugs)
Attachment #8875123 - Flags: review?(emilio+bugs)
Attachment #8875128 - Flags: review?(emilio+bugs)
Attachment #8875129 - Flags: review?(emilio+bugs)
Attachment #8875131 - Flags: review?(emilio+bugs)
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...)
Attachment #8874310 - Flags: review?(emilio+bugs)
Assignee: nobody → cam
Status: NEW → ASSIGNED
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.
Attachment #8874310 - Flags: review?(emilio+bugs) → review+
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.
Attachment #8875123 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8875124 [details]
Bug 1365162 - Part 2: Factor out lang="" namespace checks.

https://reviewboard.mozilla.org/r/146520/#review150550
Attachment #8875124 - Flags: review?(emilio+bugs) → review+
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
Attachment #8875125 - Flags: review?(emilio+bugs) → review+
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 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 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?
Attachment #8875127 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8875128 [details]
style: Define types for pseudo-class string argument storage.

https://reviewboard.mozilla.org/r/146528/#review150562
Attachment #8875128 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8875129 [details]
style: Note :lang() as being sensitive to attributes.

https://reviewboard.mozilla.org/r/146530/#review150564
Attachment #8875129 - Flags: review?(emilio+bugs) → review+
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 on attachment 8875130 [details]
Bug 1365162 - Part 6: Add FFI function for matching :lang().

https://reviewboard.mozilla.org/r/146532/#review150570
Attachment #8875130 - Flags: review?(emilio+bugs) → review+
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`?
Attachment #8875131 - Flags: review?(emilio+bugs) → review+
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.
Attachment #8875132 - Flags: review?(emilio+bugs) → review+
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
Attachment #8875126 - Flags: review?(emilio+bugs) → review+
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.
Filed bug 1370802 for the atom thing.
Attachment #8875123 - Attachment is obsolete: true
Attachment #8875128 - Attachment is obsolete: true
Attachment #8875129 - Attachment is obsolete: true
Attachment #8875131 - Attachment is obsolete: true
> 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.
Pushed by cmccormack@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.