firefox-devtools / firefox-devtools/profiler

TreeView child selectors should be changed into descendant selectors

Open
#872 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
TypeScript
Stars
1.5k
Forks
491
Avg merge
3d 5h
Merged PRs (30d)
30

Description

e.g.

```css
.treeViewBody:focus > * > * > * > .treeViewRow.selected
```

becomes

```css
.treeViewBody:focus .treeViewRow.selected
```

## This is a follow-up from #855
> julienw 5 days ago Member
>
> wow these selectors are monsters :/

> mstange 5 days ago Owner
>
> The purpose of the long selector was to avoid the "descendant" selector and only use child selectors. That way, selector matching knows how many times to call node->GetParent() until it has a node that it needs to match again .treeViewBody:focus, and it doesn't need to check the entire ancestor chain if the tree view is not focused.
>
> Unfortunately, the new selector from this patch uses a descendant selector. Can it be changed to use a child selector?

> julienw 5 days ago Member
>
> I see. However I don't think this is worth it in this case because this will always apply for one element only, the one matching the class "selected".

> mstange 5 days ago Owner
>
> The cost of selector matching is not only paid for elements which match the selector, but also for elements which don't match the selector. If style recalculation wants to compute the style for an a element, it needs to find out whether the selector matches for this element. And with this selector, it walks up the entire ancestor chain of the a element to find out whether any of those ancestors matches .treeViewRow.selected, until it finds one.

> mstange 5 days ago Owner
>
> bholley explained to me that this isn't actually the case. When we match the a element in the example, and encounter this selector, we first check whether the class names treeViewRow and selected are in the current set of ancestor class names for this node (the set is actually a bloom filter), and if not, quick-reject the selector. This is a much better state than I thought we were in!
>
> As for the original selector, .treeViewBody:focus > * > * > * > .treeViewRow.selected, you were completely right: If this were a descendant selector, we'd only walk up the ancestor of nodes which already match .treeViewRow.selected, and there's at most one of those. So that selector should definitely be simplified to a descendant selector!

> julienw 5 days ago Member
>
> Yep, sorry for the previous terse comment, I was especially referring to the existing selector. Good to know we're in a good state for the other case too !

┆Issue is synchronized with this [Jira Task](https://mozilla-hub.atlassian.net/browse/FP-720)

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.