Closed
Bug 680111
Opened 13 years ago
Closed 13 years ago
style inspector is not showing the correct selected rule
Categories
(DevTools :: General, defect, P1)
Tracking
(Not tracked)
VERIFIED
FIXED
Firefox 9
People
(Reporter: dangoor, Assigned: miker)
References
Details
(Whiteboard: [styleinspector][minotaur][has-patch][review+][fixed-in-fx-team])
Attachments
(2 files, 5 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•13 years ago
|
Whiteboard: [minotaur] → [styleinspector][minotaur]
Assignee | ||
Comment 1•13 years ago
|
||
This appears to happen for all rules so it is quite the show stopper. Must be an issue with CssLogic somewhere.
Assignee | ||
Comment 2•13 years ago
|
||
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?
Assignee | ||
Updated•13 years ago
|
Whiteboard: [styleinspector][minotaur] → [styleinspector][minotaur][has-patch]
Assignee | ||
Updated•13 years ago
|
Attachment #554388 -
Flags: review? → review?(mihai.sucan)
Comment 3•13 years ago
|
||
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-
Assignee | ||
Comment 4•13 years ago
|
||
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)
Assignee | ||
Comment 5•13 years ago
|
||
Updated TODO
Attachment #554810 -
Attachment is obsolete: true
Attachment #554810 -
Flags: review?(mihai.sucan)
Attachment #554811 -
Flags: review?(mihai.sucan)
Assignee | ||
Comment 6•13 years ago
|
||
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 7•13 years ago
|
||
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+
Assignee | ||
Comment 8•13 years ago
|
||
Addressed reviewers comments
Attachment #555049 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Whiteboard: [styleinspector][minotaur][has-patch] → [styleinspector][minotaur][has-patch][review+]
Assignee | ||
Comment 9•13 years ago
|
||
Rebased and ready to land
Updated•13 years ago
|
Whiteboard: [styleinspector][minotaur][has-patch][review+] → [styleinspector][minotaur][has-patch][review+][fixed-in-fx-team]
Comment 11•13 years ago
|
||
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 12•13 years ago
|
||
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
Updated•13 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
Comment 13•13 years ago
|
||
verified fixed on:
Mozilla/5.0 (Windows NT 6.1; rv:9.0a2) Gecko/20111010 Firefox/9.0a2
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•