1371577 - stylo: warning: Reframing due to lack of old style source: <resizer>

Status: RESOLVED FIXED

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

Description

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

After Bug 1290276 landed, loading "data:text/html,<textarea>" gives the following warning.

WARN:style::matching: Reframing due to lack of old style source: <resizer> (0x195988c10), pseudo: Some(FirstLine)
WARN:style::matching: Reframing due to lack of old style source: <resizer> (0x195988c10), pseudo: Some(FirstLetter)

I don't remember I had seen this warning during my development of bug 1290276. Perhaps this is due to recent change?

Comment 1

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

The resizer will resolve its style again after loading its xbl binding [1] at [2]. 

Emilio, any idea about this warning? 

[1] http://searchfox.org/mozilla-central/rev/a798ee4fc323f9387b7576dbed177859d29d09b7/toolkit/content/minimal-xul.css#78
[2] Added in bug 1290276 Part 7. https://hg.mozilla.org/mozilla-central/annotate/42720e3e88b6/layout/base/nsCSSFrameConstructor.cpp#l5823

Comment 2

[Emilio Cobos Álvarez (:emilio)]

That is expected for now when there are first-line or first-letter rules.

Though I find surprising we have rules matching ::first-line and ::first-letter on the resizer, so probably worth looking into why's that the case.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

So that's because from push_applicable_declarations, we call             

> rule_hash_target.get_declarations_from_xbl_bindings(applicable_declarations);

unconditionally, regardless of the `pseudo` parameter, so we end up doing selector-matching for the normal resizer instead of for the pseudo.

So, some thoughts:

 * First, why using rule_hash_target there and not `element`? They're usually the same, except when pseudos are at play. It looks wrong to me to use rule_hash_target, since the pseudo-element style would be incorrect.

 * Second, what should we do about pseudo-elements in XBL? Right now you don't pass the `pseudo` down and always use element_map, which is clearly wrong in this case. If we're fine with not supporting pseudos in XBL we can check if pseudo.is_some() and early return, otherwise you really need to check the pseudo map in push_applicable_declarations_as_xbl_only_stylist when appropriate.

Comment 4

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #3)
> So that's because from push_applicable_declarations, we call             
> 
> > rule_hash_target.get_declarations_from_xbl_bindings(applicable_declarations);
> 
> unconditionally, regardless of the `pseudo` parameter, so we end up doing
> selector-matching for the normal resizer instead of for the pseudo.
> 
> So, some thoughts:
> 
>  * First, why using rule_hash_target there and not `element`? They're
> usually the same, except when pseudos are at play. It looks wrong to me to
> use rule_hash_target, since the pseudo-element style would be incorrect.

I remember I had use `element` in my first WIP in Bug 1290276, but it fails some of the reftests, so I try rule_hash_target instead. Anyway, using `element` seems reasonable to me.

>  * Second, what should we do about pseudo-elements in XBL? Right now you
> don't pass the `pseudo` down and always use element_map, which is clearly
> wrong in this case. If we're fine with not supporting pseudos in XBL we can
> check if pseudo.is_some() and early return, otherwise you really need to
> check the pseudo map in push_applicable_declarations_as_xbl_only_stylist
> when appropriate.

I think we'll need to support pseudos in XBL. I'm working on a patch to fix this.

Comment 5

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: stylo: Fix pseudo element matching for XBL stylesheets (Bug 1371577)

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

Comment 6

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1371577 - stylo: Update test expectations after fixing pseudo element matching for XBL stylesheets.

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

Comment 7

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8877487 [details]
stylo: Fix pseudo element matching for XBL stylesheets (Bug 1371577)

https://reviewboard.mozilla.org/r/148018/#review153380

::: servo/components/style/stylist.rs:1091
(Diff revision 1)
>              debug!("skipping user rules");
>          }
>  
>          // Step 3b: XBL rules.
>          let cut_off_inheritance =
> -            rule_hash_target.get_declarations_from_xbl_bindings(applicable_declarations);
> +            rule_hash_target.get_declarations_from_xbl_bindings(pseudo_element,

Re comment 4:

If I change `rule_hash_target` to `element` here, reftests for `::placeholder` will fail like https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2754e13b621fe427a8a1c31e8d96c414fb68f24&selectedJob=106298799

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8877487 [details]
stylo: Fix pseudo element matching for XBL stylesheets (Bug 1371577)

https://reviewboard.mozilla.org/r/148018/#review153384

r=me, with comments addressed.

::: servo/components/style/stylist.rs:937
(Diff revision 1)
>          // mode info in the `SharedLayoutContext`.
>          self.quirks_mode = quirks_mode;
>      }
>  
> +    /// Returns the correspond PerPseudoElementSelectorMap given PseudoElement.
> +    fn get_element_map(&self,

Also, the name doesn't seem particularly descriptive, perhaps get_pseudo_map, get_selector_map, or just get_map is a better name?

::: servo/components/style/stylist.rs:938
(Diff revision 1)
>          self.quirks_mode = quirks_mode;
>      }
>  
> +    /// Returns the correspond PerPseudoElementSelectorMap given PseudoElement.
> +    fn get_element_map(&self,
> +                   pseudo_element: Option<&PseudoElement>) -> &PerPseudoElementSelectorMap

nit: alignment is off here.

Also, I think this should just return an `Option<&PerPseudoElementMap>`, and remove the assertion, so you'd write something like:

```
match pseudo_element {
    Some(pseudo) => self.pseudos_map.get(pseudo),
    None => Some(&self.element_map),
}
```

See below for why.

::: servo/components/style/stylist.rs:950
(Diff revision 1)
> +        }
> +    }
> +
> +    /// Returns the rule hash target given an element.
> +    fn rule_hash_target<E>(&self,
> +                           element: &E) -> E

nit: This signature fits all in one line. Also, it doesn't hurt to take `E` by value, they're just pointers.

::: servo/components/style/stylist.rs:951
(Diff revision 1)
> +    }
> +
> +    /// Returns the rule hash target given an element.
> +    fn rule_hash_target<E>(&self,
> +                           element: &E) -> E
> +        where E: TElement {

nit: move the brace to the beginning of the next line, like in the rest of the file.

::: servo/components/style/stylist.rs:958
(Diff revision 1)
> +            element.implemented_pseudo_element().is_some();
> +
> +        // NB: This causes use to rule has pseudo selectors based on the
> +        // properties of the originating element (which is fine, given the
> +        // find_first_from_right usage).
> +        let rule_hash_target = if is_implemented_pseudo {

nit: You don't need to add the extra variable, just:

```
if is_implemented_pseudo {
    element.closest_non_native_anonymous_ancestor().unwrap()
} else {
    *element
}
```

::: servo/components/style/stylist.rs:981
(Diff revision 1)
>          let mut matching_context =
>              MatchingContext::new(MatchingMode::Normal, None, self.quirks_mode);
>          let mut dummy_flag_setter = |_: &E, _: ElementSelectorFlags| {};
>  
> -        self.element_map.author.get_all_matching_rules(element,
> -                                                       element,
> +        // Bail out if we don't have the selector map of the pseudo element.
> +        if pseudo_element.is_some() && self.pseudos_map.get(pseudo_element.unwrap()).is_none() {

You're doing an extra hashmap lookup here for no good reason, so I think we should change the get_map to return an option, and do here instead:

```
let map = match self.get_map(pseudo) {
    Some(map) => map,
    None => return,
};
```

::: servo/components/style/stylist.rs:1028
(Diff revision 1)
>          debug_assert!(cfg!(feature = "gecko") ||
>                        style_attribute.is_none() || pseudo_element.is_none(),
>                        "Style attributes do not apply to pseudo-elements");
>          debug_assert!(pseudo_element.map_or(true, |p| !p.is_precomputed()));
>  
> -        let map = match pseudo_element {
> +        let map = self.get_element_map(pseudo_element);

Similarly, I think right now there could be the (unlikely) case of a pseudo-element being only in XBL sheets, and not in the main document ones, so we should also early return instead of doing `get_map(pseudo).unwrap()`.

::: servo/components/style/stylist.rs:1091
(Diff revision 1)
>              debug!("skipping user rules");
>          }
>  
>          // Step 3b: XBL rules.
>          let cut_off_inheritance =
> -            rule_hash_target.get_declarations_from_xbl_bindings(applicable_declarations);
> +            rule_hash_target.get_declarations_from_xbl_bindings(pseudo_element,

Oh, I think I know why that is, could you file a bug and ni? me for that? I'll clean that up.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8877488 [details]
Bug 1371577 - stylo: Update test expectations after fixing pseudo element matching for XBL stylesheets.

https://reviewboard.mozilla.org/r/148932/#review153396

Nice :)

::: dom/xul/crashtests/crashtests.list:11
(Diff revision 1)
>  load 326644-1.html
>  load 326644-2.html
>  load 326864-1.xul
>  load 326875-1.xul
>  load 326881-1.xul
> -asserts-if(stylo,4) load 329982-1.xhtml # bug 1370830
> +load 329982-1.xhtml # bug 1370830

nit: remove the bug comment?

Comment 11

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8877488 [details]
Bug 1371577 - stylo: Update test expectations after fixing pseudo element matching for XBL stylesheets.

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

Comment 12

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Servo PR #17318

Comment 13

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7710bc9cca30
stylo: Update test expectations after fixing pseudo element matching for XBL stylesheets. r=emilio

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/7710bc9cca30