Last Comment Bug 683672 - Style Inspector is counting some unmatched rules incorrectly
: Style Inspector is counting some unmatched rules incorrectly
Status: VERIFIED FIXED
[styleinspector][minotaur][has-patch]...
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: Firefox 9
Assigned To: Michael Ratcliffe [:miker] [:mratcliffe]
:
Mentors:
Depends on: 683737
Blocks: 674856
  Show dependency treegraph
 
Reported: 2011-08-31 12:15 PDT by Rob Campbell [:rc] (:robcee)
Modified: 2011-10-11 07:23 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
screenshot (29.93 KB, image/png)
2011-08-31 12:15 PDT, Rob Campbell [:rc] (:robcee)
no flags Details
Simple test (376 bytes, text/html)
2011-09-01 01:08 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details
Incorrect number of rules patch 1 (14.96 KB, patch)
2011-09-01 07:50 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
mihai.sucan: review-
Details | Diff | Review
Incorrect number of rules patch 2 (15.16 KB, patch)
2011-09-05 02:53 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
mihai.sucan: review+
Details | Diff | Review
[in-fx-team] Incorrect number of rules patch 3 (15.27 KB, patch)
2011-09-05 06:17 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Review

Description Rob Campbell [:rc] (:robcee) 2011-08-31 12:15:14 PDT
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.
Comment 1 Michael Ratcliffe [:miker] [:mratcliffe] 2011-09-01 01:07:36 PDT
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.
Comment 2 Michael Ratcliffe [:miker] [:mratcliffe] 2011-09-01 01:08:43 PDT
Created attachment 557437 [details]
Simple test
Comment 3 Michael Ratcliffe [:miker] [:mratcliffe] 2011-09-01 07:50:59 PDT
Created attachment 557498 [details] [diff] [review]
Incorrect number of rules patch 1
Comment 4 Mihai Sucan [:msucan] 2011-09-02 09:01:43 PDT
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.
Comment 5 Michael Ratcliffe [:miker] [:mratcliffe] 2011-09-05 02:53:21 PDT
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
Comment 6 Mihai Sucan [:msucan] 2011-09-05 05:08:44 PDT
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.
Comment 7 Michael Ratcliffe [:miker] [:mratcliffe] 2011-09-05 06:17:06 PDT
Created attachment 558273 [details] [diff] [review]
[in-fx-team] Incorrect number of rules patch 3

Done
Comment 8 Rob Campbell [:rc] (:robcee) 2011-09-06 10:54:57 PDT
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
Comment 9 Tim Taubert [:ttaubert] 2011-09-08 02:37:12 PDT
http://hg.mozilla.org/mozilla-central/rev/05b61a98eb00
Comment 10 Marek Stępień [:marcoos, inactive] 2011-09-09 10:48:07 PDT
-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
Comment 11 Mihai Sucan [:msucan] 2011-09-09 10:53:11 PDT
(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?
Comment 12 Marek Stępień [:marcoos, inactive] 2011-09-09 11:08:17 PDT
(In reply to Mihai Sucan [:msucan] from comment #11)
> Marek: Is this an acceptable course of action?

Yes, that's the correct way.
Comment 13 Michael Ratcliffe [:miker] [:mratcliffe] 2011-09-09 12:30:27 PDT
(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
Comment 14 Florin Strugariu [:Bebe] 2011-10-11 07:23:35 PDT
verified fixed on:
Mozilla/5.0 (Windows NT 6.1; rv:9.0a2) Gecko/20111010 Firefox/9.0a2

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