style inspector is not showing the correct selected rule

VERIFIED FIXED in Firefox 9

Status

()

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

People

(Reporter: Kevin Dangoor, Assigned: miker)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [styleinspector][minotaur][has-patch][review+][fixed-in-fx-team])

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

6 years ago
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.
Whiteboard: [minotaur] → [styleinspector][minotaur]
This appears to happen for all rules so it is quite the show stopper. Must be an issue with CssLogic somewhere.
Status: NEW → ASSIGNED
Depends on: 582596, 663831, 672743, 672748
OS: Mac OS X → All
Priority: -- → P1
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.
Attachment #554388 - Flags: review?
Whiteboard: [styleinspector][minotaur] → [styleinspector][minotaur][has-patch]
Attachment #554388 - Flags: review? → review?(mihai.sucan)
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!
Attachment #554388 - Flags: review?(mihai.sucan) → review-
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
Attachment #554388 - Attachment is obsolete: true
Attachment #554810 - Flags: review?(mihai.sucan)
Created attachment 554811 [details] [diff] [review]
Incorrectly selected rule patch 3

Updated TODO
Attachment #554810 - Attachment is obsolete: true
Attachment #554810 - Flags: review?(mihai.sucan)
Attachment #554811 - Flags: review?(mihai.sucan)
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.
Attachment #554811 - Attachment is obsolete: true
Attachment #554811 - Flags: review?(mihai.sucan)
Attachment #555049 - Flags: review?(mihai.sucan)
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.
Attachment #555049 - Flags: review?(mihai.sucan) → review+
Blocks: 674856
Created attachment 555741 [details] [diff] [review]
Incorrectly selected rule patch 5

Addressed reviewers comments
Attachment #555049 - Attachment is obsolete: true
Whiteboard: [styleinspector][minotaur][has-patch] → [styleinspector][minotaur][has-patch][review+]
Rebased and ready to land
No longer depends on: 663831, 672743, 672748
Blocks: 672743
Created attachment 556813 [details] [diff] [review]
[checked-in] Incorrectly selected rule patch 6

Rebased
Attachment #555741 - Attachment is obsolete: true
Whiteboard: [styleinspector][minotaur][has-patch][review+] → [styleinspector][minotaur][has-patch][review+][fixed-in-fx-team]
Comment on attachment 556813 [details] [diff] [review]
[checked-in] Incorrectly selected rule patch 6

http://hg.mozilla.org/integration/fx-team/rev/54685bf66136
Attachment #556813 - Attachment description: Incorrectly selected rule patch 6 → [in-fx-team] Incorrectly selected rule patch 6
Comment on attachment 556813 [details] [diff] [review]
[checked-in] Incorrectly selected rule patch 6

http://hg.mozilla.org/mozilla-central/rev/54685bf66136
Attachment #556813 - Attachment description: [in-fx-team] Incorrectly selected rule patch 6 → [checked-in] Incorrectly selected rule patch 6
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
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.