Last Comment Bug 680111 - style inspector is not showing the correct selected rule
: style inspector is not showing the correct selected rule
Status: VERIFIED FIXED
[styleinspector][minotaur][has-patch]...
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: x86 All
: P1 normal (vote)
: Firefox 9
Assigned To: Michael Ratcliffe [:miker] [:mratcliffe]
:
: J. Ryan Stinnett [:jryans] (use ni?)
Mentors:
Depends on: 582596
Blocks: 672743 674856
  Show dependency treegraph
 
Reported: 2011-08-18 09:00 PDT by Kevin Dangoor
Modified: 2011-10-11 07:15 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Style Inspector screenshot that shows the problem (16.98 KB, image/png)
2011-08-18 09:00 PDT, Kevin Dangoor
no flags Details
Incorrectly selected rule patch 1 (5.41 KB, patch)
2011-08-19 06:12 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
mihai.sucan: review-
Details | Diff | Splinter Review
Incorrectly selected rule patch 2 (8.52 KB, patch)
2011-08-22 03:28 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
Incorrectly selected rule patch 3 (8.52 KB, patch)
2011-08-22 03:30 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
Incorrectly selected rule patch 4 (9.21 KB, patch)
2011-08-23 01:00 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
mihai.sucan: review+
Details | Diff | Splinter Review
Incorrectly selected rule patch 5 (8.91 KB, patch)
2011-08-25 08:28 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
[checked-in] Incorrectly selected rule patch 6 (8.44 KB, patch)
2011-08-30 06:00 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review

Description Kevin Dangoor 2011-08-18 09:00:39 PDT
Created attachment 554104 [details]
Style Inspector screenshot that shows the problem

I was capturing a handful of screenshots and I ran into this...

On the front page of http://cnn.com, there is a "Featured" section. Each of the images there has a div around it. The div is something like <div class="cnn_mtlpinimg">

Now look at the style inspector screen shot. It shows padding-bottom as 4px, but shows div -> 0pt as the winning rule and .cnn_mtlpinimg as crossed out.
Comment 1 Michael Ratcliffe [:miker] [:mratcliffe] 2011-08-19 04:18:49 PDT
This appears to happen for all rules so it is quite the show stopper. Must be an issue with CssLogic somewhere.
Comment 2 Michael Ratcliffe [:miker] [:mratcliffe] 2011-08-19 06:12:20 PDT
Created attachment 554388 [details] [diff] [review]
Incorrectly selected rule patch 1

Back in April we removed specificity calculations from the style inspector. At the time I was told that I could simply remove all of the code but did not realize that we were also using specificity to calculate the best match.

Specificity is resurrected by this patch (but not displayed to the user).

This code already passed review in bug 582596 but a DevTools peer could look over it.
Comment 3 Mihai Sucan [:msucan] 2011-08-19 09:48:56 PDT
Comment on attachment 554388 [details] [diff] [review]
Incorrectly selected rule patch 1

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

Patch looks fine and, yes, it fixes the cnn.com problem. Great!

The fix has r+, but please add a test, so we'll catch this kind of problems/regressions in the future. r- for the missing test. Looking forward to see the test! Thanks!

::: browser/devtools/styleinspector/CssLogic.jsm
@@ +1185,5 @@
> +    specificity.classes = 0;
> +    specificity.tags = 0;
> +
> +    // Split on CSS combinators (section 5.2).
> +    // TODO: We need to properly parse the selector. See bug 590090.

Bringing specificity back reintroduces the need for us to properly parse selectors.

Please open a follow up bug report. We need to address the problems from:

1. bug 592743.
2. bug 590090. These have a fix implemented in the patch for bug 585565 (among other changes). This patch didn't get merged into the main style inspector code. We should just copy what is relevant.

If we don't address these problems, our list of selectors is quite broken, unfortunately.

Once you open the follow up, please update the patch TODO and point to the new bug number. Thanks!
Comment 4 Michael Ratcliffe [:miker] [:mratcliffe] 2011-08-22 03:28:37 PDT
Created attachment 554810 [details] [diff] [review]
Incorrectly selected rule patch 2

Added test ... that was hard. I look forwards to the day when we can actually debug tests properly (object trees etc.)

I have reopened Bug 592743 - Style inspector specificity calculator makes mistakes to address the issues that you have raised
Comment 5 Michael Ratcliffe [:miker] [:mratcliffe] 2011-08-22 03:30:36 PDT
Created attachment 554811 [details] [diff] [review]
Incorrectly selected rule patch 3

Updated TODO
Comment 6 Michael Ratcliffe [:miker] [:mratcliffe] 2011-08-23 01:00:33 PDT
Created attachment 555049 [details] [diff] [review]
Incorrectly selected rule patch 4

Reviewer asked me to add comments to the test to clarify what I am doing when checking that the correct CSS selectors have been selected.
Comment 7 Mihai Sucan [:msucan] 2011-08-23 06:20:47 PDT
Comment on attachment 555049 [details] [diff] [review]
Incorrectly selected rule patch 4

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

The patch and the test are fine. r+! Please address the comments below. Thank you!

(Please note that for Bug 592743 I expect there will be a new separate test file which checks that the exact order of matchedSelectors is correct.)

::: browser/devtools/styleinspector/test/browser/browser_styleinspector_webconsole.js
@@ +142,5 @@
> +        selector = sel.selector.text;
> +        value = sel.value;
> +        break;
> +      }
> +    }

Why do you need to loop through the matchedSelectors to find the best match? Isn't matchedSelectors[0] always the best match?

@@ +157,5 @@
> +        is(value, "cursive", "correct css property value for #" + eltId);
> +        break;
> +      case eltArray[2]:
> +        is(selector, "#container", "correct best match for #container");
> +        is(value, "fantasy", "correct css property value for #" + eltId);

It would be less confusing if you'd use switch (eltId) { ... }, so we know which element you are checking.
Comment 8 Michael Ratcliffe [:miker] [:mratcliffe] 2011-08-25 08:28:04 PDT
Created attachment 555741 [details] [diff] [review]
Incorrectly selected rule patch 5

Addressed reviewers comments
Comment 9 Michael Ratcliffe [:miker] [:mratcliffe] 2011-08-30 03:47:16 PDT
Rebased and ready to land
Comment 10 Michael Ratcliffe [:miker] [:mratcliffe] 2011-08-30 06:00:45 PDT
Created attachment 556813 [details] [diff] [review]
[checked-in] Incorrectly selected rule patch 6

Rebased
Comment 11 Rob Campbell [:rc] (:robcee) 2011-08-30 09:56:11 PDT
Comment on attachment 556813 [details] [diff] [review]
[checked-in] Incorrectly selected rule patch 6

http://hg.mozilla.org/integration/fx-team/rev/54685bf66136
Comment 12 Rob Campbell [:rc] (:robcee) 2011-08-31 05:47:49 PDT
Comment on attachment 556813 [details] [diff] [review]
[checked-in] Incorrectly selected rule patch 6

http://hg.mozilla.org/mozilla-central/rev/54685bf66136
Comment 13 Florin Strugariu [:Bebe] 2011-10-11 07:15:49 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.