680111 - style inspector is not showing the correct selected rule

Status: VERIFIED FIXED

(DevTools :: General, defect, P1)

Description

[Kevin Dangoor]

Attachment: 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

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

This appears to happen for all rules so it is quite the show stopper. Must be an issue with CssLogic somewhere.

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 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]]

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

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 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

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Incorrectly selected rule patch 3

Updated TODO

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 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]]

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

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Incorrectly selected rule patch 5

Addressed reviewers comments

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Rebased and ready to land

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: [checked-in] Incorrectly selected rule patch 6

Rebased

Comment 11

[Rob Campbell [:rc] (:robcee)]

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)]

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]]

verified fixed on:
Mozilla/5.0 (Windows NT 6.1; rv:9.0a2) Gecko/20111010 Firefox/9.0a2