586974 - Inspector style panel ranks all unmatched rules the same

Status: RESOLVED WONTFIX

(DevTools :: General, defect)

Description

[Joe Walker [:jwalker]]

Some unmatched rules are a closer match than others, we should find a way to rank the close matches higher than the unmatched ones.

For example,

If the selected element is body > div.foo > p#id
Then the selector "div.foo > p#is" is clearly more relevant than the "img" selector.

We should be able to display the rules marking which parts of the selector they failed on, and sort them by relevance.

Comment 1

[Mihai Sucan [:msucan]]

We could do this as follows:

1. Determine the "path" to the highlighted element with that exact "thought process". An array:

elementPath = [
{tag: "body"},
{tag: "div", classNames: ["foo"]},
{tag: "p", id: "id"}
]

2. Break the rule.selector into an array of same format:

selectorArray = [
{tag: "div", classNames: ["foo"]},
{tag: "p", id: "is"}
]

or [ {tag: "img" } ] (for the "img" selector).

3. Compare the two arrays against each other. The one that matches more array elements wins in sorting.

Problem: The above approach doesn't make a difference between "body > div" or "body div". Still, I think we can cook something up. Something like:

selectorArray = [
{tag: "body", next: 1},
{tag: "div", classNames: ["foo"], next: 2},
{tag: "p", id: "is"}
]
( for selector "body > div.foo > p#is" )

versus:

selectorArray = [
{tag: "body"},
{tag: "div", classNames: ["foo"]},
{tag: "p", id: "is"}
]
( for selector "body div.foo p#is" )

When comparison is done, we can check if the selectorArray element has a next, if yes, we jump to that, and we make a strict check against the next element in elementPath. If the check fails, we stop the algorithm. Otherwise we continue until one of the arrays reaches an end.

If the selectorArray does not have a next, we stay on the same element. We just iterate through the next element in elementPath array. When we find a match, *then* we move to the next element in the selectorArray.

We should apply this sorting even for matched rules that have equal specificity(I think that's not often, but still).

Any thoughts?

Comment 2

[Joe Walker [:jwalker]]

The way the firefox does the matching is like this. Assume that we're matching the selector "p#foo > div .bar":

1. split into simple selectors
"p#foo > div .bar" -> [ "p#foo", combinator_>, "div", combinator_space, ".bar" ]

2. create a list of nodes matching the last simple selector

3. filter this list using the previous combinator and simple selector

4. repeat step 3 until we are at the start of the list

It's easy to create indexes to make most of the lookups done by step 2 fast, however it's hard to do a similar thing for step 3 because it all depends where you are coming from. (which is why rules like ".class *" are really slow).

I'm not sure about other browsers, but if this is a universal algorithm then there could be benefits to using this in a sorting system - I think it's a good thing if more devs understand how their browsers are working.

An alternative to the algorithm presented by Mihai could involve repeating this process from the end, marking the simple selectors match/miss, and then sorting by length of match.

In many respects this is a worse algorithm because a miss in the last simple selector would give whole selector a low score. It's only redeeming feature is that it might give an insight into how browsers do things.

Comment 3

[Joe Walker [:jwalker]]

If there is anything to drop from our list for firefox4, this would be it from my point of view.

Happy for dissent though.

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

So if I understand this correctly it is about the order of rules in the CSS Doctor rather than specificity in the CSS Inspector?

Comment 5

[Joe Walker [:jwalker]]

No.

We've dig inside "Fonts and Color" (or whatever it's called) and inside "background-color" and then inside the unmatched rules. They are just presented in an essentially random list. It would be nice if they could be presented sorted so that the ones at the top were more likely to be the ones the user wanted to be in the matching set.

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Upon investigation I realized that there are quite a lot of selectors that we should take into account to accomplish this:
*
E
.info
#footer
E F
E > F
E + F
E[att]
E[att=val]
E[att~=val]
E[att|=val]
E:first-child
E:link
E:visited
E:active
E:hover
E:focus
E:lang(c)
E:first-line
E:first-letter
E:before
E:after

Obviously, * would never be in the unmatched list but we need to ensure that the most likely to fail are high in the list.

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Unmatched property order patch

The logic I eventually followed is as follows:
1. Sort into selector groups (descendant, adjacent, child, attribute, pseudo class and pseudo element) ... in my opinion this is the order in which errors are most likely to appear.
2. Sort a second time moving all selectors to the top where either the node name, id or class is identified.

It is probably possible to do this in one pass but then the code would be far more complex and less manageable.

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Screenshot

Here is an example of the unmatched rule ordering. The top rule is unmatched because container is the id of the containing element and not the class name. The second rule is unmatched because the element was the first span element in a sequence and not the second.

All the rules that reference either the currently selected elements node name, id or class are at the top of the list. From t3.css:41 and onwards there are no close matches.

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Unmatched property order patch

Rebased patch

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Unmatched property order patch

Fixed scoping issue

Comment 11

[Mihai Sucan [:msucan]]

Comment on attachment 541636 [details] [diff] [review]
Unmatched property order patch

Review of attachment 541636 [details] [diff] [review]:
-----------------------------------------------------------------

I like this, but I think we want something better. In the sense that, sure, we should not overly complicate the patch, but I believe we can easily make a better ranking method.

Check on a big web site with lots of styling: at the moment I see tags mixed with IDs in sorting, even id-based selectors themselves have no correct sorting between them. It's not clear which selector is "more relevant". It's not easy for the user to "see" most-specific unmatched selectors.

What I suggest: split the selector text into simple selectors: .class.names, #ids, tagNames, :pseudos, [attributes]. Ignore how they are combined (>, + whatever). Build a score - the more of these match, the higher the score. Sort based on that. If the two selectors are equal at the end, sort alphabetically, so, for example tag names or #ids, .classnames are consecutively ordered.

You can do this in one step, and it should be easy, and clear to the user as well.

Thoughts? Rob, Joe, what do you guys think?

::: browser/base/content/csslogic.js
@@ +1365,5 @@
> +    let self = this;
> +    this._unmatchedSelectors.sort(function(aSelectorInfo1, aSelectorInfo2) {
> +      let thisStyleText = aSelectorInfo1.selector.text.toLowerCase();
> +      let thatStyleText = aSelectorInfo2.selector.text.toLowerCase();
> +      let elt = self._cssLogic.viewedElement;

Please use a clearer variable name, like "element" or "node".

@@ +1371,5 @@
> +      let id = elt.id;
> +      let classNames = elt.className.trim();
> +
> +      if (classNames) {
> +        classNames = classNames.split(" ");

You do not need to do this. You have the elt.classList API.

@@ +1385,5 @@
> +      }
> +
> +      let rx = new RegExp(pattern.toLowerCase());
> +      let match1 = rx.test(thisStyleText);
> +      let match2 = rx.test(thatStyleText);

If an #id AND a class name matches the element in one selector, but in the second selector only a class name matches ... then the two selectors are considered equal, which is not really true.

@@ +1649,5 @@
> +      }
> +    };
> +
> +    if (!this.compareToUnmatched.selectorMatchers) {
> +      this.compareToUnmatched.selectorMatchers = {

You can avoid this check. Just put it outside of the method.

CssSelectorInfo.selectorMatchers = {... the list ...};

@@ +1662,5 @@
> +
> +    for each (let matcher in this.compareToUnmatched.selectorMatchers) {
> +      let result = selectorHelper(matcher);
> +      if (typeof result != "undefined") {
> +        return result;

The choice of using typeof result != "undefined" makes the code confusing.

Also, I believe that sorting selectors into such groups is visually confusing, with no specific benefit. Please provide examples how this sorting makes things clearer to the user. (maybe i haven't found the right examples)

Comment 12

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Actually, a scored sorting system is probably better. As long as it is possible to cover all cases from comment 6 it would be great.

> What I suggest: split the selector text into simple selectors: .class.names, #ids, tagNames, :pseudos, [attributes]. Ignore how they are combined (>, + whatever). Build a score - the more of these match, the higher the score. Sort based on that. If the two selectors are equal at the end, sort alphabetically, so, for example tag names or #ids, .classnames are consecutively ordered.

I guess we could weight the scores according to how far down the chain we are ... I like your idea, I will work on that when I get chance.

Comment 13

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #11)
> Comment on attachment 541636 [details] [diff] [review] [review]
> Unmatched property order patch
> 
> Review of attachment 541636 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> I like this, but I think we want something better. In the sense that, sure,
> we should not overly complicate the patch, but I believe we can easily make
> a better ranking method.

I agree that we need something better for unmatched rule sorting.

I think the most-likely reason someone will be looking through the unmatched rules is to find a rule that they expected to be applied to a given node to see why it's not there. The current list is a bit of a jumble, I think iterating in stylesheet order? (correct me if I'm wrong, Mike or Mihai.

> Check on a big web site with lots of styling: at the moment I see tags mixed
> with IDs in sorting, even id-based selectors themselves have no correct
> sorting between them. It's not clear which selector is "more relevant". It's
> not easy for the user to "see" most-specific unmatched selectors.
> 
> What I suggest: split the selector text into simple selectors: .class.names,
> #ids, tagNames, :pseudos, [attributes]. Ignore how they are combined (>, +
> whatever). Build a score - the more of these match, the higher the score.
> Sort based on that. If the two selectors are equal at the end, sort
> alphabetically, so, for example tag names or #ids, .classnames are
> consecutively ordered.

I think this makes sense, and is probably the way most CSS devs are going to want to look for them. Especially if you can expand and collapse them though that might be getting more complicated than we really want.

Let's do something like this in a follow-up bug. I don't want to block review on the style inspector for just this.

Comment 14

[Rob Campbell [:rc] (:robcee)]

> Let's do something like this in a follow-up bug. I don't want to block
> review on the style inspector for just this.

actually, talking to mihai, it probably makes sense to just do this chunk right here. Move expanding and collapsing to a follow-up. Thanks!

Comment 15

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I think that you are correct that at the moment everything is in stylesheet order. This patch simply sorts the selectors into groups and then moves any rules that contain the nodeName, class or id of the node to the top of the list.

I have no problem with leaving this bug out for now and adding the sorting when we have a better system, it does not block the style inspector. I will work on this after the panel manager is finished.

Comment 16

[Mihai Sucan [:msucan]]

Thank you Rob for your comments!

Mike: agreed, when you have the updated patch, please ask me for review again. Thanks!

Comment 17

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

We no longer use unmatched rules. Closing bug.

Bug triage, filter on PEGASUS.

Comment 18

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Bug triage, filter on PEGASUS.