Inspector - inline style rules do not populate the CSSRuleView immediately

RESOLVED FIXED in Firefox 27

Status

RESOLVED FIXED
5 years ago
6 months ago

People

(Reporter: bgrins, Assigned: bgrins)

Tracking

(Blocks: 2 bugs)

unspecified
Firefox 27
x86
Mac OS X
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

5 years ago
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)

Updated

5 years ago
Blocks: 831711
(Assignee)

Updated

5 years ago
Assignee: nobody → bgrinstead
(Assignee)

Comment 1

5 years ago
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>
(Assignee)

Comment 2

5 years ago
Created attachment 807929 [details] [diff] [review]
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
(Assignee)

Comment 3

5 years ago
Created attachment 807985 [details] [diff] [review]
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
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+
(Assignee)

Comment 5

5 years ago
Created attachment 808597 [details] [diff] [review]
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
Attachment #807985 - Attachment is obsolete: true
Attachment #808597 - Flags: review+
(Assignee)

Updated

5 years ago
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
Last Resolved: 5 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
(Assignee)

Comment 9

5 years ago
(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)
(Assignee)

Comment 12

5 years ago
(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.
(Assignee)

Updated

5 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

5 years ago
Blocks: 922146
(Assignee)

Comment 13

5 years ago
Created attachment 816250 [details] [diff] [review]
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).
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+
(Assignee)

Comment 15

5 years ago
Created attachment 816604 [details] [diff] [review]
style-911678-4.patch

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+
(Assignee)

Comment 16

5 years ago
Created attachment 817134 [details] [diff] [review]
style-911678-5.patch

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+
(Assignee)

Comment 18

5 years ago
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/integration/fx-team/rev/a8177c0528e6
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/a8177c0528e6
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]

Updated

6 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.