Closed Bug 1364412 Opened 7 years ago Closed 7 years ago

stylo: Support state selectors in pseudo-elements.

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(7 files, 5 obsolete files)

Bug 1364412: Simplify Servo_HasAuthorSpecifiedRules looking at the pseudo style.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1364412: Don't ignore the pseudo-element argument in ResolvePseudoElementStyle.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1364412: Reftest expectation updates.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1364412: Convert pseudo-elements to an enum.
59 bytes, text/x-review-board-request
hiro
: review+
xidorn
: review+
Details
Interdiff of the file moving in the last part, for ease of review
3.94 KB, patch
xidorn
: review+
Details | Diff | Splinter Review
Bug 1364412: Expose pseudo-element flags, and properly reject pseudos in non-UA sheets.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1364412: Properly reject to parse pseudo-element states that don't support state.
59 bytes, text/x-review-board-request
bholley
: review+
Details 
Uncovered by bug 1364377, we don't support selectors like:

foo::-moz-progress-bar:hover { .. }

Before that bug, those pseudos just didn't show up.

That's slightly annoying to do, but probably not too much.
I'm taking this.
Assignee: nobody → emilio+bugs
Assertions are bug 1352306. Still one selector left in test_pseudo_element_parsing.html I need to look into. Other failures are the ones uncovered in bug 1364377, but it's strictly a win. I expect the last commit to fix the last remaining cases of layout/style/test/test_pseudoelement_state.html.
Comment on attachment 8867282 [details]
Bug 1364412: Track pseudo-element's state dependencies too.

https://reviewboard.mozilla.org/r/138818/#review142268
Attachment #8867282 - Flags: review?(cam) → review+
A known problem of this is that we need to reject parsing of state selectors in some pseudos but not others (that's the remaining failure in test_pseudo_element_parsing.html).

This patch blanket allows it. I'm running out of time for today, and I'd like to address that properly in a followup bug I think, given the solution I want for it and to make PseudoElement smaller is to switch our PseudoElement representation to be CSSPseudoElementType instead of atoms, and that involves some work.

If people think that's reasonable, that'd be lovely :). Otherwise I'll work on those patches next week.
Comment on attachment 8867505 [details]
Bug 1364412: Don't ignore the pseudo-element argument in ResolvePseudoElementStyle.

https://reviewboard.mozilla.org/r/139044/#review142312
Attachment #8867505 - Flags: review?(cam) → review+
Comment on attachment 8867556 [details]
Bug 1364412: Convert pseudo-elements to an enum.

https://reviewboard.mozilla.org/r/139098/#review142344

Thanks for doing this!  Animation codes looks much better!

r=me for C++ and its glue parts.

::: dom/canvas/CanvasRenderingContext2D.cpp:2877
(Diff revision 1)
> -    parentStyle = styleSet->ResolveTransientServoStyle(aElement, nullptr);
> +    parentStyle =
> +      styleSet->ResolveTransientServoStyle(aElement,
> +                                           CSSPseudoElementType::NotPseudo);

Here is the most favorite part of this patch.  Passing nullptr is really hard to read what the nullptr means. yay!
Attachment #8867556 - Flags: review?(hikezoe) → review+
Comment on attachment 8867556 [details]
Bug 1364412: Convert pseudo-elements to an enum.

https://reviewboard.mozilla.org/r/139098/#review142354

Doing file moving and mutating on a file in one commit really makes it harder to review. Please split this into at least two patches, one for removing binding_tools and moving regen_atoms.py into another directory, and the other for this change itself.

Also it seems to me that changes to `generated/` directory contains many unrelated changes. Probably you are using the files generated locally? Could you instead use the binding files generated from TreeHerder to minimize potential merge conflicts when people try to update bindings?

Actually I'd prefer you exclude the changes to that directory from this commit. Those changes in general don't really need review, but they are annoying because of their lengthiness. I think it would be better to only include them when landing to Servo, or at least, keep them in a separate commit.
Attachment #8867556 - Flags: review?(xidorn+moz) → review-
I'm not too hacky with the path insertion thing, but I guess it's better than duplicating all that code.
Attachment #8867616 - Flags: review?(xidorn+moz)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #34)
> I'm not too hacky with the path insertion thing, but I guess it's better
> than duplicating all that code.

err, should be "too happy with the hacky path insertion..." Still early in the morning here :)
Blocks: 1364850
Comment on attachment 8867274 [details]
Bug 1364412: Allow pseudo-element selectors to store state.

https://reviewboard.mozilla.org/r/138810/#review142472

::: servo/components/style/gecko/selector_parser.rs:388
(Diff revision 3)
>  
>  /// The dummy struct we use to implement our selector parsing.
>  #[derive(Clone, Debug, PartialEq, Eq, Hash)]
>  pub struct SelectorImpl;
>  
> +/// Some subset of pseudo-elements in Gecko are sensible to some state

s/sensible/sensitive/

::: servo/components/style/gecko/selector_parser.rs:391
(Diff revision 3)
>  pub struct SelectorImpl;
>  
> +/// Some subset of pseudo-elements in Gecko are sensible to some state
> +/// selectors.
> +///
> +/// We store the sensible states in this struct in order to properly handle

same here.
Attachment #8867274 - Flags: review?(bobbyholley) → review+
Comment on attachment 8867275 [details]
Bug 1364412: Store full selectors in the Rule object.

https://reviewboard.mozilla.org/r/138812/#review142478
Attachment #8867275 - Flags: review?(bobbyholley) → review+
Comment on attachment 8867276 [details]
Bug 1364412: Use the pseudo selector to reject state selectors.

https://reviewboard.mozilla.org/r/138814/#review142480

::: servo/components/style/stylist.rs:1389
(Diff revision 3)
> +            debug_assert_eq!(rule.selector.pseudo_element.is_some(),
> +                             pseudo_element.is_some(),
> +                             "Testing pseudo-elements against the wrong map");
> +
> +            if let Some((pseudo, pseudo_state)) = pseudo_element {
> +                let pseudo_selector =
> +                    rule.selector.pseudo_element.as_ref().unwrap();
> +
> +                debug_assert_eq!(pseudo_selector.pseudo_element(), pseudo,
> +                                 "Testing pseudo-element against the wrong entry");
> +
> +                let state = pseudo_selector.state();
> +                if !state.is_empty() && !pseudo_state.intersects(state) {
> +                    continue;
> +                }

The extra branch here before bloom rejection is going to hurt on bloom_basic. But I can live with it for a day or two if we're definitely removing it in the followup by moving this logic into matches_complex_selector.

::: servo/components/style/stylist.rs:1401
(Diff revision 3)
> +
> +                debug_assert_eq!(pseudo_selector.pseudo_element(), pseudo,
> +                                 "Testing pseudo-element against the wrong entry");
> +
> +                let state = pseudo_selector.state();
> +                if !state.is_empty() && !pseudo_state.intersects(state) {

Given that we're storing mutiple flags in pseudo_state (rather than just storing a single one like we do in Component), I don't think we can do intersect, we need contains (specifically, state.contains(pseudo_state)). And I think that's ok, given that we don't have :any-link here? Please fix that and add a comment explaining it.

A test that fails without that change would be nice, but only if it doesn't take too long.

::: servo/ports/geckolib/glue.rs:1020
(Diff revision 3)
> +        // FIXME(emilio): This looks pretty wrong! Shouldn't it be at least an
> +        // empty style inheriting from the element?

Yeah, that sounds right. Please file.

::: servo/ports/geckolib/glue.rs:1059
(Diff revision 3)
> +            // FIXME(emilio): We should get the pseudo state here!
> +            d.stylist.lazy_pseudo_rules(&guards,

Indeed. Is that part of these patches, or do we need a followup? Same with ResolveRuleNode.
Attachment #8867276 - Flags: review?(bobbyholley) → review+
Comment on attachment 8867277 [details]
Bug 1364412: Simplify Servo_HasAuthorSpecifiedRules looking at the pseudo style.

https://reviewboard.mozilla.org/r/138816/#review142494

::: layout/base/nsPresContext.cpp
(Diff revision 3)
> -    if (!elem->HasServoData()) {
> -      return false;

Let's keep the !HasServoData early-return, and make the servo method expect(). Given how hard we've worked to prevent bailout cases in the Servo_Foo functions, I think it's better to keep these checks explicit in the caller.

::: servo/ports/geckolib/glue.rs:1002
(Diff revision 3)
> +    let data = match element.borrow_data() {
> +        Some(data) => data,
> +        None => return false,
> +    };
> +
> +    let primary_style = match data.get_styles() {
> +        Some(s) => &s.primary,
> +        None => return false,
> +    };
> +

Per above, let's expect() both of these . I know the original code handled with-servo-data-but-no-primary-styles, but that wasn't what I asked for when reviewing that code. We should assert both.
Attachment #8867277 - Flags: review?(bobbyholley) → review+
Comment on attachment 8867522 [details]
Bug 1364412: Reftest expectation updates.

https://reviewboard.mozilla.org/r/139072/#review142496

Nice. :-)
Attachment #8867522 - Flags: review?(bobbyholley) → review+
Attachment #8867616 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8867276 [details]
Bug 1364412: Use the pseudo selector to reject state selectors.

https://reviewboard.mozilla.org/r/138814/#review142480

> Given that we're storing mutiple flags in pseudo_state (rather than just storing a single one like we do in Component), I don't think we can do intersect, we need contains (specifically, state.contains(pseudo_state)). And I think that's ok, given that we don't have :any-link here? Please fix that and add a comment explaining it.
> 
> A test that fails without that change would be nice, but only if it doesn't take too long.

Actually, Gecko only allows a single pseudo-class, apparently. I'll fix the parsing to only allow one instead.

> Yeah, that sounds right. Please file.

Filed bug 1364880.

> Indeed. Is that part of these patches, or do we need a followup? Same with ResolveRuleNode.

This goes away in following patches, so does this function, so nothing left to do here AFAIK.
Comment on attachment 8867556 [details]
Bug 1364412: Convert pseudo-elements to an enum.

https://reviewboard.mozilla.org/r/139098/#review142532

r=me with nits addressed.

(TBH I guess I'm too sleepy now. This patch looks roughly fine. I probably should have another look at this patch again tomorrow morning, but that doesn't need to block you landing it.)

::: servo/components/style/gecko/pseudo_element.rs:7
(Diff revision 1)
> +//! Note that a few autogenerated bits of this live in
> +//! `pseudo_element_definition.mako.rs`. If you touch that file, you also need
> +//! to regenerate the generated files.

This is not necessary. regen_atoms.py is invoked by build_gecko.rs and thus should be updated at build-time automatically. And anyone who needs to update the in-tree files just need to extract the stylo-bindings.zip from TreeHerder, and all files in gecko/generated are included in it.

::: servo/components/style/gecko/pseudo_element_definition.mako.rs:10
(Diff revision 1)
> +/// Gecko's pseudo-element definition.
> +#[derive(Clone, Debug, PartialEq, Eq, Hash)]
> +pub enum PseudoElement {
> +    % for pseudo in PSEUDOS:
> +        /// ${pseudo.value}
> +        ${pseudo.original_ident[0].upper() + pseudo.original_ident[1:]},

You added `capitalized` method to pseudos, which can probably be used here?

::: servo/components/style/gecko/pseudo_element_definition.mako.rs:51
(Diff revision 1)
> +    /// Whether this pseudo-element is an anonymous box.
> +    #[inline]
> +    fn is_anon_box(&self) -> bool {
> +        match *self {
> +            % for pseudo in PSEUDOS:
> +                PseudoElement::${pseudo.capitalized()} => ${str(pseudo.type() == "nsICSSAnonBoxPseudo").lower()},

`is_anon_box()`?

::: servo/components/style/gecko/pseudo_element_definition.mako.rs:109
(Diff revision 1)
> +    #[inline]
> +    pub fn from_slice(s: &str, in_ua_stylesheet: bool) -> Option<Self> {
> +        use std::ascii::AsciiExt;
> +
> +        % for pseudo in PSEUDOS:
> +            if !${str(pseudo.is_anon_box()).lower()} || in_ua_stylesheet {

There are some non-anon-box pseudo-elements which can only appear in UA stylesheets as well.

I guess this is more an optimization? So probably doesn't matter too much.

::: servo/components/style/gecko/pseudo_element_definition.mako.rs:110
(Diff revision 1)
> +    pub fn from_slice(s: &str, in_ua_stylesheet: bool) -> Option<Self> {
> +        use std::ascii::AsciiExt;
> +
> +        % for pseudo in PSEUDOS:
> +            if !${str(pseudo.is_anon_box()).lower()} || in_ua_stylesheet {
> +                if s.eq_ignore_ascii_case("${pseudo.value[1:]}") {

I'm not sure how Rust handles string literals. I wonder would it save some code side if you do `"${pseudo.value}"[1..]`?
Attachment #8867556 - Flags: review- → review+
Comment on attachment 8867556 [details]
Bug 1364412: Convert pseudo-elements to an enum.

https://reviewboard.mozilla.org/r/139098/#review142532

> This is not necessary. regen_atoms.py is invoked by build_gecko.rs and thus should be updated at build-time automatically. And anyone who needs to update the in-tree files just need to extract the stylo-bindings.zip from TreeHerder, and all files in gecko/generated are included in it.

Right, I meant to update the in-tree bindings. I'll reword it.

> You added `capitalized` method to pseudos, which can probably be used here?

Heh, I added capitalized when I saw myself repeating this too often, I guess I forgot to update the ones I had already written, good catch :)

> `is_anon_box()`?

Same as above, thanks :)

> There are some non-anon-box pseudo-elements which can only appear in UA stylesheets as well.
> 
> I guess this is more an optimization? So probably doesn't matter too much.

We apparently aren't reading the flags, so we are (and were) exposing the `CSS_PSEUDO_ELEMENT_UA_SHEET_ONLY` pseudos.

I need to expose the flags to reject pseudos as well, so I'll ensure this gets fixed too.

> I'm not sure how Rust handles string literals. I wonder would it save some code side if you do `"${pseudo.value}"[1..]`?

I'd expect any compiler to optimize this into a slice pointing to the full string... I could check I guess.
Comment on attachment 8867276 [details]
Bug 1364412: Use the pseudo selector to reject state selectors.

https://reviewboard.mozilla.org/r/138814/#review142480

> Actually, Gecko only allows a single pseudo-class, apparently. I'll fix the parsing to only allow one instead.

This is actually wrong, gecko allows multiple selectors here, so I'll fix that and add a test.
Comment on attachment 8867717 [details]
Bug 1364412: Expose pseudo-element flags, and properly reject pseudos in non-UA sheets.

https://reviewboard.mozilla.org/r/139292/#review142616
Attachment #8867717 - Flags: review?(bobbyholley) → review+
Comment on attachment 8867718 [details]
Bug 1364412: Properly reject to parse pseudo-element states that don't support state.

https://reviewboard.mozilla.org/r/139294/#review142622
Attachment #8867718 - Flags: review?(bobbyholley) → review+
Comment on attachment 8867506 [details]
Bug 1364412: Properly handle state restyle hints for pseudo-elements.

https://reviewboard.mozilla.org/r/139046/#review142624

Per IRC discussion, I'd like to try avoiding the current complexity of sharing compute_hint_for_element_with_map.
Attachment #8867506 - Flags: review?(bobbyholley) → review-
Comment on attachment 8867506 [details]
Bug 1364412: Properly handle state restyle hints for pseudo-elements.

https://reviewboard.mozilla.org/r/139046/#review142658

::: servo/components/style/restyle_hints.rs:626
(Diff revision 5)
>                  ss.visit(&mut visitor);
>                  index += 1; // Account for the simple selector.
>              }
>  
> +
> +            let pseudo_selector_affects_state =

Not sure what the "affects" refers to. Seems like this should be "pseudo_element_is_state_dependent" or something.

::: servo/components/style/restyle_hints.rs:647
(Diff revision 5)
> -                    if selector.pseudo_element.is_some() {
> -                        // TODO(emilio): use more fancy restyle hints to avoid
> -                        // restyling the whole subtree when pseudos change.
> -                        //
> +                    // FIXME(emilio): Be more granular about this. See the
> +                    // comment in `PseudoElementDependency` about how could this
> +                    // be modified in order to be more efficient and restyle
> +                    // less.

I don't think we need this additional FIXME, given that we have the comment above, and given that there are various reasons why we're unlikely to fix this anytime soon.

::: servo/components/style/restyle_hints.rs:680
(Diff revision 5)
>      }
>  
>      /// Create an empty `DependencySet`.
>      pub fn new() -> Self {
> -        DependencySet(SelectorMap::new())
> +        DependencySet {
> +            element_dependencies: SelectorMap::new(),

I'd prefer to just call these 'dependencies', or if really necessary, 'normal' or 'regular' dependencies. Same with the struct name.

::: servo/components/style/restyle_hints.rs:686
(Diff revision 5)
> +    ///
> +    /// FIXME(emilio): Add also pseudo-element dependencies.
>      pub fn len(&self) -> usize {
> -        self.0.len()
> +        self.element_dependencies.len()

How about keeping track of a num_pseudo_dependencies on DependencySet, and incrementing that whenever we add a pseudo dependency (which should be rare)?

::: servo/components/style/restyle_hints.rs:747
(Diff revision 5)
> +        });
> +
> +        hint
> +    }
> +
> +    fn compute_element_hint<E>(

Similar to above, I'd prefer to just make this compute_hint, and have it early-return compute_pseudo_hint if there's a pseudo. An alternative way to look at this is that I'd like to inline compute_element_hint into compute_hint.

::: servo/components/style/restyle_hints.rs:758
(Diff revision 5)
>      {
>          debug_assert!(el.has_snapshot(), "Shouldn't be here!");
> -        let snapshot_el = ElementWrapper::new(el.clone(), snapshots);
>  
> -        let snapshot =
> -            snapshot_el.snapshot().expect("has_snapshot lied so badly");
> +        let wrapper = ElementWrapper::new(el.clone(), snapshots);
> +        let element_snapshot = wrapper.snapshot().unwrap();

Let's just call this snapshot like it was before. No need for the churn of renaming it.

::: servo/components/style/restyle_hints.rs:779
(Diff revision 5)
> -            snapshot.each_class(|c| if !el.has_class(c) { additional_classes.push(c.clone()) });
> +            element_snapshot.each_class(|c| {
> +                if !el.has_class(c) {
> +                    additional_classes.push(c.clone())
> +                }
> +            });
> +        }
> +
> +        map.lookup_with_additional(*el, additional_id, &additional_classes, &mut |dep| {
> +            trace!("scanning dependency: {:?}", dep);
> +            if !dep.sensitivities.sensitive_to(attributes_changed,
> +                                               state_changes) {
> +                trace!(" > non-sensitive");
> +                return true;
> -        }
> +            }
>  
> -        self.0.lookup_with_additional(*el, additional_id, &additional_classes, &mut |dep| {
> -            if !dep.sensitivities.sensitive_to(attrs_changed, state_changes) ||
> +            if hint.contains(dep.hint) {
> +                trace!(" > hint was already there");
> -               hint.contains(dep.hint) {
>                  return true;

It doesn't look like any of this changed. Can you revert it? Especially with complicated patches like this, I'd really prefer to skip drive-by reformatting.
Attachment #8867506 - Flags: review?(bobbyholley) → review+
Attachment #8867274 - Attachment is obsolete: true
Attachment #8867275 - Attachment is obsolete: true
Attachment #8867276 - Attachment is obsolete: true
Attachment #8867282 - Attachment is obsolete: true
Attachment #8867506 - Attachment is obsolete: true
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/21a5f7033f60
Simplify Servo_HasAuthorSpecifiedRules looking at the pseudo style. r=bholley
https://hg.mozilla.org/integration/autoland/rev/85726f8e31c5
Don't ignore the pseudo-element argument in ResolvePseudoElementStyle. r=heycam
https://hg.mozilla.org/integration/autoland/rev/03f9267f4d01
Reftest expectation updates. r=bholley
https://hg.mozilla.org/integration/autoland/rev/ff24d9958862
Convert pseudo-elements to an enum. r=hiro,xidorn
https://hg.mozilla.org/integration/autoland/rev/d05ac6d7ae31
Expose pseudo-element flags, and properly reject pseudos in non-UA sheets. r=bholley
https://hg.mozilla.org/integration/autoland/rev/651b63732c13
Properly reject to parse pseudo-element states that don't support state. r=bholley
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a7de58787e46
Update reftest and mochitest expectations for pseudo-elements selectors. r=me
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bd4e12a3fed9
Update reftest expectations for pseudo-elements selectors. r=me
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: