Style inspector specificity calculator makes mistakes

RESOLVED FIXED in Firefox 13

Status

()

Firefox
Developer Tools
P2
normal
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: jwalker, Assigned: miker)

Tracking

unspecified
Firefox 13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [computedview][ruleview])

Attachments

(2 attachments, 2 obsolete attachments)

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.
Blocks: 586984
Depends on: 582596

Updated

7 years ago
Whiteboard: [kd4b6]
Created attachment 471470 [details]
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.
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.
blocking2.0: --- → final+

Updated

7 years ago
Severity: normal → blocker

Comment 3

7 years ago
Inspector feature postponed. Removing blocking flags from Inspector bugs.
blocking2.0: final+ → ---

Comment 4

7 years ago
Removing items from kd4b6.
Whiteboard: [kd4b6]

Comment 5

7 years ago
Reprioritizing bugs. You can filter the mail on the word TEABAGS.
Severity: blocker → normal
Assignee: nobody → jwalker
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
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → INVALID
Reopened because specificity is needed by the style inspector to discover which is the currently applied rule for a CSS property.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Whiteboard: [styleinspector][minotaur]
We should also consider some of the fixes in https://bug585565.bugzilla.mozilla.org/attachment.cgi?id=471904 here

Updated

6 years ago
Priority: -- → P2
Assignee: jwalker → mratcliffe
Status: REOPENED → ASSIGNED
We should be using exactly the same rules to calculate css specificity as Firefox because the style inspector and browser should *never* disagree.
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.
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
Bug 682318 logged.
Depends on: 682318
Bug triage, filter on PEGASUS.
Whiteboard: [styleinspector][minotaur] → [computedview][ruleview]
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.
Created attachment 601257 [details] [diff] [review]
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.
Attachment #601257 - Flags: review?(jwalker)
Created attachment 601260 [details] [diff] [review]
Patch 2

Removed hilariously obvious bug.
Attachment #601257 - Attachment is obsolete: true
Attachment #601260 - Flags: review?(jwalker)
Attachment #601257 - Flags: review?(jwalker)
Created attachment 601261 [details] [diff] [review]
Patch 3

What the heck is wrong with me? Added missing comment.
Attachment #601260 - Attachment is obsolete: true
Attachment #601261 - Flags: review?(jwalker)
Attachment #601260 - Flags: review?(jwalker)
Whiteboard: [computedview][ruleview] → [computedview][ruleview][has-patch]
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?
Attachment #601261 - Flags: review?(jwalker) → review+
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
Whiteboard: [computedview][ruleview][has-patch] → [computedview][ruleview][land-in-fx-team]
No longer depends on: 682318
https://hg.mozilla.org/integration/fx-team/rev/a04e0f00d144
Whiteboard: [computedview][ruleview][land-in-fx-team] → [computedview][ruleview][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/a04e0f00d144
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [computedview][ruleview][fixed-in-fx-team] → [computedview][ruleview]
Target Milestone: --- → Firefox 13
You need to log in before you can comment on or make changes to this bug.