Closed Bug 1399105 Opened 7 years ago Closed 6 years ago

Rules View does not update when JS changes CSS classes

Categories

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

57 Branch
defect

Tracking

(firefox57 affected)

RESOLVED DUPLICATE of bug 1447736
Tracking Status
firefox57 --- affected

People

(Reporter: nachtigall, Unassigned)

Details

Attachments

(1 file)

STR:
===
1. Go to https://codepen.io/nachtigall/pen/boGNwP
2. Inspect '.inner'
3. Click on the red '.inner' with mouse (this toggles/adds an '.changedIt' on the '.outer'
4. Now the rule the following rule comes into effect:

.outer.changedIt .inner {
  width: 50px;
}


ER:
===
The Rules View updates and shows me the correct property that is applied (here 'width')


AR:
===
The rules view does not update and it is really hard to see what property '.inner' has. The correct values/rules are only shown after manually selecting another element and then re-selecting '.inner'. See screencasted GIF attached.

This 'bug' has really caused a lot of time/effort to me (I've hit this quite a lot in the past), because one really gets the wrong results/directions. 

This is really annoying when debugging JS-heavy applications/sites.

FWIW, this works in Chrome, that is Rule's View does update here (not meant to demoralize, there are other things in Firefox DevTools that are way better <3  ;)
Thanks for filing.
Priority: -- → P1
Assignee: nobody → gl
Status: NEW → ASSIGNED
Hiya,

I am currently looking for bugs to fix as part of my Open Source Development module at Coventry University and I am interested in developing this bug.

Please could you assign this task to me and give me more information.

This is my first bug fix and any help would be appreciated.

Thank you.
I believe I have found the solution to the problem, please could you give me details on what I need to do now to submit it to you.

Thank you.
Assignee: gl → teddsr
(In reply to Rayann Tedds from comment #3)
> I believe I have found the solution to the problem, please could you give me
> details on what I need to do now to submit it to you.
> 
> Thank you.

Hey Rayann,

Can you ping me on Slack at :gl https://devtools-html-slack.herokuapp.com/.

It's hard to provide you more information without knowing if you're using the Git or Mercurial workflow.

Thanks
The relevant part of the code is https://searchfox.org/mozilla-central/source/devtools/client/inspector/rules/rules.js#1683.

You can see that the target checks if it is the current selected node. I think it should probably do something along the lines of checking if the selected node is contained inside of the target that we check. If we go with this approach, we will need to add a target.contains(node) method to the NodeActor. See an example with the implementation of getCloestBackgroundColor https://searchfox.org/mozilla-central/search?q=getClosestBackgroundColor&path=. This has some consequences if CSS Selector spec ever adds a parent selector.

Does this sound good Patrick?
Flags: needinfo?(pbrosset)
(In reply to Gabriel [:gl] (ΦωΦ) from comment #5)
> The relevant part of the code is
> https://searchfox.org/mozilla-central/source/devtools/client/inspector/rules/
> rules.js#1683.
> 
> You can see that the target checks if it is the current selected node. I
> think it should probably do something along the lines of checking if the
> selected node is contained inside of the target that we check. If we go with
> this approach, we will need to add a target.contains(node) method to the
> NodeActor. See an example with the implementation of
> getCloestBackgroundColor
> https://searchfox.org/mozilla-central/
> search?q=getClosestBackgroundColor&path=. This has some consequences if CSS
> Selector spec ever adds a parent selector.
> 
> Does this sound good Patrick?

Just to clarify, are you just looking for the CSS to work in Firefox too? Or am I missing a vital piece of the bug?
And I am using git :)
(In reply to Rayann Tedds from comment #6)
> (In reply to Gabriel [:gl] (ΦωΦ) from comment #5)
> > The relevant part of the code is
> > https://searchfox.org/mozilla-central/source/devtools/client/inspector/rules/
> > rules.js#1683.
> > 
> > You can see that the target checks if it is the current selected node. I
> > think it should probably do something along the lines of checking if the
> > selected node is contained inside of the target that we check. If we go with
> > this approach, we will need to add a target.contains(node) method to the
> > NodeActor. See an example with the implementation of
> > getCloestBackgroundColor
> > https://searchfox.org/mozilla-central/
> > search?q=getClosestBackgroundColor&path=. This has some consequences if CSS
> > Selector spec ever adds a parent selector.
> > 
> > Does this sound good Patrick?
> 
> Just to clarify, are you just looking for the CSS to work in Firefox too? Or
> am I missing a vital piece of the bug?

I am not quite sure I understood your question, but the problem is that JS can cause a attribute change to a parent container of the current selected element. In this bug, the attribute change is a CSS class change on the parent container and there is a CSS rule that can appear when the parent container has a certain CSS class (see example for further clarification). The problem is that we don't update the CSS rules when the parent container gets this attribute change. You can see in the relevant part of the code that this attribute change is classified as a mutation and we also change for the type of mutation which is "attribute". We don't call update because we only originally care if the mutation happened on the selected node. 

Hope that helps. Otherwise, comment about what is still unclear or message me on Slack. Timezone might be tricky for me, but pbro is also available in the European timezones.
(In reply to Gabriel [:gl] (ΦωΦ) from comment #5)
> The relevant part of the code is
> https://searchfox.org/mozilla-central/source/devtools/client/inspector/rules/
> rules.js#1683.
> 
> You can see that the target checks if it is the current selected node. I
> think it should probably do something along the lines of checking if the
> selected node is contained inside of the target that we check. If we go with
> this approach, we will need to add a target.contains(node) method to the
> NodeActor. See an example with the implementation of
> getCloestBackgroundColor
> https://searchfox.org/mozilla-central/
> search?q=getClosestBackgroundColor&path=. This has some consequences if CSS
> Selector spec ever adds a parent selector.
> 
> Does this sound good Patrick?
Yes I think this is the right idea.
However, I don't think we need to call the server. The protocol methods we use in the inspector always guaranty that nodeFront.parent() works (because we can't display disconnected nodes). So, I think it would be simple to just check if this.inspector.selection.nodeFront is the target, or target.parent(), target.parent().parent() ... in a while loop.
We should stop looping until we find a document (no need to go up iframe boundaries too).
Flags: needinfo?(pbrosset)
Resetting the assignee due to lack of activity.
Assignee: teddsr → nobody
Status: ASSIGNED → NEW
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
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: