911678 - Inspector - inline style rules do not populate the CSSRuleView immediately

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Brian Grinstead [:bgrins]]

If you edit the style attribute on the selected element in the inspector, then the inline rules do not show up in the CSS Rule View.  You need to select another element and reselect the first for it to show up.

Comment 1

[Brian Grinstead [:bgrins]]

Also, if the style changes from a script while it is selected, you do not see the changes immediately.  See this demo page: http://fiddle.jshell.net/bgrins/XBzWn/show/:

    <div style="border: solid 10px black;">random</div>
    <script>
    var el = document.querySelector("div");

    var props = ["color", "backgroundColor", "borderColor"];
    var vals = ["red", "green", "blue"];

    setInterval(function() {
        var random1 = Math.floor(Math.random() * props.length);
        var random2 = Math.floor(Math.random() * vals.length);
        el.style[props[random1]] = vals[random2];
    }, 1000);
    </script>

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: style-911678.patch

This is working, but I'm still trying to figure out some test failures on styleinspector/test/browser_ruleview_update.js.  It is relying on the promise returned from a manually invoked call to ruleView.nodeChanged.  This test is the only place that the returned promise is used from that function.  And really, it is because of this bug that it needs to do that (it is basically reselecting the same node after each attribute change).

Since this patch fixes this bug, I'm considering rewriting the test to listen for the inspector "rule-view-refreshed" event and removing the promise returned by nodeChanged

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: style-911678-1.patch

Things this patch does (functionality):

* When an element is selected in the markup view and its 'style' attribute changes, reflect those changes in the rule view and computed style view immediately without needing to reselect the node in the markup view.
* When other attributes change that affect the CSS rules applied to an element (generally "class" or "id", but it could be any attribute being selected in CSS), update the rule view and computed style view to show only the currently applied rules.

Things this patch does (in code):

* Adds an immediateLayoutChange function to inspector-panel, that will emit an event causing the rule view and computed style view to refresh.
* Call this function inside of the mutation observer in markupview, when an attribute changes on the selected node.
* Add more coverage to the browser_inspector_changes test, including changes to rule view while element is selected. 
* Update the browser_ruleview_update test that was relying on the behavior fixed in this bug, by listening to the "rule-view-refreshed" event instead of the nodeChanged() promise.  Do not return a promise from nodeChanged anymore - this test was the only place using that functionality.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=78557beca5b4

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 807985 [details] [diff] [review]
style-911678-1.patch

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

r+, but only with the simple change below... just a little housekeeping.

::: browser/devtools/styleinspector/test/browser_ruleview_update.js
@@ +160,5 @@
>  }
>  
>  function finishTest()
>  {
>    inspector = ruleView = null;

You have changed rule to a global and are storing a rule object in it. That is fine but you will need to set it to null here to release the object.

Comment 5

[Brian Grinstead [:bgrins]]

Attachment: style-911678-2.patch

Set `rule` to null to cleanup styleinspector/test/browser_ruleview_update.js test.  Resolved conflicts from latest fx-team.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=0c1c4b04d9c2

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/f9fd3a1b03ed

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f9fd3a1b03ed

Comment 8

[Carsten Book [:Tomcat]]

backed this out from inbound because of errors like https://tbpl.mozilla.org/php/getParsedLog.php?id=28270942&tree=Mozilla-Inbound on Mac OSX where we had a nearly 80 % orange in Mochitest Browser Chrome - investigating if this backout fix the problem

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Carsten Book [:Tomcat] from comment #8)
> backed this out from inbound because of errors like
> https://tbpl.mozilla.org/php/getParsedLog.php?id=28270942&tree=Mozilla-
> Inbound on Mac OSX where we had a nearly 80 % orange in Mochitest Browser
> Chrome - investigating if this backout fix the problem

I went ahead and pushed latest fx-team (containing the changes) to try: https://tbpl.mozilla.org/?tree=Try&rev=3bcc3c78c403.  OSX 10.6 debug seems to be failing, but maybe from another test this time (see details at bottom of comment).  I do see from the original log that it was failing on browser_inspector_changes.js (changed in this patch) - is the problem now resolved after the backout?

   > TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser-window/browser_about.js | Test timed out
   > Bug 632290 - Intermittent browser_about.js | Test timed out | Found unexpected add-ons manager window still open TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser-window/browser_about.js | Found unexpected add-ons manager window still open
   > Bug 632290 - Intermittent browser_about.js | Test timed out | Found unexpected add-ons manager window still open

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/be4d13d375e9

Comment 11

[Carsten Book [:Tomcat]]

hey :)

yeah seems ok now, thanks for fixing this!

(In reply to Brian Grinstead [:bgrins] from comment #9)
> (In reply to Carsten Book [:Tomcat] from comment #8)
> > backed this out from inbound because of errors like
> > https://tbpl.mozilla.org/php/getParsedLog.php?id=28270942&tree=Mozilla-
> > Inbound on Mac OSX where we had a nearly 80 % orange in Mochitest Browser
> > Chrome - investigating if this backout fix the problem
> 
> I went ahead and pushed latest fx-team (containing the changes) to try:
> https://tbpl.mozilla.org/?tree=Try&rev=3bcc3c78c403.  OSX 10.6 debug seems
> to be failing, but maybe from another test this time (see details at bottom
> of comment).  I do see from the original log that it was failing on
> browser_inspector_changes.js (changed in this patch) - is the problem now
> resolved after the backout?
> 
>    > TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser-
> window/browser_about.js | Test timed out
>    > Bug 632290 - Intermittent browser_about.js | Test timed out | Found
> unexpected add-ons manager window still open TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser-
> window/browser_about.js | Found unexpected add-ons manager window still open
>    > Bug 632290 - Intermittent browser_about.js | Test timed out | Found
> unexpected add-ons manager window still open

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to Carsten Book [:Tomcat] from comment #11)
> hey :)
> 
> yeah seems ok now, thanks for fixing this!
> 

Was this problem resolved on its own? I didn't actually make any changes to the patch - just pushed to try again.  It looked like there was just a problem on OSX 10.6 debug with a different test, so I wasn't sure if it was just an issue with that particular configuration failing on tests, or if this patch is specifically causing problems.

Comment 13

[Brian Grinstead [:bgrins]]

Attachment: style-911678-3.patch

Mike, this is a minor update from the patch that is already r+ (Attachment 808597 [details] [diff]).  Basically, I modified the test to wait for the layout-change event *before* listening to computed-view-refreshed (as this was firing earlier from other changes in certain OSX builds).

This has been pushed to try here: https://tbpl.mozilla.org/?tree=Try&rev=ca8e65ec7a7e.  There are quite a few oranges, but none of them are on related tests (retriggered a bunch of tests just to be sure).

Comment 14

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 816250 [details] [diff] [review]
style-911678-3.patch

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

I see that the oranges are nothing to do with this patch so r+.

Comment 15

[Brian Grinstead [:bgrins]]

Attachment: style-911678-4.patch

Same patch, but fixes conflicts when applying.  Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=785c9dbae521

Comment 16

[Brian Grinstead [:bgrins]]

Attachment: style-911678-5.patch

Rebuilt off latest code and pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=c14527f9d575.

Comment 17

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 817134 [details] [diff] [review]
style-911678-5.patch

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

Ah, you were listening for an extra event.

Comment 18

[Brian Grinstead [:bgrins]]

Y(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #17)
> Comment on attachment 817134 [details] [diff] [review]
> style-911678-5.patch
> 
> Review of attachment 817134 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ah, you were listening for an extra event.

Yes, after pulling down the latest code the test began to fail when listening for just "inspector-updated", so listening for both that and "rule-view-refreshed" fixed it locally, but caused failures on try.  Then switching to *just* "rule-view-refreshed" instead of waiting for both fixed it both locally and remotely.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/a8177c0528e6

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a8177c0528e6