Closed Bug 1397644 Opened 7 years ago Closed 7 years ago

stylo: support matching for ::-moz-tree-* pseudo-elements

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(8 files, 1 obsolete file)

Store args of tree pseudo-elements as Atom.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Accept argument-less tree pseudo-element selector.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Add slots in PerPseudoElementMap for tree pseudos.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Make tree pseudos not precomputed since they are not really anonymous boxes.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Correct pseudo element type of tree pseudos.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1397644 part 1 - Implement XUL tree pseudo style resolution for stylo.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1397644 part 2 - Remove test annotations for this bug.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Servo PR
41 bytes, text/x-github-pull-request
Details | Review 
Parsing of these pseudo-elements was implemented in bug 1348488, and we would need to implement the full support of these pseudo-elements for using stylo in chrome.
Bug 1324698 probably depends on this.
Blocks: 1324698
Blocks: 1376898
Priority: -- → P4
Assignee: nobody → xidorn+moz
Depends on: 1407843
Depends on: 1409576
No longer depends on: 1409576
So... we probably need to invoke a special matching function in selectors::matching::matches_complex_selector somehow, so that we can do the special check for XUL tree pseudo.

There are several options:

1. Add a function reference (as trait object reference) to MatchingContext which takes a pseudo-element and returns whether it matches. Conceptually this matches the old Gecko approach before bug 1407843. But this is probably very hard, because MatchingContext is not generic over SelectorImpl trait, so it cannot contain any impl-specific bits in its definition.

2. Add a method to SelectorImpl to do the special matching, and put some context data in MatchingContext. It is the same concept of Gecko approach after bug 1407843. This would be either hard or unsafe as well, because of the same reason as (1): we cannot put impl-specific data in MatchingContext. We may put an opaque pointer there, but we would lose all the type-safety...

3. Add an optional function parameter like the "flags_setter", which captures the necessary information itself, all the way from get_pseudo_style down to matches_complex_selector. This would change a bunch of functions, and majority of them would be called with None on that parameter in other places. It seems rather unfortunate to change this many functions just for tree pseudo.

4. Change how we get the input words. Since we only request the style from nsTreeBodyFrame, which should always be the primary frame of <treechildren> element, we can probably add a function in nsTreeBodyFrame to get the array, and query that array via element when doing the matching. This way we would not need to pass any data with context, and thus we can probably reuse most of existing code and only need to add a new method to SelectorImpl. The down side is that it would add a dependency from style system to layout, which is quite unfortunate, but maybe acceptable?

I'm going to do a trial of (4) in Gecko and see if that works as expected. If so, we can implement this way in Stylo to avoid the annoyance of (1) ~ (3).
Depends on: 1409596
A variation to (2) would be putting an &Any in MatchingContext as extra data, and add a new method to SelectorImpl, then have that method dynamically cast the extra data to what we need.

(4) is not that easy since style resolving can be invoked before the frame is set as the primary frame. We can probably assert that in the case the element doesn't have the primary frame, the array is empty. This may be true for code there.

Also it seems to me that the array is usually populated just before the code tries to get the style context, so I guess conceptually that array should be considered as an extra local argument of selector matching rather than an element state, which means (4) probably doesn't make as much sense as I thought before.

Another option is to pass the Option<&PseudoElement> further down into matches_complex_selector, and we can construct a PseudoElement contains the information.
I talked with Xidorn about this, and I think that there's anything particularly hard about making MatchingContext not generic over SelectorImpl.

  https://github.com/servo/servo/pull/18934 is a simple patch that does it.
Comment on attachment 8920000 [details]
Store args of tree pseudo-elements as Atom.

https://reviewboard.mozilla.org/r/190960/#review196192

::: servo/components/style/gecko/selector_parser.rs:395
(Diff revision 1)
>              // separated by either comma or space.
>              let mut args = Vec::new();
>              loop {
>                  let location = parser.current_source_location();
>                  match parser.next() {
> -                    Ok(&Token::Ident(ref ident)) => args.push(ident.as_ref().to_owned()),
> +                    Ok(&Token::Ident(ref ident)) => args.push(Atom::from(ident.as_ref())),

I wonder if we should use `CustomIdent`, and call `CustomIdent::to_css` instead. That way we would need to change less places when we finally avoid converting to a rust string for `Atom`, wdyt?
Attachment #8920000 - Flags: review?(emilio) → review+
Comment on attachment 8920001 [details]
Accept argument-less tree pseudo-element selector.

https://reviewboard.mozilla.org/r/190962/#review196194

::: servo/components/style/gecko/selector_parser.rs:384
(Diff revision 1)
>      fn parse_pseudo_element(&self, location: SourceLocation, name: CowRcStr<'i>)
>                              -> Result<PseudoElement, ParseError<'i>> {
>          PseudoElement::from_slice(&name, self.in_user_agent_stylesheet())
> +            .or_else(|| {
> +                if name.starts_with("-moz-tree-") {
> +                    PseudoElement::tree_pseudo_element(&name, Box::new([]))

Should we instead use `Option<Box<>>`? Seems like that would've prevented the serialization bug above... or maybe not. Anyway, the other serialization patch should probably move here.

Also, do we want to parse them on content docs? (I'd rather don't, and doesn't seem a web-compat problem)
Attachment #8920001 - Flags: review?(emilio) → review+
Comment on attachment 8920002 [details]
Add slots in PerPseudoElementMap for tree pseudos.

https://reviewboard.mozilla.org/r/190964/#review196198
Attachment #8920002 - Flags: review?(emilio) → review+
Comment on attachment 8920003 [details]
Make tree pseudos not precomputed since they are not really anonymous boxes.

https://reviewboard.mozilla.org/r/190966/#review196204

::: servo/components/style/stylist.rs:1978
(Diff revision 1)
>                      for selector in &style_rule.selectors.0 {
>                          self.num_selectors += 1;
>  
>                          let map = match selector.pseudo_element() {
>                              Some(pseudo) if pseudo.is_precomputed() => {
>                                  if !selector.is_universal() ||

This should move back to be an assertion then:

`debug_assert!(matches!(origin, Origin::UserAgent))`,

You should probably keep the `!selector.is_universal()` though.
Attachment #8920003 - Flags: review?(emilio) → review+
Comment on attachment 8920004 [details]
Correct pseudo element type of tree pseudos.

https://reviewboard.mozilla.org/r/190968/#review196208
Attachment #8920004 - Flags: review?(emilio) → review+
Comment on attachment 8920005 [details]
Bug 1397644 part 1 - Implement XUL tree pseudo style resolution for stylo.

https://reviewboard.mozilla.org/r/190970/#review196210

r=me

::: commit-message-2c530:1
(Diff revision 1)
> +Bug 1397644 part 1 - Implement XUL tree pseudo style resolving for stylo. r?emilio

"style resolution" instead of "style resolving", maybe?

::: layout/style/ServoStyleSet.cpp:715
(Diff revision 1)
> +{
> +  MOZ_ASSERT(nsCSSAnonBoxes::IsTreePseudoElement(aPseudoTag));
> +  MOZ_ASSERT(aParentContext);
> +  MOZ_ASSERT(!StylistNeedsUpdate());
> +
> +  RefPtr<ServoStyleContext> style =

I think you can just:

```
return Servo_ResolveXULTreePseudoStyle(
    aParentElement,
    ...
).Consume();
```

if you want. Not sure even if it's cleaner, but thought I'd mention it.

::: layout/style/StyleSetHandle.h:149
(Diff revision 1)
>      inline already_AddRefed<nsStyleContext>
>      ResolveInheritingAnonymousBoxStyle(nsAtom* aPseudoTag,
>                                         nsStyleContext* aParentContext);
>      inline already_AddRefed<nsStyleContext>
>      ResolveNonInheritingAnonymousBoxStyle(nsAtom* aPseudoTag);
> +#ifdef MOZ_XUL

Is it worth to keep the `MOZ_XUL` ifdefs? I suspect firefox doesn't build without it :)

::: servo/components/selectors/context.rs:114
(Diff revision 1)
>      /// The current nesting level of selectors that we're matching.
>      pub nesting_level: usize,
>  
> +    /// An optional hook function for checking whether a pseudo-element
> +    /// should match when matching_mode is ForStatelessPseudoElement.
> +    pub pseudo_element_matching: Option<&'a Fn(&Impl::PseudoElement) -> bool>,

Maybe `stateless_pseudo_matching_function`? Not sure I love it, but `pseudo_element_matching` doesn't say a lot about it.

Maybe `pseudo_element_matching_function`, at least?

::: servo/components/style/stylist.rs:819
(Diff revision 1)
>      ) -> Option<Arc<ComputedValues>>
>      where
>          E: TElement,
>      {
>          let cascade_inputs =
> -            self.lazy_pseudo_rules(guards, element, pseudo, is_probe, rule_inclusion);
> +            self.lazy_pseudo_rules(guards, element, pseudo, is_probe,

nit: Use block indentation for the arguments:

```
self.lazy_pseudo_rules(
    guards,
    element,
    pseudo,
    is_probe,
    rule_inclusion,
    matching_func,
);
```

::: servo/ports/geckolib/glue.rs:1869
(Diff revision 1)
>              Strong::null()
>          }
>      }
>  }
>  
> +fn debug_atom_array(atoms: &AtomArray) -> String {

Maybe `assert!(cfg!(debug_assertions))` here?

::: servo/ports/geckolib/glue.rs:1889
(Diff revision 1)
> +pub extern "C" fn Servo_ComputedValues_ResolveXULTreePseudoStyle(
> +    element: RawGeckoElementBorrowed,
> +    pseudo_tag: *mut nsAtom,
> +    inherited_style: ServoStyleContextBorrowed,
> +    input_word: *const AtomArray,
> +    raw_data: RawServoStyleSetBorrowed) -> ServoStyleContextStrong

nit: The return value should be formatted like:

```
) -> ServoStyleContextStrong {
```
Attachment #8920005 - Flags: review?(emilio) → review+
Comment on attachment 8920006 [details]
Don't serialize parens when tree pseudo don't have argument.

https://reviewboard.mozilla.org/r/190972/#review196222
Attachment #8920006 - Flags: review?(emilio) → review+
Comment on attachment 8920007 [details]
Bug 1397644 part 2 - Remove test annotations for this bug.

https://reviewboard.mozilla.org/r/190974/#review196224
Attachment #8920007 - Flags: review?(emilio) → review+
Comment on attachment 8920005 [details]
Bug 1397644 part 1 - Implement XUL tree pseudo style resolution for stylo.

https://reviewboard.mozilla.org/r/190970/#review196210

> Is it worth to keep the `MOZ_XUL` ifdefs? I suspect firefox doesn't build without it :)

I'm 80% confident that Fennec builds without `MOZ_XUL` so we need to keep it. Actually we would probably need to add some xul flag to geckolib at some point as well for Fennec I suppose.

> Maybe `assert!(cfg!(debug_assertions))` here?

How is that useful? I suppose log wouldn't run this function if `RUST_LOG` isn't set for this module. Also `ResolveXULTreePseudoStyle` wouldn't be called very often (because layout code caches the result), so I'm not too concerned about this becoming a big noise.
Comment on attachment 8920000 [details]
Store args of tree pseudo-elements as Atom.

https://reviewboard.mozilla.org/r/190960/#review196192

> I wonder if we should use `CustomIdent`, and call `CustomIdent::to_css` instead. That way we would need to change less places when we finally avoid converting to a rust string for `Atom`, wdyt?

That might be a good idea, but `CustomIdent` maps to a well-defined concept in CSS (the `<custom-ident>`), which excludes certain keywords. Although those keywords may not appear here, I'm not sure whether we want to mix them... I suppose we don't.
Comment on attachment 8920001 [details]
Accept argument-less tree pseudo-element selector.

https://reviewboard.mozilla.org/r/190962/#review196194

> Should we instead use `Option<Box<>>`? Seems like that would've prevented the serialization bug above... or maybe not. Anyway, the other serialization patch should probably move here.
> 
> Also, do we want to parse them on content docs? (I'd rather don't, and doesn't seem a web-compat problem)

Yeah, probably we can move that patch here... That patch lives there simply because I found it after I finish the main part (part 1), and I reuse one function added in the main part. Maybe I can merge both into this patch.

For content docs, I think we do? We haven't unship tree pseudos (see bug 1379031). This specific case may not matter, but we probably want to keep consistent on tree pseudos for both non-functional and functional.

We should unship them together at some point...
Comment on attachment 8920003 [details]
Make tree pseudos not precomputed since they are not really anonymous boxes.

https://reviewboard.mozilla.org/r/190966/#review196204

> This should move back to be an assertion then:
> 
> `debug_assert!(matches!(origin, Origin::UserAgent))`,
> 
> You should probably keep the `!selector.is_universal()` though.

I think we can assert both...
Attached file Servo PR — 

    
  
Blocks: 1409604

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8920006 -
        Attachment is obsolete: true

    
    

    
  
We're sorry - something has gone wrong while rewriting or rebasing your commits. The commits being pushed no longer match what was requested. Please file a bug.

    
    

    
  
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/372b5056da25
part 1 - Implement XUL tree pseudo style resolution for stylo. r=emilio
https://hg.mozilla.org/integration/autoland/rev/5fb43b31a8f7
part 2 - Remove test annotations for this bug. r=emilio

    
  
Blocks: 1409884

    
  
Blocks: 1409630

    
  
Blocks: 1409356

    
    

    
  
I'm assuming there's no intent to try uplifting this to 57 at this point.

          status-firefox57:
          affected → wontfix

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: