Closed Bug 911678 Opened 7 years ago Closed 7 years ago

Inspector - inline style rules do not populate the CSSRuleView immediately

Categories

(DevTools :: Inspector, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 27

People

(Reporter: bgrins, Assigned: bgrins)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 5 obsolete files)

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.
Assignee: nobody → bgrinstead
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>
Attached patch style-911678.patch (obsolete) — Splinter Review
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
Attached patch style-911678-1.patch (obsolete) — Splinter Review
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
Attachment #807929 - Attachment is obsolete: true
Attachment #807985 - Flags: review?(mratcliffe)
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.
Attachment #807985 - Flags: review?(mratcliffe) → review+
Attached patch style-911678-2.patch (obsolete) — Splinter Review
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
Attachment #807985 - Attachment is obsolete: true
Attachment #808597 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/f9fd3a1b03ed
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/f9fd3a1b03ed
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
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
(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
Flags: needinfo?(cbook)
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
Flags: needinfo?(cbook)
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 922146
Attached patch style-911678-3.patch (obsolete) — Splinter Review
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).
Attachment #808597 - Attachment is obsolete: true
Attachment #816250 - Flags: review?(mratcliffe)
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+.
Attachment #816250 - Flags: review?(mratcliffe) → review+
Attached patch style-911678-4.patch (obsolete) — Splinter Review
Same patch, but fixes conflicts when applying.  Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=785c9dbae521
Attachment #816250 - Attachment is obsolete: true
Attachment #816604 - Flags: review+
Rebuilt off latest code and pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=c14527f9d575.
Attachment #816604 - Attachment is obsolete: true
Attachment #817134 - Flags: review?(mratcliffe)
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.
Attachment #817134 - Flags: review?(mratcliffe) → review+
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a8177c0528e6
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.