592743 - Style inspector specificity calculator makes mistakes

Status: RESOLVED FIXED

(DevTools :: General, defect, P2)

Description

[Joe Walker [:jwalker]]

It should not count the ":not(...)" pseudo-class with the classes.
So "a:not(#foo)" has a specificity of ids=0, classes=0, tags=1

The comment "Pseudo elements count as elements" should read "Pseudo classes count as classes", and the following line should add to .classes and not to .tags

Pseudo elements (::) should be added to tags - currently they are ignored. We need to find a way to distinguish between pseudo elements and pseudo classes.

The selector string should split on /[ >+~]/

We should pre-parse the selector string to replace "/\[.*\]/\[\]/g" in other words to remove the contents of attribute filters so [a=b] becomes []. This will prevent ~, | or * which are allowed in attribute filters to assume other meanings.

We should check that namespace|div is counted as div. I am fairly sure that it is, but it's worth checking.

Comment 1

[Joe Walker [:jwalker]]

Attachment: Visual map of selector spec

While digging through the spec I made some notes, which I've tidied up and scanned in case anyone else needs to do the same thing, or in case I lose the original.

Comment 2

[Justin Dolske [:Dolske]]

Marking blocking+, with the rationale that if we ship Firefox 4 with a CSS Inspector, it really needs to behave correctly. Having the browser and inspector disagree about how an element is being styled would be terribly confusing.

Comment 3

[Kevin Dangoor]

Inspector feature postponed. Removing blocking flags from Inspector bugs.

Comment 4

[Kevin Dangoor]

Removing items from kd4b6.

Comment 5

[Kevin Dangoor]

Reprioritizing bugs. You can filter the mail on the word TEABAGS.

Comment 6

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

Joe said that the CSS Doctor does a much better job of explaining specificity so we will be removing specificity from the Style Inspector.

Closing this bug as invalid in favor of bug 653084

Comment 7

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

Reopened because specificity is needed by the style inspector to discover which is the currently applied rule for a CSS property.

Comment 8

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

We should also consider some of the fixes in https://bug585565.bugzilla.mozilla.org/attachment.cgi?id=471904 here

Comment 9

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

We should be using exactly the same rules to calculate css specificity as Firefox because the style inspector and browser should *never* disagree.

Comment 10

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

Looking at nsCSSSelector::CalcWeightWithoutNegations it seems like Firefox fails to calculate -moz-any() specificity correctly (bug 561154).

Thinking about this, it is probably better if we could expose specificity through nsIDOMCSSRule, this way we would be less prone to future bugs, we could use the same weighting as Fx and the style inspector would be showing what the browser is actually doing.

Comment 11

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

FWIW: Here is the scoring from nsCSSSelector::CalcWeightWithoutNegations:
weight = 0
if (tag)
  weight += 1
if (id)
  weight += 65536
if (class)
  weight += 256
if (pseudoclass)
  weight += 256
if (pseudoelement)
  weight += 256

Comment 12

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

Bug 682318 logged.

Comment 13

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

Bug triage, filter on PEGASUS.

Comment 14

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

Seems like the platform code has been changed slightly:
  let weight = 0;

  for each (tag in selector) {
    weight += 0x000001;
  }
  for each (id in selector) {
    weight += 0x010000;
  }

  // FIXME (bug 561154):  This is incorrect for :-moz-any(), which isn't
  // really a pseudo-class.  In order to handle :-moz-any() correctly,
  // we need to compute specificity after we match, based on which
  // option we matched with (and thus also need to try the
  // highest-specificity options first).

  for each (class || pseudoClass || pseudoElement  in selector) {
    weight += 0x000100;
  }

In the absence of an API to get a CSSSelector's specificity we can use this algorithm.

Comment 15

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

Attachment: Patch

This covers everything, including pseudo-classes and pseudo-elements. Joe, I have used replace() as opposed to simply counting the strings as this method is less error prone.

Comment 16

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

Attachment: Patch 2

Removed hilariously obvious bug.

Comment 17

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

Attachment: Patch 3

What the heck is wrong with me? Added missing comment.

Comment 18

[Joe Walker [:jwalker]]

Comment on attachment 601261 [details] [diff] [review]
Patch 3

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

::: browser/devtools/styleinspector/CssLogic.jsm
@@ +82,5 @@
> +const RX_PSEUDO_CLASS_OR_ELT = /(:[\w-]+\().*?\)/g;
> +const RX_CONNECTORS = /\s*[\s>+~]\s*/g;
> +const RX_ID = /\s*#\w+\s*/g;
> +const RX_CLASS_OR_ATTRIBUTE = /\s*(?:\.\w+|\[.+?\])\s*/g;
> +const RX_PSEUDO = /\s*:?:([\w-]+)(\(?\)?)\s*/g;

I'm possibly in the minority here, but I'd think that placing the regexes 1400 lines away from where they are used doesn't help readability.

Could we put them closer?

I don't know enough about JS compilers, but will I actually slow things down if we put them inline?

Comment 19

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

I agree that it makes the code less readable, but it is almost 50% faster doing it this way in the latest nightly as, if we don't cache the regexes, a new RegExp object is created every time one is encountered.

I used http://jsperf.com/creating-a-regex-in-and-outside-a-loop for comparison. In previous reviews I have been told to move regexes to the top for this reason.

try=green

Comment 20

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

https://hg.mozilla.org/integration/fx-team/rev/a04e0f00d144

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/a04e0f00d144