Closed Bug 1293616 Opened 8 years ago Closed 8 years ago

editing a selector causes the inspector rules panel to lie about which declarations are winning

Categories

(DevTools :: Inspector: Rules, defect, P2)

Product:
DevTools
Component:
Inspector: Rules
Version:
47 Branch
Platform:
Unspecified
macOS
Type:
defect

Tracking

(firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: matteo, Assigned: tromey)

Details

Attachments

(4 files)

Normal priority
20.56 KB, image/png
Details
The bugged behaviour
17.94 KB, image/png
Details
simple testcase
449 bytes, text/html
Details
Bug 1293616 - insert new rule in correct position in rule list;
58 bytes, text/x-review-board-request
pbro
: review+
Details 
User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:44.0) Gecko/20100101 Firefox/44.0
Build ID: 20160711192800

Steps to reproduce:

I put 2 rules that match an image:

#some_id img {position: absolute}
#the_img_id {position: relative}

-> my image is in position relative
This work like a charm. howerver if the first one is a multiple selector like:

#some_id img, #another_id p {position: absolute}
#the_img_id {position: relative}

Then the first one take precedence of the id of the image and my image is in position absolute


Actual results:

The wrong rule is applied and my image is in position absolute


Expected results:

The id selector of my image should be higher in precedence and my image should be in position relative
found on firefox 47 mac, exists in firefox linux 47 too
OS: Unspecified → Mac OS X
(In reply to Matteo from comment #0)
> I put 2 rules that match an image:
> 
> #some_id img {position: absolute}
> #the_img_id {position: relative}
> 
> -> my image is in position relative

This isn't supposed to happen, since the first selector has higher specificity than the second.  I'm guessing you have a typo, such as a trailing "," at the end.

> This work like a charm. howerver if the first one is a multiple selector
> like:
> 
> #some_id img, #another_id p {position: absolute}
> #the_img_id {position: relative}
> 
> Then the first one take precedence of the id of the image and my image is in
> position absolute

This is correct, since the rules in the first line have higher specificity.

I presume you see the same behavior in other browsers as well.

If not, please reopen.
Status: UNCONFIRMED → RESOLVED
Closed: 8 years ago
Resolution: --- → INVALID
Summary: coma in css selector break the order of applying css → comma in css selector break the order of applying css
(And if you reopen, please attach a testcase showing the bug.)
I have no typo, but if you say it's normal then i misunderstand the higher specificity.

So you say that
#any_div_id img (in clear any image in my selected div) is higher than pointing directly the image with his id ?

If it's the case, i'm sorry for the invalid bug post, otherwise it's a bug.

If i'm wrong, can you provide a link to complete documentation about higher priority because on mdn it's comparing selector like id/class and pseudo but i didn't found anything about computing (i.e. id of ancestor + pseudo between direct id)
Ok i got it. But i discover why i'm wrong.
There is a bug which indicate me the wrong higher specificity.
I will provide you some screenshot. The but is that if i remove the part behind the coma (including the coma), the rule loose it's priority. (I was thinking this is the normal behaviour and in fact, it's the opposite).
I will provide you step to reproduce and i notice that it's not the same in chromium.

Should i reopen this one or create a new one ?
Attached image Normal priority — 
The normal behaviour as you described
Attached image The bugged behaviour — 
Between the previous image and this, i just removed the part after the comma in the selector and priority changed
I reopened because i have no response for last comment (maybe you check only comment from open ticket)
Please let me know if you prefer me to create a new one to avoid confusion with the first comments or if it's ok for you
Status: RESOLVED → UNCONFIRMED
Resolution: INVALID → ---
Could you provide the URL where this happens, or attach a testcase?
And are you now saying that:
 * there's a bug in the CSS implementation in Gecko, or
 * there's a bug in the devtools inspector showing the wrong thing?
The person who notice me the bug (my html integrator) say that he tested without the devtools so i think it's in the css implementation.
I only now the devtools part because i wanted to see the bug before submitting.
To reproduce:
url: http://www.helenevaldisere.com/agence
Inspect the closing pink quote.
-> add a position:absolute to the #close-quotes rules
-> remove the ", #helene-delval-citaion p" part from the rule "#helene-delval-citation img, #helene-delval-citation p"
It should change nothing but in fact, #close-quotes takes higher priority

To confirm it's not in the dev tools i will make a simple html test and post the result as new comment.
I made a static of this page (by ctrl + s), modify the css as described directly in the css file.
The results is the same so i think it's in the css implementation.
Dou you want the zipped files i've done or have you enough d
Sorry, i mistake with the keyboard.
Dou you want the zipped files i've done or have you enough data to reproduce ?
(In reply to Matteo from comment #11)
> The person who notice me the bug (my html integrator) say that he tested
> without the devtools so i think it's in the css implementation.

Was part of his test this:

> -> remove the ", #helene-delval-citaion p" part from the rule
> "#helene-delval-citation img, #helene-delval-citation p"
> It should change nothing but in fact, #close-quotes takes higher priority

which should change the layout of the page, because it removes the 'float: left' from the p.

I think this is a bug in the devtools, incorrectly crossing out the wrong style.
Attached file simple testcase — 
Steps to reproduce:
 1. load this testcase
 2. open inspector (Ctrl+Shift+C)
 3. click the pointer icon to select an element
 4. click on the quotes
 5. in the rules panel, remove the ", #helene-delval-citation p" from the end of the selector that contains it
 6. click back in the page

Actual results:
 the rules panel shows that the aqua background is not being applied and the fuchsia background is being applied, but the background of the quote is still aqua (as it should be)

Expected results:
 rules panel matches display, as it did before editing the selector
Component: CSS Parsing and Computation → Developer Tools: CSS Rules Inspector
Product: Core → Firefox
Summary: comma in css selector break the order of applying css → editing a selector causes the inspector rules panel to lie about which declarations are winning
Status: UNCONFIRMED → NEW
Ever confirmed: true
Inspector bug triage, filter on CLIMBING SHOES.

Inspector should not lie -> P2.
pbro: I'd like to bring your attention on this one, maybe move it to your list of high prio P2s or to P1 ?
Flags: needinfo?(pbrosset)
Priority: -- → P2
Added this bug to bug 1264624.
When you select another node and then select the image again, that fixes the issue. So refreshing the rule-view works correctly. This means that this is a problem with either the response from the modifySelector request, or the way the rule-editor refreshes the rule UI after the selector was modified.
Flags: needinfo?(pbrosset)
I think the bug is that we insert the new rule into the wrong position in elementStyles.rule
when modifying the selector.
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Priority: P2 → --
(I think P2 was just removed by accident)
(filter on CLIMBING SHOES)
Priority: -- → P2
Comment on attachment 8787680 [details]
Bug 1293616 - insert new rule in correct position in rule list;

https://reviewboard.mozilla.org/r/76372/#review74476
Attachment #8787680 - Flags: review?(pbrosset) → review+
What if the edits to the selector should actually cause it to have a different priority relative to the other rules that match the element?  Does that work correctly?
(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #23)
> What if the edits to the selector should actually cause it to have a
> different priority relative to the other rules that match the element?  Does
> that work correctly?

Thanks for thinking of this.

It doesn't work.  I think some bigger change is going to be needed.
A few notes on the coming patch:

First, I changed _onSelectorDone to re-fetch the list of applied rules using
getApplied.  I considered adding more information to modifySelector's result, but
that would require yet another compatibility shim ("modifySelector3" I guess), and
this route didn't seem notably worse.

Second, I originally had this code also insert the new rule editor into the correct
place in the parent node.  However, that turns out to be tricky if one edits the selector
to change it so that it would enter or leave the pseudo-elements sub-panel.  So I did not
do that.

Third, I tried the much simpler change of simply always refreshing the rule view, using
refreshPanel.  To my surprise this didn't work at all.
Comment on attachment 8787680 [details]
Bug 1293616 - insert new rule in correct position in rule list;

This changed enough to require re-review.
Attachment #8787680 - Flags: review+ → review?(pbrosset)
Comment on attachment 8787680 [details]
Bug 1293616 - insert new rule in correct position in rule list;

https://reviewboard.mozilla.org/r/76372/#review83812

This change looks good, thanks. Thanks for using a Task, makes it easier to read too.

::: devtools/client/inspector/rules/test/browser_rules_edit-selector_10.js:7
(Diff revision 3)
> +/* Any copyright is dedicated to the Public Domain.
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Regression test for bug 1293616.

I think it's nice when these comments describe what the test actually does. Giving the reference to the bug number is useful, but if there was a sentence that simply explains to me what use case this is testing, this would be even better.
Attachment #8787680 - Flags: review?(pbrosset) → review+
I've updated the comment a bit.
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5f4f04a669ea
insert new rule in correct position in rule list; r=pbro
https://hg.mozilla.org/mozilla-central/rev/5f4f04a669ea
Status: ASSIGNED → RESOLVED
Closed: 8 years ago → 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
No longer blocks: top-inspector-bugs
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: