Closed Bug 1240038 Opened 8 years ago Closed 8 years ago

a new rule added in the style editor doesn't show up in the inspector

Categories

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

Product:
DevTools
Component:
Inspector: Rules
Type:
defect

Tracking

(firefox50 fixed, firefox51 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 50
Tracking Flags:
Tracking Status
firefox50 --- fixed
firefox51 --- verified

People

(Reporter: tromey, Assigned: nchevobbe)

Details

(Whiteboard: [btpp-fix-later])

Attachments

(2 files)

css-to.html
208 bytes, text/html
Details
Bug 1240038 - Listen for changes when a StyleSheetActor is created.
58 bytes, text/x-review-board-request
tromey
: review+
Details
Attached file css-to.html — 
Open the attached html.
Open the inspector, make sure <body> is selected.
Open the style editor and add a new rule:

body {
  background-color: seagreen;
}

Now switch back to the inspector.
The new rule should show up, but it does not.

Selecting the <a> and reselecting <body> will show the rule.
The bug here is that the page style actor only looks for style-applied events
on sheets that already contribute a rule to a node.  Instead I think the page
style actor should watch all the sheets, and then emit stylesheet-updated event
if (1) the sheet already contributed a rule (this covers deleting a rule) or
(2) the sheet now contributes a rule.

Another approach might be to watch all the sheets and always emit events, regardless
of what has changed.  This would be less efficient but would be simpler to implement.
Triaging as P2, this is a pretty serious problem, but does not qualify as a P1. It'd be nice to fix this soon-ish though, now that we have style-editor/rule-view synchronization.
(filter on CLIMBING SHOES).
Priority: -- → P2
Whiteboard: [btpp-fix-later]
Assignee: nobody → chevobbe.nicolas
A listener was added for stylesheet changes only if the stylesheet already
contributed to the page before the modification. So if there was a stylesheet
without any rule matching a node of the page, when a rule was created in it for
the current selected node in the markup view, the rule view wasn't refreshed.
The fix here is to emit an event, from the TabActor, when a StyleSheetActor is created,
and listen to such event to watch for changes in it, in order to update the rule view.
Modify an existing rule view/stylesheet editor sync test to make sure this bug is fixed.

Review commit: https://reviewboard.mozilla.org/r/58646/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58646/
Attachment #8761475 - Flags: review?(ttromey)
This is a simple fix to this bug, but it works well.
TRY seems okay https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b6515fc7857&selectedJob=22205983 .

For the test, I was planning to add a new one, but the existing test already test the sync between the stylesheet editor and the rule view, and in order to test this bug, I just add to remove the existing rule in the stylesheet of the test page (without my patch, removing said rule will cause the test to timeout). This looks like a quick win for me and I found it good enough, ut feel free to tell me if we should have a more complete test (e.g. select a node, add a new stylesheet, modify it, wait for the correct event on the rule view — it's quite similar to the one we have though).

Like Tom says in Comment 1, it would be nice to only emit the event if the stylesheet was contributing or now contribute to the page. Unfortunately, I did not found an easy/performant way to do this. The only thing I can think about is looping through the rules of the stylesheet, and see if at least one match a node on the page (and then, cache the result somewhere in order to detect if the page was contributing when some rule is deleted). I'm not sure this would be more efficient than to always emit the event, but again, if you disagree, I'll try to do this.
Attachment #8761475 - Flags: review?(ttromey) → review+
Comment on attachment 8761475 [details]
Bug 1240038 - Listen for changes when a StyleSheetActor is created.

https://reviewboard.mozilla.org/r/58646/#review55880

Thank you for the patch, and for your patience.  This review took me longer than I expected because, despite the small size of the patch, it took a while for me to convince myself that I understood it.

I think this is ok.  However, it needs one comment update, noted below.

::: devtools/server/actors/webbrowser.js:2044
(Diff revision 1)
>      }
>      let actor = new StyleSheetActor(styleSheet, this);
>      this._styleSheetActors.set(styleSheet, actor);
>  
>      this._tabPool.addActor(actor);
> +    events.emit(this, "stylesheet-added", actor);

TabActor has a long introductory comment explaining all the events it emits.  This comment needs an update now.
Comment on attachment 8761475 [details]
Bug 1240038 - Listen for changes when a StyleSheetActor is created.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58646/diff/1-2/
I fixed what Tom suggested, rebased (see Comment 6) and pushed to TRY again https://treeherder.mozilla.org/#/jobs?repo=try&revision=911b32ea1826&selectedJob=22315256. TRY is good expect known intermittents.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/b76ee691ca9b
Listen for changes when a StyleSheetActor is created. r=tromey
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b76ee691ca9b
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
No longer blocks: top-inspector-bugs
I have reproduced this bug with Nightly 46.0a1(2015-12-29) on Windows 10, 64 bit!

The Bug's fix is now verified on Aurora 50.0a2.

Build ID 	20160803004014
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0

[bugday-20160803]
I have reproduced this bug on Nightly 46.0a1(2016-01-15) (Build ID: 20160115030235) on Linux, 64 bit!

The bug's fix is now verified on Latest Nightly 51.0a1!

Build ID: 20160803030226
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:51.0) Gecko/20100101 Firefox/51.0
OS: Ubuntu 16.04, Linux 4.4.0-31-generic

As this bug’s fix is also verified on Windows 10, 64 bit (comment 10), I am marking this as verified!
Status: RESOLVED → VERIFIED
QA Whiteboard: [bugday-20160803]
status-firefox51: --- → verified
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: