Style Inspector is counting some unmatched rules incorrectly

VERIFIED FIXED in Firefox 9

Status

()

Firefox
Developer Tools
P1
normal
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: rc, Assigned: miker)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [styleinspector][minotaur][has-patch][review+])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 557272 [details]
screenshot

in the about:home page, with the <div id="sessionRestoreContainer"> selected, the vertical-align rule claims there are 4 unmatched rules but the contents list 5.

Similarly, text-align claims 6 and shows 7.

See attached screenshot.
The text is correct but this is a problem with terminology. Take the following testcase as an example:

<html>
  <head>
    <style>
      #test, .test1, .test2, .test3, .test4 {
        color: #000;
      }

      div {
        position: absolute;
        top: 40px;
        left: 20px;
        border: 1px solid #000;
        color: #111;
        width: 100px;
        height: 50px;
      }
    </style>
  </head>
  <body>
    inspectstyle($("test"));
    <div id="test" class="test1 test2 test3 test4">Test div</div>
  </body>
</html>

There are 2 rules in the above test and 6 selectors. Unfortunately, in the style inspector the expando says "2 rules" but clicking on it displays the 6 selectors. The expando should read "6 selectors" rather than "2 rules."

This is misleading, hence marking this as minotaur.
Assignee: nobody → mratcliffe
Whiteboard: [styleinspector][minotaur]
Created attachment 557437 [details]
Simple test
Depends on: 683320
Created attachment 557498 [details] [diff] [review]
Incorrect number of rules patch 1
Attachment #557498 - Flags: review?(mihai.sucan)
Whiteboard: [styleinspector][minotaur] → [styleinspector][minotaur][has-patch]
Priority: -- → P1
Comment on attachment 557498 [details] [diff] [review]
Incorrect number of rules patch 1

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

Very close to r+. Please address the comments below. Thank you!

::: browser/devtools/styleinspector/CssLogic.jsm
@@ +316,5 @@
>      let cssSheet = this.getSheet(aDomSheet, false, this._sheetIndex++);
> +
> +    // Avoid strict warnings
> +    if (typeof cssSheet._passId == "undefined") {
> +      cssSheet._passId = null;

Please do not do this here.

Change the CssSheet prototype to include _passId: null.

(Same comment applies to all the changes in this file, adjusted for each case.)

::: browser/devtools/styleinspector/StyleInspector.jsm
@@ +112,5 @@
>          this.cssLogic = new CssLogic();
>          this.cssHtmlTree = new CssHtmlTree(iframe, this.cssLogic, this);
>        }
>  
> +      let selectedNode = this.selectedNode || null;

Why is this change needed here?

::: browser/devtools/styleinspector/test/browser/Makefile.in
@@ +53,5 @@
>    $(NULL)
>  
>  _BROWSER_TEST_PAGES = \
>    browser_styleinspector_webconsole.htm \
> +  browser_bug683672.htm \

Nit: please use the .html extension.

::: browser/devtools/styleinspector/test/browser/browser_bug683672.js
@@ +48,5 @@
> +  let numMatchedSelectors = propertyInfo.matchedSelectors.length;
> +  let numUnmatchedSelectors = propertyInfo.unmatchedSelectors.length;
> +
> +  is(numMatchedSelectors, 6, "correct number of matched selectors");
> +  is(numUnmatchedSelectors, 7, "correct number of unmatched selectors");

This test file doesn't check if the CssHtmlTree fixes are there. CssLogic was fine. The bug is in CssHtmlTree, with the part that shows the rule title. Please check from the test that the rule title is correct - the same number of (un)matched selectors as in CssLogic.

Thank you!

::: browser/locales/en-US/chrome/browser/styleinspector.properties
@@ +39,2 @@
>  # See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
> +rule.showUnmatchedLink=1 unmatched selector...;#1 unmatched selectors...

Please use an ellipsis instead of three dots.
Attachment #557498 - Flags: review?(mihai.sucan) → review-
Depends on: 683737
No longer depends on: 683320
Created attachment 558252 [details] [diff] [review]
Incorrect number of rules patch 2

(In reply to Mihai Sucan [:msucan] from comment #4)
> Comment on attachment 557498 [details] [diff] [review]
> Incorrect number of rules patch 1
> 
> Review of attachment 557498 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Very close to r+. Please address the comments below. Thank you!
> 
> ::: browser/devtools/styleinspector/CssLogic.jsm
> @@ +316,5 @@
> >      let cssSheet = this.getSheet(aDomSheet, false, this._sheetIndex++);
> > +
> > +    // Avoid strict warnings
> > +    if (typeof cssSheet._passId == "undefined") {
> > +      cssSheet._passId = null;
> 
> Please do not do this here.
> 
> Change the CssSheet prototype to include _passId: null.
> 
> (Same comment applies to all the changes in this file, adjusted for each
> case.)
> 

Done

> ::: browser/devtools/styleinspector/StyleInspector.jsm
> @@ +112,5 @@
> >          this.cssLogic = new CssLogic();
> >          this.cssHtmlTree = new CssHtmlTree(iframe, this.cssLogic, this);
> >        }
> >  
> > +      let selectedNode = this.selectedNode || null;
> 
> Why is this change needed here?
> 

Removed

> ::: browser/devtools/styleinspector/test/browser/Makefile.in
> @@ +53,5 @@
> >    $(NULL)
> >  
> >  _BROWSER_TEST_PAGES = \
> >    browser_styleinspector_webconsole.htm \
> > +  browser_bug683672.htm \
> 
> Nit: please use the .html extension.
> 

Done

> ::: browser/devtools/styleinspector/test/browser/browser_bug683672.js
> @@ +48,5 @@
> > +  let numMatchedSelectors = propertyInfo.matchedSelectors.length;
> > +  let numUnmatchedSelectors = propertyInfo.unmatchedSelectors.length;
> > +
> > +  is(numMatchedSelectors, 6, "correct number of matched selectors");
> > +  is(numUnmatchedSelectors, 7, "correct number of unmatched selectors");
> 
> This test file doesn't check if the CssHtmlTree fixes are there. CssLogic
> was fine. The bug is in CssHtmlTree, with the part that shows the rule
> title. Please check from the test that the rule title is correct - the same
> number of (un)matched selectors as in CssLogic.
> 
> Thank you!
> 

Okay, we now check the titles now as well.

> ::: browser/locales/en-US/chrome/browser/styleinspector.properties
> @@ +39,2 @@
> >  # See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
> > +rule.showUnmatchedLink=1 unmatched selector...;#1 unmatched selectors...
> 
> Please use an ellipsis instead of three dots.

Done
Attachment #557498 - Attachment is obsolete: true
Attachment #558252 - Flags: review?(mihai.sucan)
Comment on attachment 558252 [details] [diff] [review]
Incorrect number of rules patch 2

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

This looks fine, r+! Please address the comment below and push the patch (and the deps) to the try server.

Thank you!

::: browser/devtools/styleinspector/test/browser/browser_bug683672.html
@@ +1,2 @@
> +<html>
> +  <head>

Please add the PD license boilerplate to this test file as well.
Attachment #558252 - Flags: review?(mihai.sucan) → review+
Created attachment 558273 [details] [diff] [review]
[in-fx-team] Incorrect number of rules patch 3

Done
Attachment #558252 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Whiteboard: [styleinspector][minotaur][has-patch] → [styleinspector][minotaur][has-patch][review+][land-in-fx-team]
Blocks: 674856
(Reporter)

Updated

6 years ago
Whiteboard: [styleinspector][minotaur][has-patch][review+][land-in-fx-team] → [styleinspector][minotaur][has-patch][review+][fixed-in-fx-team]
(Reporter)

Comment 8

6 years ago
Comment on attachment 558273 [details] [diff] [review]
[in-fx-team] Incorrect number of rules patch 3

http://hg.mozilla.org/integration/fx-team/rev/05b61a98eb00
Attachment #558273 - Attachment description: Incorrect number of rules patch 3 → [in-fx-team] Incorrect number of rules patch 3
http://hg.mozilla.org/mozilla-central/rev/05b61a98eb00
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [styleinspector][minotaur][has-patch][review+][fixed-in-fx-team] → [styleinspector][minotaur][has-patch][review+]
Target Milestone: --- → Firefox 9
-rule.showUnmatchedLink=One unmatched rule...;#1 unmatched rules...
+rule.showUnmatchedLink=1 unmatched selector…;#1 unmatched selectors…

The property name should be changed when the meaning changes. Since selector != rule, this one here should be renamed, too.

ccing Pike
(In reply to Marek Stępień :marcoos from comment #10)
> -rule.showUnmatchedLink=One unmatched rule...;#1 unmatched rules...
> +rule.showUnmatchedLink=1 unmatched selector…;#1 unmatched selectors…
> 
> The property name should be changed when the meaning changes. Since selector
> != rule, this one here should be renamed, too.
> 
> ccing Pike

Thanks for your report Marek! Sorry for not catching this during review.

Mike: can you please open a follow bug report to fix this, and submit a quick patch to rename the string property name? Thanks!

Marek: Is this an acceptable course of action?
(In reply to Mihai Sucan [:msucan] from comment #11)
> Marek: Is this an acceptable course of action?

Yes, that's the correct way.
(In reply to Marek Stępień :marcoos from comment #10)
> -rule.showUnmatchedLink=One unmatched rule...;#1 unmatched rules...
> +rule.showUnmatchedLink=1 unmatched selector…;#1 unmatched selectors…
> 
> The property name should be changed when the meaning changes. Since selector
> != rule, this one here should be renamed, too.
> 

Bug 685979 logged
verified fixed on:
Mozilla/5.0 (Windows NT 6.1; rv:9.0a2) Gecko/20111010 Firefox/9.0a2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.