1456779 - "Computed" view value does not get updated when 3-pane-inspector is on

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Description

[Jens]

The Computed view does not get updated any more, when fiddling with values in the Rules view when the 3-Pane inspector is on. When the 3-Pan inspector is toggled off, then it works (probably because a click on the "Computed" view just updates which does not happen when it's already visible)

STR:

1. Go to https://codepen.io/nachtigall/pen/RyRovV with Nightly and 3-pane inspector on. So have "Rules" and "Computed" visible.
2. Select `.floating` in the Rules view
3. See `float` being `right` in the Computed rule 
4. In the Rules view change `.floating` to `left`

AR:
Computed still shows being the old value `right`

ER:
Computed should update to the new value `left`

Interestingly, the updating works for the `Layout` view.

Comment 1

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 1456779 - Computed view should refresh on reflows.

Review commit: https://reviewboard.mozilla.org/r/251426/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/251426/

Comment 2

[Patrick Brosset <:pbro>]

Comment on attachment 8985947 [details]
Bug 1456779 - Computed view should refresh on reflows.

https://reviewboard.mozilla.org/r/251426/#review257842

::: devtools/client/inspector/computed/computed.js:1481
(Diff revision 1)
>      }
>  
>      const isInactive = !this.isSidebarActive() &&
>                       this.inspector.selection.nodeFront;
>      if (isInactive) {
> +      this.inspector.reflowTracker.untrackReflows(this, this.refresh);

You should also do this in the `destroy` function.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e2eaa601a6d
Computed view should refresh on reflows. r=pbro

Comment 4

[Cristina Coroiu [:ccoroiu]]

Backed out changeset 5e2eaa601a6d (bug 1456779) for devtools failures at devtools/client/inspector/computed/test/browser_computed_getNodeInfo.js

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/6d6d308fb49d157ffde75c8ed7523470075bb6f0

Failure push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=5e2eaa601a6d98b5c6d892dc1a6f409145edef7e

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=183853540&repo=mozilla-inbound&lineNumber=1996

20:21:51     INFO -  273 INFO TEST-PASS | devtools/client/inspector/computed/test/browser_computed_getNodeInfo.js | undefined assertion name -
20:21:51     INFO -  274 INFO Testing a matched rule selector (parentmatch)
20:21:51     INFO -  Buffered messages finished
20:21:51    ERROR -  275 INFO TEST-UNEXPECTED-FAIL | devtools/client/inspector/computed/test/browser_computed_getNodeInfo.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/inspector/computed/test/browser_computed_getNodeInfo.js:140 - TypeError: nodeInfo is null
20:21:51     INFO -  Stack trace:
20:21:51     INFO -  assertNodeInfo@chrome://mochitests/content/browser/devtools/client/inspector/computed/test/browser_computed_getNodeInfo.js:140:7
20:21:51     INFO -  @chrome://mochitests/content/browser/devtools/client/inspector/computed/test/browser_computed_getNodeInfo.js:176:5
20:21:51     INFO -  Async*Tester_execTest/<@chrome://mochikit/content/browser-test.js:1098:34
20:21:51     INFO -  async*Tester_execTest@chrome://mochikit/content/browser-test.js:1089:16
20:21:51     INFO -  nextTest/<@chrome://mochikit/content/browser-test.js:991:9
20:21:51     INFO -  SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
20:21:51     INFO -  276 INFO Leaving test bound
20:21:52     INFO -  277 INFO Removing tab.
20:21:52     INFO -  278 INFO Waiting for event: 'TabClose' on [object XULElement].
20:21:52     INFO -  279 INFO Got event: 'TabClose' on [object XULElement].
20:21:52     INFO -  280 INFO Tab removed and finished closing

Comment 5

[Jens]

Since this already has a reviewed+ patch and just a test failure I wonder if this has just been forgotten... Work on this bug looks like 80% done and would be sad if it bitrottens. Gabriel, any chance you could see why the test fails?

Comment 6

[Patrick Brosset <:pbro>]

This bug has not been updated in the last 3 months. Resetting the assignee field.
Please, feel free to pick it up again and add a comment outlining your plans for it if you do still intend to work on it.
This is just trying to clean our backlog of bugs and make bugs available for people.

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

Haven't had a chance to work on this. The work is mostly done except for figuring out the test failure.

Comment 10

[Patrick Brosset <:pbro>]

:jdescottes analyzed the problem and provided a useful summary in bug 1579155 comment 1.

My analysis is that the update of the computed view here should be triggered from a stylesheet-updated event.
However this event is not emitted by the pageStyle actor when the update comes from the RuleView, because it
triggers an update of type UPDATE_PRESERVING_RULES. And only style updates with type UPDATE_GENERAL are
turned into stylesheet-updated events [...]

Please see the entire comment in the other bug for more info.

Comment 11

[Patrick Brosset <:pbro>]

Worth noting that the STR in bug 1579155 mentions changing a color. Doing so most probably won't cause a reflow. So, while the approach in the original patch here will work for properties that do cause reflows, it won't work for colors, etc.

Comment 12

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1456779: Update the computed value view when the rule in rule view is updated in case of 3-pane. r?pbro!

Comment 13

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1456779: Add test for the computed view updating. r?pbro!

Depends on D46119

Comment 14

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 1456779: Update the computed view when the 3 pane rule view is updated. r=daisuke

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bb1556ff54d6
Update the computed view when the 3 pane rule view is updated. r=daisuke

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3918541135bd
Add test for the computed view updating. r=gl

Comment 17

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/bb1556ff54d6
https://hg.mozilla.org/mozilla-central/rev/3918541135bd