Open Bug 922146 Opened 11 years ago Updated 2 years ago

[rule view] Update matching selectors as style sheets are created/loaded

Categories

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

Product:
DevTools
Component:
Inspector: Rules
Type:
defect

Tracking

(Not tracked)

People

(Reporter: pbro, Unassigned)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 6 obsolete files)

bug922146-refresh-ruleview-on-new-stylesheet V7.patch
(deleted), patch
Details | Diff | Splinter Review 
It is possible to load or even create new stylesheets after the document has loaded, via javascript.

This means that, while a node is selected in the markup-view and its styles are shown on the side panel in the rule-view, these styles may change.

The markup-view already listens to markup mutations today in order to react to changes in the DOM tree. A similar sort of mechanism is therefore needed in the rule-view (and computed-view).

Here's how a new style may be added:

> var sheet = document.createElement("STYLE");
> sheet.id = "new-style-sheet";
> sheet.type = "text/css";
> sheet.appendChild(document.createTextNode("div {display:none;}"));
> document.getElementsByTagName("HEAD")[0].appendChild(sheet);
Blocks: 911209
Assignee: nobody → pbrosset
Depends on: 911678
So, essentially, we should refresh any of the rule, computed, font and layout views when:

- a new stylesheet is appended to the document (a new <style> tag or <link> tag)
- an existing stylesheet's content is changed

I think a MutationObserver should work for this and we already have one in the inspector actor, unless there is another event or platform API we could use for this.

Paul? Any idea for this?
Flags: needinfo?(paul)
(In reply to Patrick Brosset [:pbrosset] from comment #1)
> So, essentially, we should refresh any of the rule, computed, font and
> layout views when:
> 
> - a new stylesheet is appended to the document (a new <style> tag or <link> tag)

You can use the StyleSheetAdded / StyleSheetRemoved events.

> - an existing stylesheet's content is changed

I wish we could easily do that… but afaik, it's not possible from the JS world.

Cameron, on idea how we could do that?
Flags: needinfo?(paul) → needinfo?(cam)
Changes to the existing text of a style sheet, e.g. myStyleElement.textContent = "...", should result in a StyleSheetRemoved event followed by a StyleSheetAdded event.  Is that sufficient, or do you need to track that they are due to a change?
Flags: needinfo?(cam)
That should be fine. I'll experiment with those events.
Thanks Cameron and Paul for your help.
For info, the events described above were added in bug 839103
I'm working on a patch that I will very soon be attaching for review.

That patch will do the following:
- listen to StyleSheetAdded/Removed events at PageStyleActor level
- use these events to send protocol events down to the client
- listen to these protocol events at style inspector (rule + computed views), font inspector, layout view levels
- use these events to refresh the views

The next obvious candidate would be the style editor, but this will be done in a follow-up bug as it has some dependencies.

Another candidate related to this is synchronizing changes between the style inspector and style editor (in both directions), but this is probably a bigger change too, so while I'll investigate this here, I'll probably file another bug.
Blocks: 947119
Bug 947119 will be focusing on refreshing the style editor when the events mentioned above occur.
Ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=7390be94dcd8
Attachment #8343662 - Flags: review?(bgrinstead)
(In reply to Patrick Brosset [:pbrosset] from comment #8)
> Created attachment 8343662 [details] [diff] [review]
> bug922146-refresh-ruleview-on-new-stylesheet.patch
> 
> Ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=7390be94dcd8

Looks like there may be issues with the bc tests in the debug builds here
If I remember correctly, memory leaks can be detected in debug builds but not in normal builds, that's why only debug builds fail here.
I think the reason is that I've added a couple of event listeners in the style actor:

> let browser = this.inspector.tabActor.browser;
> browser.addEventListener("StyleSheetAdded", this._onStyleSheetAdded, true);
> browser.addEventListener("StyleSheetRemoved", this._onStyleSheetRemoved, true);

And these listeners are being removed when the style actor gets destroyed.
I'm guessing that when we run bc tests, many of them are ended in a way that actors don't get destroyed properly, hence the listeners never get removed.
(In reply to Patrick Brosset [:pbrosset] from comment #10)
> If I remember correctly, memory leaks can be detected in debug builds but
> not in normal builds, that's why only debug builds fail here.
> I think the reason is that I've added a couple of event listeners in the
> style actor:
> 
> > let browser = this.inspector.tabActor.browser;
> > browser.addEventListener("StyleSheetAdded", this._onStyleSheetAdded, true);
> > browser.addEventListener("StyleSheetRemoved", this._onStyleSheetRemoved, true);
> 
> And these listeners are being removed when the style actor gets destroyed.
> I'm guessing that when we run bc tests, many of them are ended in a way that
> actors don't get destroyed properly, hence the listeners never get removed.

Could be - I would build in debug mode and check on that test locally.  Hopefully it will be reproducible and you can narrow it down to a particular line.  If you can't reproduce it locally you could always add logging when the handlers are removed and push to try to see if the remove handlers are being fired.
I managed to reproduce the error locally with a debug build and found a way to fix it.
I need to do some more rework on this though, so will be attaching a new patch probably tomorrow.
V2 of this patch changes the following things:

- gets rid of the memory leak detected during debug builds
- refactors the way the stylesheet-added/removed events are added when a new-root mutation occurs
- fixed a number of tests that were dynamically adding stylesheets to the content doc, therefore triggering the stylesheet-added event and refreshing the rule view before the test started
Attachment #8343662 - Attachment is obsolete: true
Attachment #8343662 - Flags: review?(bgrinstead)
Attachment #8345226 - Flags: review?(bgrinstead)
New ongoing try build for this patch: https://tbpl.mozilla.org/?tree=Try&rev=393af396eb30
Try build says I need to work more on this still ... The situation is better, but browser_ruleview_inherit.js and browser_ruleview_override.js fail intermittently in debug builds.
Comment on attachment 8345226 [details] [diff] [review]
bug922146-refresh-ruleview-on-new-stylesheet V2.patch

I'm working on a new patch to solve the intermittent try failures.
I will be attaching it as soon as the new try is green.
Attachment #8345226 - Flags: review?(bgrinstead)
Fixed the remaining failing test.
Ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=3f1a51ba8570
Attachment #8345226 - Attachment is obsolete: true
Attachment #8349922 - Flags: review?(bgrinstead)
Comment on attachment 8349922 [details] [diff] [review]
bug922146-refresh-ruleview-on-new-stylesheet V3.patch

Review of attachment 8349922 [details] [diff] [review]:
-----------------------------------------------------------------

I've taken a look through the code and left a couple of notes.  Here is some general usage feedback:

* I was using this URL to test: http://jsfiddle.net/bgrins/fhr6h/, it doesn't seem to update the styles.  However, when loading the test page directly (not in a frame) it does work: http://fiddle.jshell.net/bgrins/fhr6h/show/.  Maybe there is an issue with iframes - it is only listening for changes on the root document.  Not sure if this is something we want to tackle here.
* The highlighter doesn't update on change, even when the dimensions of the element have changed (you can see this here: http://fiddle.jshell.net/bgrins/fhr6h/show/).  This could be handled as a follow up after Bug 916443 lands, since there are many changes to the highlighter there.

Also, it seems that this will degrade fine with older versions of the server, since it is just listening for new events.  But just double checking to make sure this will be OK - is there anything here that will cause issues on older versions of the server?

::: browser/devtools/fontinspector/test/browser_bug922146_refresh_on_stylesheet_change.js
@@ +65,5 @@
> +  }
> +}
> +
> +function generateCssText() {
> +  var font = ["Arial", "Verdana", "Times New Roman", "Georgia"][

Couldn't we just hardcode a font here instead of randomly generating one, since it is only listening for changes?  Or if we want changes to definitely happen, could hardcode each font individually in each styleSheetChanges function to guarantee that it changes.  Same goes for the other tests that use random values in their generateCssText functions.

::: browser/devtools/styleinspector/rule-view.js
@@ +192,5 @@
>      }).then(entries => {
>        // Make sure the dummy element has been created before continuing...
>        return this.dummyElementPromise.then(() => {
>          if (this.populated != populated) {
> +          // Don't care anymore. Element was changed before that completed

Can you update this comment to explain a little bit more what has happened to get into this state.  Element was changed before *what* completed?

::: browser/devtools/styleinspector/test/browser_bug705707_is_content_stylesheet.js
@@ +66,5 @@
>    for (let rule of ruleView._elementStyle.rules) {
>      rule.editor.addProperty("font-weight", "bold", "");
>    }
>  
> +  executeSoon(() => {

Is there a reason this is in an exectuteSoon?

::: toolkit/devtools/server/actors/styles.js
@@ +97,5 @@
> +
> +    // Listen for stylesheet events on the document, and make sure to listen
> +    // on the right document when page navigates
> +    this._onRootFrameLoad = () => {
> +      this.tabBrowser = this.inspector.tabActor.browser;

From what I understand, we don't have access to browser in Firefox OS: https://bugzilla.mozilla.org/show_bug.cgi?id=916443#c81
Attachment #8349922 - Flags: review?(bgrinstead)
Thanks Brian, I'm working on the various things you highlighted and will reply soon.

The main problems the previous patch has is that it won't work with documents nested in frames and on FirefoxOS. Both problems are, I think, related and linked to the fact that I used the `this.inspector.tabActor.browser` object to list to the StyleSheet[Added|Removed] events.

@Cameron: The only javascript usage occurence of these events I could find is in this test:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_bug839103.js#10
It suggests the events should be listened to on the gBrowser object. This doesn't seem like something we can do on FirefoxOS.
Also, is there a way to get notified of these events even in children frames?
Flags: needinfo?(cam)
(In reply to Brian Grinstead [:bgrins] from comment #18)
> Comment on attachment 8349922 [details] [diff] [review]
> bug922146-refresh-ruleview-on-new-stylesheet V3.patch
>
> Review of attachment 8349922 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> I've taken a look through the code and left a couple of notes.  Here is some
> general usage feedback:
>
> * I was using this URL to test: http://jsfiddle.net/bgrins/fhr6h/, it
> doesn't seem to update the styles.  However, when loading the test page
> directly (not in a frame) it does work:
> http://fiddle.jshell.net/bgrins/fhr6h/show/.  Maybe there is an issue with
> iframes - it is only listening for changes on the root document.  Not sure
> if this is something we want to tackle here.
This should definitely be handled in this bug. See comment 19.

> * The highlighter doesn't update on change, even when the dimensions of the
> element have changed (you can see this here:
> http://fiddle.jshell.net/bgrins/fhr6h/show/).  This could be handled as a
> follow up after Bug 916443 lands, since there are many changes to the
> highlighter there.
I didn't notice this problem however. When I did test, the highlighter was responding to width/height changes, which is expected as it listens to mozAfterPaint events. As you pointed out anyway, if there are problems let's handle them after 916443 lands.

> Also, it seems that this will degrade fine with older versions of the
> server, since it is just listening for new events.  But just double checking
> to make sure this will be OK - is there anything here that will cause issues
> on older versions of the server?
I've tested this case and it works fine. As you said, using events makes this easily backward-compatible.

> browser/devtools/fontinspector/test/
> browser_bug922146_refresh_on_stylesheet_change.js
> @@ +65,5 @@
> > +  }
> > +}
> > +
> > +function generateCssText() {
> > +  var font = ["Arial", "Verdana", "Times New Roman", "Georgia"][
>
> Couldn't we just hardcode a font here instead of randomly generating one,
> since it is only listening for changes?  Or if we want changes to definitely
> happen, could hardcode each font individually in each styleSheetChanges
> function to guarantee that it changes.  Same goes for the other tests that
> use random values in their generateCssText functions.
You're right, randomly generating values doesn't help better testing here, I've hard coded CSS text.

> ::: browser/devtools/styleinspector/rule-view.js
> @@ +192,5 @@
> >      }).then(entries => {
> >        // Make sure the dummy element has been created before continuing...
> >        return this.dummyElementPromise.then(() => {
> >          if (this.populated != populated) {
> > +          // Don't care anymore. Element was changed before that completed
>
> Can you update this comment to explain a little bit more what has happened
> to get into this state.  Element was changed before *what* completed?
Done

> browser/devtools/styleinspector/test/browser_bug705707_is_content_stylesheet.
> js
> @@ +66,5 @@
> >    for (let rule of ruleView._elementStyle.rules) {
> >      rule.editor.addProperty("font-weight", "bold", "");
> >    }
> >
> > +  executeSoon(() => {
>
> Is there a reason this is in an exectuteSoon?
After 2 weeks of xmas break, nope, can't recall a reason.
Pretty sure it used to fail for some reason, maybe on debug try builds. I'll remove it and test again.

> ::: toolkit/devtools/server/actors/styles.js
> @@ +97,5 @@
> > +
> > +    // Listen for stylesheet events on the document, and make sure to listen
> > +    // on the right document when page navigates
> > +    this._onRootFrameLoad = () => {
> > +      this.tabBrowser = this.inspector.tabActor.browser;
>
> From what I understand, we don't have access to browser in Firefox OS:
> https://bugzilla.mozilla.org/show_bug.cgi?id=916443#c81
See comment 19 for this one too. I'm not sure yet how to proceed, but once I know, I'll test this out by patching my FxOS device.
> browser/devtools/styleinspector/test/browser_bug705707_is_content_stylesheet.
> js
> @@ +66,5 @@
> >    for (let rule of ruleView._elementStyle.rules) {
> >      rule.editor.addProperty("font-weight", "bold", "");
> >    }
> >  
> > +  executeSoon(() => {
> 
> Is there a reason this is in an exectuteSoon?
Actually, I remember now. Adding a property, now that we have server-side events for when stylesheets get changed, will end up refreshing the rule-view, so we need to wait until that occurs before going on with the test.
However, I can replace the nasty executeSoon with a better inspector.once("rule-view.refreshed", ...) event listener instead.
(In reply to Patrick Brosset [:pbrosset] from comment #19)
> @Cameron: The only javascript usage occurence of these events I could find
> is in this test:
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/
> general/browser_bug839103.js#10
> It suggests the events should be listened to on the gBrowser object. This
> doesn't seem like something we can do on FirefoxOS.
> Also, is there a way to get notified of these events even in children frames?

I haven't worked with the style sheet events myself, but it looks like they get dispatched to the document itself, based on the code in nsDocument.cpp, and presumably they then bubble up to the browser element?

An alternative would be to register an nsIDocumentObserver, which has the same style related notification callbacks.
Flags: needinfo?(cam)
(In reply to Cameron McCormack (:heycam) from comment #22)
> An alternative would be to register an nsIDocumentObserver, which has the
> same style related notification callbacks.

Which you register on the document object with nsIDocument::AddObserver.


For the events, note that you need to set |doc.styleSheetChangeEventsEnabled = true| to get the style sheet events dispatching and that that property (and the events) are chrome only.
Here's a new take on this bug. The implementation differs from the last patch:

- now, a new server method is available to start watching stylesheet changes in a particular document
- this is now used by the inspector-panel to start watching changes in the document of the currently selected node
- once watching, the PageStyle actor sends events to the client when stylesheets change in the corresponding document

This make it work on sites that have iframes.
Also, the server-side code doesn't use the browser object anymore but instead the document and window, making it compatible with FxOS.
Attachment #8349922 - Attachment is obsolete: true
Attachment #8356116 - Flags: review?(bgrinstead)
Disregard the last try build, I must have pushed the wrong thing.
New ongoing try: https://tbpl.mozilla.org/?tree=Try&rev=951297cea0a2
Should be fine this time, I forgot to check for a null variable and for some reason, tests were passing locally, can't figure out why.
Anyway, here's the new patch
Attachment #8356116 - Attachment is obsolete: true
Attachment #8356116 - Flags: review?(bgrinstead)
Attachment #8356210 - Flags: review?(bgrinstead)
Comment on attachment 8356210 [details] [diff] [review]
bug922146-refresh-ruleview-on-new-stylesheet V5.patch

Review of attachment 8356210 [details] [diff] [review]:
-----------------------------------------------------------------

Can you rebase the patch?  It is failing to apply on latest.

::: toolkit/devtools/server/actors/styles.js
@@ +105,5 @@
> +    // Stores the documents for which stylesheets events are observed
> +    this.styleSheetChangesDocs = new Set;
> +  },
> +
> +  destroy: function() {

When is this actor destroyed - is it on every new page load, or only once devtools closes?  If it persists for longer than a page load, I'm thinking we should unwatchStyleSheetChanges more aggressively.

@@ +181,5 @@
> +    // Accessing doc or its window may fail on tab close or navigation.
> +    // When that occurs, we want to silently fail as it doesn't matter if this
> +    // is not executed
> +    try {
> +      doc.styleSheetChangeEventsEnabled = true;

Should this be doc.styleSheetChangeEventsEnabled = false?
Attachment #8356210 - Flags: review?(bgrinstead)
> Can you rebase the patch?  It is failing to apply on latest.
Done.

> ::: toolkit/devtools/server/actors/styles.js
> @@ +105,5 @@
> > +    // Stores the documents for which stylesheets events are observed
> > +    this.styleSheetChangesDocs = new Set;
> > +  },
> > +
> > +  destroy: function() {
> 
> When is this actor destroyed - is it on every new page load, or only once
> devtools closes?  If it persists for longer than a page load, I'm thinking
> we should unwatchStyleSheetChanges more aggressively.
That's interesting, I just checked and indeed, the PageStyleActor has the lifetime of the InspectorActor, which isn't that of a given page since it's global. So it means my change will end up adding and adding documents to observe until the devtools server is stopped.
I was originally thinking that this actor would be destroyed/re-instantiated at each page load. I guess it never needed too since it's not storing any information.
I'll need to think more about this.

> @@ +181,5 @@
> > +    // Accessing doc or its window may fail on tab close or navigation.
> > +    // When that occurs, we want to silently fail as it doesn't matter if this
> > +    // is not executed
> > +    try {
> > +      doc.styleSheetChangeEventsEnabled = true;
> 
> Should this be doc.styleSheetChangeEventsEnabled = false?
Yes, changed.
Now the PageStyleActor listens to the walker's ProgressListener's frame unload event in order to clean up the stylesheet events.

There was also a bug in the rule-view whereby quickly calling several times the nodeChanged function would end up in a messed-up display with old rules still there.
I added a simplistic queue (which isn't one, really just a flag telling if a refresh should be done after the current one is completed) that fixes this.
Attachment #8356210 - Attachment is obsolete: true
Attachment #8362507 - Flags: review?(bgrinstead)
Comment on attachment 8362507 [details] [diff] [review]
bug922146-refresh-ruleview-on-new-stylesheet V6.patch

Review of attachment 8362507 [details] [diff] [review]:
-----------------------------------------------------------------

The changes since the last review look good.  I've got a question about backwards compat with older version of the server in the notes, and I'm not sure what the best solution for this is, but want to make sure we've at least considered it before landing.

I've also started a new test since the last one seems to be for a different patch: https://tbpl.mozilla.org/?tree=Try&rev=004c00a4d3dc.

::: browser/devtools/inspector/inspector-panel.js
@@ +397,5 @@
> +    // Start watching for stylesheet changes in the new node's document (if any)
> +    // so that sidebar panels can update themselves when that happens
> +    let styleSheetChangePromise = promise.resolve();
> +    if (selection) {
> +      styleSheetChangePromise = this.pageStyle.watchStyleSheetChanges(selection);

This could possibly cause test failures and runtime errors on older versions of the server.  The inspector updating function inside of this mainThread.dispatch seems to only be used for events in tests, but if this is running on an older version of the server then I believe watchStyleSheetChanges will throw an error ("unrecognizedPacketType"?).  You may have an idea if / how we should change this.  My first thought it to set a trait indicating that the feature is available and only call the function if so (otherwise just dispatch the update immediately as it used to).

Since we don't yet have tests running in this environment that I know of, it is hard to say for sure if this is an issue (or if changes we make will actually fix potential issues).  At the least it would remove console.errors when running older versions of the server, I guess.

::: toolkit/devtools/server/actors/inspector.js
@@ +1836,5 @@
>        this.queueMutation({
>          type: "newRoot",
>          target: this.rootNode.form()
>        });
> +      events.emit(this, "root-frame-load");

root-frame-load and root-frame-unload don't seem to be used.  Should they still be here?

::: toolkit/devtools/server/actors/styles.js
@@ +154,5 @@
>    /**
> +   * Start watching for added/removed stylesheets in a given content document
> +   * and fire events to the client when this happens.
> +   * Note that when a stylesheet's content is changed, both a removed and added
> +   * events will be fired.

s/events/event
Attachment #8362507 - Flags: review?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #32)
> Comment on attachment 8362507 [details] [diff] [review]
> bug922146-refresh-ruleview-on-new-stylesheet V6.patch
> 
> Review of attachment 8362507 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The changes since the last review look good.  I've got a question about
> backwards compat with older version of the server in the notes, and I'm not
> sure what the best solution for this is, but want to make sure we've at
> least considered it before landing.
> 
> I've also started a new test since the last one seems to be for a different
> patch: https://tbpl.mozilla.org/?tree=Try&rev=004c00a4d3dc.
https://tbpl.mozilla.org/?tree=Try&rev=15acbefe3991 was correct in fact, it's just that when I push to try, I rename the actual patch with the trychooser syntax, so the first one in the list isn't an empty patch.
The one you started seems to fail at a test that wasn't there when I last created the patch (browser_ruleview_multiple_properties.js). I'll check if it fails locally and investigate.

> ::: browser/devtools/inspector/inspector-panel.js
> @@ +397,5 @@
> > +    // Start watching for stylesheet changes in the new node's document (if any)
> > +    // so that sidebar panels can update themselves when that happens
> > +    let styleSheetChangePromise = promise.resolve();
> > +    if (selection) {
> > +      styleSheetChangePromise = this.pageStyle.watchStyleSheetChanges(selection);
> 
> This could possibly cause test failures and runtime errors on older versions
> of the server.  The inspector updating function inside of this
> mainThread.dispatch seems to only be used for events in tests, but if this
> is running on an older version of the server then I believe
> watchStyleSheetChanges will throw an error ("unrecognizedPacketType"?).  You
> may have an idea if / how we should change this.  My first thought it to set
> a trait indicating that the feature is available and only call the function
> if so (otherwise just dispatch the update immediately as it used to).
> 
> Since we don't yet have tests running in this environment that I know of, it
> is hard to say for sure if this is an issue (or if changes we make will
> actually fix potential issues).  At the least it would remove console.errors
> when running older versions of the server, I guess.
You're right, I forgot about this usecase unfortunately... So, yes, a trait would work, but somehow it makes me sad to have to introduce another one. It feels like it'd be better directly asking protocol.js if a particular actor method exists or not.
Another way that I'll try now is not asking the client to call watchStyleSheetChanges but keep everything server-side, so that we can be back to only sending events, which is better for compatibility.

> ::: toolkit/devtools/server/actors/inspector.js
> @@ +1836,5 @@
> >        this.queueMutation({
> >          type: "newRoot",
> >          target: this.rootNode.form()
> >        });
> > +      events.emit(this, "root-frame-load");
> 
> root-frame-load and root-frame-unload don't seem to be used.  Should they
> still be here?
You're correct, they don't seem to be used anywhere. I'll remove them.

> ::: toolkit/devtools/server/actors/styles.js
> @@ +154,5 @@
> >    /**
> > +   * Start watching for added/removed stylesheets in a given content document
> > +   * and fire events to the client when this happens.
> > +   * Note that when a stylesheet's content is changed, both a removed and added
> > +   * events will be fired.
> 
> s/events/event
Done.
So, here's a new take again on this bug. I think this is the right way of doing this now.

There's now no need for the client to ask for watching nodes, so it's backward compatible since it's only based on events.
What happens now is that the PageStyleActor listens to frame load and unload events to start and stop watching stylesheet changes in them.
It also looks at all existing windows when it initializes and remove all remaining windows when it gets destroyed.

This way, we're sure to always watch all stylesheet changes we actually care about, and only these ones.
This will make it possible to later implement bug 911209 to show display:none nodes differently in the markup-view (no matter which frames they're in).
Indeed, up until my v6 patch, only those frames which we selected elements in would have been watched.

There's one remaining point though: the inspector sidebar panels will get refreshed even if stylesheets are added in other frames. We might want to discuss whether this is an important problem or not.
I don't have any idea yet on how to filter these events.
We could send the target document along with the event, but that would require the client to get the document for the currently selected node to compare.
We could also send some stylesheet identifier with the event, so that the rule-view can compare, but this would be useless for the computed/font/layout view.
Any ideas welcome.

Finally, I fixed the browser_ruleview_multiple_properties.js test which was failing because of the simple queuing mechanism I implemented in rule-view.
For some reason, it turns out a "layout-change" event is thrown during that test and that triggered a nodeChanged execution during the refresh of the rule-view.

Ongoing try https://tbpl.mozilla.org/?tree=Try&rev=53963578da68
Attachment #8362507 - Attachment is obsolete: true
Attachment #8363564 - Flags: review?(bgrinstead)
Tests in the try builds seem to pass fine, however I must have introduced a leak in the PageStyleActor cause all debug build report at least 1 leaked docshell.
I'll take a look at this next.
Comment on attachment 8363564 [details] [diff] [review]
bug922146-refresh-ruleview-on-new-stylesheet V7.patch

I forgot this review request was still there.
I'm cancelling it for now because:

- the code introduces a leak (detected in debug try builds)
- the browser toolbox seems to fail with the patch (getting the debuggee's window chromeEventHandler returns undefined)
- I'd like to progress on https://gist.github.com/captainbrosset/8834705 first before doing this change.
Attachment #8363564 - Flags: review?(bgrinstead)
Hi Cameron,
These stylesheetadded/removed events are working great so far and will help the devtools quite a lot. However, I was wondering if there are events being fired when the CSSOM is changed.
Indeed, in the devtools, we would also need to react when some content javascript does e.g. document.styleSheets[0].cssRules[0].setProperty("color", "blue");
The CSSOM spec doesn't define any events. Do we already have something in place similar to how stylesheetadded/removed work?

For info, if you change the CSSOM via node.style, then our tools do show the changes, but that's only because it triggers a DOM node attribute mutation and we listen to this today.
Flags: needinfo?(cam)
Hi Patrick sorry I overlooked your needinfo for a week.

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #37)
> These stylesheetadded/removed events are working great so far and will help
> the devtools quite a lot. However, I was wondering if there are events being
> fired when the CSSOM is changed.
> Indeed, in the devtools, we would also need to react when some content
> javascript does e.g.
> document.styleSheets[0].cssRules[0].setProperty("color", "blue");
> The CSSOM spec doesn't define any events. Do we already have something in
> place similar to how stylesheetadded/removed work?

Yes I believe StyleRuleChanged on an nsIDocumentObserver is called in that case.
Flags: needinfo?(cam)
Priority: -- → P3
Component: Developer Tools: Inspector → Developer Tools: CSS Rules Inspector
Unassigned myself as I'm not working on this.
Assignee: pbrosset → nobody
Product: Firefox → DevTools
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: