Closed Bug 683672 Opened 13 years ago Closed 13 years ago

Style Inspector is counting some unmatched rules incorrectly

Categories

(DevTools :: General, defect, P1)

Product:
DevTools
Component:
General
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
Firefox 9

People

(Reporter: rcampbell, Assigned: miker)

References

Details

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

Attachments

(3 files, 2 obsolete files)

screenshot
29.93 KB, image/png
Details
Simple test
376 bytes, text/html
Details
[in-fx-team] Incorrect number of rules patch 3
15.27 KB, patch
Details | Diff | Splinter Review
Attached image 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]

        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

    
    

    
  


  
(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+
Status: NEW → ASSIGNED
Whiteboard: [styleinspector][minotaur][has-patch] → [styleinspector][minotaur][has-patch][review+][land-in-fx-team]

    
  
Whiteboard: [styleinspector][minotaur][has-patch][review+][land-in-fx-team] → [styleinspector][minotaur][has-patch][review+][fixed-in-fx-team]

    
    

    
  
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
Closed: 13 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

    
  
Product: Firefox → DevTools

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: