Last Comment Bug 592743 - Style inspector specificity calculator makes mistakes
: Style inspector specificity calculator makes mistakes
Status: RESOLVED FIXED
[computedview][ruleview]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: P2 normal (vote)
: Firefox 13
Assigned To: Michael Ratcliffe [:miker] [:mratcliffe]
:
Mentors:
Depends on: 582596
Blocks: 586984
  Show dependency treegraph
 
Reported: 2010-09-01 10:20 PDT by Joe Walker [:jwalker] (needinfo me or ping on irc)
Modified: 2012-03-02 02:07 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Visual map of selector spec (457.45 KB, image/jpeg)
2010-09-02 05:59 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
no flags Details
Patch (7.96 KB, patch)
2012-02-28 06:13 PST, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Review
Patch 2 (7.92 KB, patch)
2012-02-28 06:20 PST, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Review
Patch 3 (8.00 KB, patch)
2012-02-28 06:27 PST, Michael Ratcliffe [:miker] [:mratcliffe]
jwalker: review+
Details | Diff | Review

Description Joe Walker [:jwalker] (needinfo me or ping on irc) 2010-09-01 10:20:50 PDT
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] (needinfo me or ping on irc) 2010-09-02 05:59:53 PDT
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.
Comment 2 Justin Dolske [:Dolske] 2010-09-03 02:49:02 PDT
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 2010-09-03 19:56:57 PDT
Inspector feature postponed. Removing blocking flags from Inspector bugs.
Comment 4 Kevin Dangoor 2010-09-03 20:56:54 PDT
Removing items from kd4b6.
Comment 5 Kevin Dangoor 2010-09-04 19:00:40 PDT
Reprioritizing bugs. You can filter the mail on the word TEABAGS.
Comment 6 Michael Ratcliffe [:miker] [:mratcliffe] 2011-04-27 05:51:21 PDT
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 Michael Ratcliffe [:miker] [:mratcliffe] 2011-08-22 02:10:11 PDT
Reopened because specificity is needed by the style inspector to discover which is the currently applied rule for a CSS property.
Comment 8 Michael Ratcliffe [:miker] [:mratcliffe] 2011-08-22 02:15:37 PDT
We should also consider some of the fixes in https://bug585565.bugzilla.mozilla.org/attachment.cgi?id=471904 here
Comment 9 Michael Ratcliffe [:miker] [:mratcliffe] 2011-08-26 03:46:53 PDT
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 Michael Ratcliffe [:miker] [:mratcliffe] 2011-08-26 06:06:39 PDT
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 Michael Ratcliffe [:miker] [:mratcliffe] 2011-08-26 06:10:23 PDT
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 Michael Ratcliffe [:miker] [:mratcliffe] 2011-08-26 10:00:44 PDT
Bug 682318 logged.
Comment 13 Michael Ratcliffe [:miker] [:mratcliffe] 2012-01-11 04:17:28 PST
Bug triage, filter on PEGASUS.
Comment 14 Michael Ratcliffe [:miker] [:mratcliffe] 2012-02-23 07:25:06 PST
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 Michael Ratcliffe [:miker] [:mratcliffe] 2012-02-28 06:13:48 PST
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.
Comment 16 Michael Ratcliffe [:miker] [:mratcliffe] 2012-02-28 06:20:16 PST
Created attachment 601260 [details] [diff] [review]
Patch 2

Removed hilariously obvious bug.
Comment 17 Michael Ratcliffe [:miker] [:mratcliffe] 2012-02-28 06:27:08 PST
Created attachment 601261 [details] [diff] [review]
Patch 3

What the heck is wrong with me? Added missing comment.
Comment 18 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-02-28 06:52:08 PST
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 Michael Ratcliffe [:miker] [:mratcliffe] 2012-02-29 03:04:09 PST
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) 2012-03-01 05:36:26 PST
https://hg.mozilla.org/integration/fx-team/rev/a04e0f00d144
Comment 21 Tim Taubert [:ttaubert] 2012-03-02 02:07:03 PST
https://hg.mozilla.org/mozilla-central/rev/a04e0f00d144

Note You need to log in before you can comment on or make changes to this bug.