1345699 - stylo: Ensure that we never optimize out recascading in NAC subtrees

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

This isn't so much a work item as it is a reminder to make sure we don't screw this up when we start optimizing the cascade more. Since NAC inherits style from the first non-NAC ancestor, we need to make sure that we don't mistakenly conclude that our direct children can't have inherited any of our properties and then cull the cascade.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: Bug 1345699 - stylo: Always re-cascade in native anonymous subtrees.

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

Comment 2

[Cameron McCormack (:heycam)]

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

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8874352 [details]
Bug 1345699 - stylo: Always re-cascade in native anonymous subtrees.

https://reviewboard.mozilla.org/r/145716/#review149678

::: servo/components/style/traversal.rs:704
(Diff revision 1)
>          }
>      }
>  
> +    // We must always cascade native anonymous subtrees, since they inherit styles
> +    // from their first non-NAC ancestor.
> +    if element.is_native_anonymous() {

This is somewhat sad for `::before/::after` pseudo-elements I guess... Could we guard it with something like:

```
element.is_native_anonymous() && self.parent_element() != self.closest_native_anonymous_ancestor()
```

Anyway. r=me.

Is there a way to add a test for this? I guess it involves some of the number input pseudos, which may be annoying.

Comment 4

[Cameron McCormack (:heycam)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #3)
> Is there a way to add a test for this? I guess it involves some of the
> number input pseudos, which may be annoying.

Not only that, it requires a kind of restyle where we'll decide to stop cascading mid-way through the NAC subtree.  I'm putting that test in the too hard basket...

Comment 5

[Cameron McCormack (:heycam)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #3)
> This is somewhat sad for `::before/::after` pseudo-elements I guess... Could
> we guard it with something like:
> 
> ```
> element.is_native_anonymous() && self.parent_element() !=
> self.closest_native_anonymous_ancestor()
> ```

How does this help ::before / ::after?

Comment 6

[Emilio Cobos Álvarez (:emilio)]

It would avoid forcibly recascading those, since they wouldn't match that (their DOM parent is their closest non-NAC ancestor).

Of course I made a typo and the condition should read closest_non_native_anonymous_ancestor though.

Comment 7

[Cameron McCormack (:heycam)]

But isn't is the case for the root of any NAC subtree, that its parent is the closest non-NAC ancestor?

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Oh, sure, but aren't we force are cascading those? You're right that we could check the root flag instead.

Comment 9

[Cameron McCormack (:heycam)]

Per IRC discussion last night, we should move this check up into the "if compute_self" block so that we don't force cascade NAC subtrees when we didn't even restyle the originating element.

Comment 10

[Cameron McCormack (:heycam)]

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

Comment 11

[Cameron McCormack (:heycam)]

https://github.com/servo/servo/pull/17204