Closed Bug 1265796 Opened 7 years ago Closed 7 years ago

replace inIDOMUtils.getSubpropertiesForCSSProperty

Categories

(DevTools :: Framework, enhancement, P1)

Product:
DevTools
Component:
Framework
Type:
enhancement

Tracking

(firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 52
Iteration:
52.1 - Oct 3
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: tromey, Assigned: tromey)

References

(Blocks 1 open bug)

Details

(Whiteboard: [devtools-html])

Attachments

(4 files, 8 obsolete files)

Bug 1265796 - add .eslintrc to devtools/shared/platform/content/;
58 bytes, text/x-review-board-request
gregtatum
: review+
Details
Bug 1265796 - remove dummy document from rule view;
58 bytes, text/x-review-board-request
gregtatum
: review+
Details
Bug 1265796 - add CSS subproperty info to CssProperties;
58 bytes, text/x-review-board-request
gregtatum
: review+
Details
Bug 1265796 - fix mach devtools-css-db for linux, debug builds;
58 bytes, text/x-review-board-request
gregtatum
: review+
Details 
Replace inIDOMUtils.getSubpropertiesForCSSProperty for the devtools
de-chrome-ification project.
This is used in \devtools\client\inspector\rules\models\text-property.js in order to display the list of long-hand properties for a short-hand property in the rule-view panel.

Something like this is done:

var domUtils = Cc["@mozilla.org/inspector/dom-utils;1"].getService(Ci.inIDOMUtils);
domUtils.getSubpropertiesForCSSProperty(name);

For instance, for "transition", the following list is returned:
transition-property,transition-duration,transition-timing-function,transition-delay

I think the best course of action would probably be to get this information from the back-end instead (the getApplied method should send it along), and then we can switch to using getSubpropertiesForCSSProperty if this information isn't present (connected to an old backend).
Flags: qe-verify-
Priority: -- → P1
Assignee: nobody → gtatum
Status: NEW → ASSIGNED
Iteration: --- → 49.1 - May 9
I was thinking about this while working on bug 1069829 (which touches some of the same files).

The whole logic in text-property.js/updateComputed needs changing. We shouldn't create a dummy HTML element here anymore. We should add a new actor method to PageStyleActor, something like getComputedLongHandProperties (or something shorter).

The nice thing here is that these properties are only shown when you expand the short-hand, so we can totally rewrite this to be async.

@Greg: this is your first bug in the area of the code, don't hesitate to ping me or Tom about this, we can help.
Iteration: 49.1 - May 9 → 49.2 - May 23
Iteration: 49.2 - May 23 → 49.3 - Jun 6
Ok, here is a patch for taking out getSubpropertiesForCSSProperty. This builds upon
the CssPropertiesActor work.
Attachment #8758430 - Flags: review?(pbrosset)
Comment on attachment 8758430 [details] [diff] [review]
replace inIDOMUtils.getSubpropertiesForCSSProperty

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

I have one concern with this approach. On one hand, I like the approach of just adding more data to the css-properties actor, that seems like a good fit here. On the other hand, I believe the getApplied method (on the PageStyleActor) should actually return this data.

This is what the chrome protocol seems to be doing: https://chromedevtools.github.io/debugger-protocol-viewer/1-1/CSS/#type-CSSStyle
and the closer we can be to that, the easier we make our lives in the future I guess.

Whenever we call pageStyleActor.getApplied to get the list of applied css rules for a node we should get, not only the list of properties in each rule, but also the list of subproperties and their values.

This could be done in StyleRuleActor.form.

I guess the good thing is that if we did this, then that would also take care of my comment in text-property.js, because we wouldn't need the dummyElement anymore.
I think since we're going to do it later anyway, we might as well do it now.

What do you think?

::: devtools/client/inspector/rules/models/text-property.js
@@ +79,2 @@
>      // and see what the computed style looks like.
>      let dummyElement = this.rule.elementStyle.ruleView.dummyElement;

I mentioned this in comment 2, we will need to get rid of this dummyElement logic. It is a DOM node created in the toolbox to get the value for sub properties. However, it depends on the browser we are currently inspecting, not the one the inspector runs in now.
This can of course be done as a separate bug.

::: devtools/client/shared/css-properties-db.js
@@ +424,5 @@
> +/**
> + * This list has the properties with only 1 subproperty removed.
> + */
> +exports.subpropertiesList = {
> +  "all": [

"all" seems to contain all longhand properties. Do we actually need it in the inspector? What uses it? Could we instead derive it from the other data here? It seems to me like it shouldn't be here, and that the css-properties actor should not send it either.
Attachment #8758430 - Flags: review?(pbrosset)
Iteration: 49.3 - Jun 6 → 50.1
Iteration: 50.1 → 50.2
I really wasn't looking at the broader context of this problem. I was only looking at replacing the single function. Patrick your thoughts on addressing the dummyElement make sense to me, and I feel like the getSubpropertiesForCSSProperty should be part of making this call async and handling it on the server.

I'm going to need some broader context to how the inspector works to address this properly. I'm taking some time to go through the tool's codebase to get a better understanding of how all of this fits together.
Making this async is stumping me. I've attempted a few approaches, but it ends up touching a lot of code in ways that scare me. I have a bunch of code locally on this, but it's not anything worth sharing yet.

Approach 1: Make TextProperty.prototype.updateComputed() async. I started down this path, and I may explore it some more. The problem is that the constructor calls this function, which injects lots of async assumptions everywhere.

Approach 2: Make the `computed` array a promise that resolves to the list of computed elements. This approach so far is leading to some weird async errors. It's a little scary on knowing what is effected, but I might be able to pursue this route.
(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #8)
> Approach 1: Make TextProperty.prototype.updateComputed() async. I started
> down this path, and I may explore it some more. The problem is that the
> constructor calls this function, which injects lots of async assumptions
> everywhere.
This sounds like the right approach to me, and I don't see why the TextProperty constructor calls this function. The long-hand properties should only be constructed lazily, when shown the first time.
Blocks: 1266097
Ok, this is the approach I'm taking. I've still got a few issues that I need to sort out in the tests. This will resolve bug 1266097 as well. I moved the dummyElement testing to the server. In addition I took away the hiddenDOM logic, and I'm creating the element actually on the debuggee, but not attaching it to the DOM. As far as I can tell all of the computed elements still get calculated correctly. I also probably need to add a test for my new getComputedProperties property.

The failing tests I've got locally are:

> INFO TEST-UNEXPECTED-FAIL | devtools/client/inspector/rules/test/browser_rules_add-property-commented.js | The 'background-color' property is disabled. -
> INFO TEST-UNEXPECTED-FAIL | devtools/client/inspector/rules/test/browser_rules_add-property-commented.js | The 'height' property is enabled. -
> INFO TEST-UNEXPECTED-FAIL | devtools/client/inspector/rules/test/browser_rules_cycle-color.js | Color  is still displayed as a hex value. - Got blue, expected #00f
> INFO TEST-UNEXPECTED-FAIL | devtools/client/inspector/rules/test/browser_rules_multiple-properties-unfinished_01.js | Test timed out -
> INFO TEST-UNEXPECTED-FAIL | devtools/client/inspector/rules/test/browser_rules_pseudo-element_01.js | First added property is on back of array - Got [object Object], expected [object Object]
> INFO TEST-UNEXPECTED-FAIL | devtools/client/inspector/rules/test/browser_rules_pseudo-element_01.js | Second added property is on back of array - Got [object Object], expected [object Object]
> INFO TEST-UNEXPECTED-FAIL | devtools/client/inspector/rules/test/browser_rules_pseudo-element_01.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/inspector/rules/test/head.js:631 - TypeError: textProp.editor is undefined

I also pushed to try to see if anything else is breaking.

Another thing I'm not sure on is how to be backwards compatible with targets that don't have the PageStyleActor's getComputedProperties method.
Attachment #8758430 - Attachment is obsolete: true
Iteration: 50.2 - Jul 4 → 50.3 - Jul 18
Assignee: gtatum → nobody
Status: ASSIGNED → NEW
Iteration: 50.3 - Jul 18 → ---
Priority: P1 → P2
Assignee: nobody → gtatum
Status: NEW → ASSIGNED
Iteration: --- → 50.3 - Jul 18
Priority: P2 → P1
Iteration: 50.3 - Jul 18 → 50.4 - Aug 1
Hmm... it looks like the computed properties are needed initially. As the rules are being loaded in they are checked each time for their computed values to know whether or not the value is being overridden as can be seen here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/rules/models/element-style.js?q=path%3Aelement-style&redirect_type=single#242-305

So it looks like we won't be able to refactor and only get this information when you are expanding the shorthand properties.
(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #13)
> Hmm... it looks like the computed properties are needed initially. As the
> rules are being loaded in they are checked each time for their computed
> values to know whether or not the value is being overridden as can be seen
> here:
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/
> rules/models/element-style.js?q=path%3Aelement-
> style&redirect_type=single#242-305
> 
> So it looks like we won't be able to refactor and only get this information
> when you are expanding the shorthand properties.
So, I suggest investigating plan B: basically what I said in comment 5: StyleRuleActor can send the computed sub-properties and their values.
1 - StyleRuleActor is server-side, which is what we want to calculate sub properties and values correctly anyway
2 - Anytime an element is selected in the inspector, a call to PageStyleActor.getApplied is made, and it returns the list of all rules that apply to the element, each one of these rule is a StyleRuleActor instance.
3 - So, if we make the StyleRuleActor.form not only contain the list of text properties (as we do today), but also, for each of them, the corresponding list of sub properties and their computed values, then we're fine: the information will be available to text-property.js right from the start, no need to fake it with a dummy DOM element, no extra async calls to make to the server.
Iteration: 50.4 - Aug 1 → 51.1 - Aug 15
Blocks: 1266847
Blocks: 1291866
Iteration: 51.1 - Aug 15 → 51.2 - Aug 29
Iteration: 51.2 - Aug 29 → 51.3 - Sep 12
Just checking in, this blocks bug 1288835, which blocks bug 1276366, which will hopefully eliminate a rather large class of window leaks. Do we plan on continuing to pursue this?
Flags: needinfo?(gtatum)
I'm still actively working on this bug, it's just taking a long time due to the complexity of adding async behavior to the call stacks.
Flags: needinfo?(gtatum)
Attachment #8766546 - Attachment is obsolete: true
Assignee: gtatum → ttromey
Depends on: 1301078
> So, I suggest investigating plan B: basically what I said in comment 5: StyleRuleActor can send the computed sub-properties and their values.

This did seem promising, so I looked into it a bit today.
The main problem seems to be that neither PageStyleActor nor StyleRuleActor
record the node being examined.  It isn't clear to me that it would be
ok to stick this into the PageStyleActor.
So, barring new information, I think I am going to drop this in favor
of Greg's existing approach.
For browser_rules_cycle-color.js, the bug seems to be that we fetch the "blue"
value from the store, here:

TextProperty@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/rules/models/text-property.js:52:17
Rule.prototype._getTextProperties@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/rules/models/rule.js:476:22
Rule@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/rules/models/rule.js:65:20
ElementStyle.prototype._maybeAddRule<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/rules/models/element-style.js:189:14
More logging shows that perhaps the problem is that we aren't waiting for the
unit change to round-trip through the server before moving on with the test.
Waiting longer helps, but introduces a new failure, which occurs because
TextPropertyEditor._onSwatchCommit calls |this.update| before the rule
rewriting in |_onValueDone| has completed.
No longer blocks: 1291866
Attaching my current patches in case you want to see what they look like.
The earlier patches have changes relative to Greg's, as well.
I consider the "element-style" patch the worst of the bunch FWIW, so if you
just want to see the terribleness, start there.

As discussed in the meeting, I'm going to abandon this approach in favor of
a purely client-side approach for the time being.  I may file a follow-up
inspector bug and reference this bug for future work.
Blocks: 1303746
Attachment #8792119 - Attachment is obsolete: true
Attachment #8792120 - Attachment is obsolete: true
Attachment #8792121 - Attachment is obsolete: true
Comment on attachment 8792118 [details]
Bug 1265796 - add CSS subproperty info to CssProperties;

https://reviewboard.mozilla.org/r/79340/#review78224

Waiting for the new solution we discussed on IRC.
Attachment #8792118 - Flags: review?(gtatum) → review-
Attachment #8787651 - Attachment is obsolete: true
Attachment #8787652 - Attachment is obsolete: true
Attachment #8787653 - Attachment is obsolete: true
Testing showed an eslint nit I overlooked.
Comment on attachment 8792116 [details]
Bug 1265796 - add .eslintrc to devtools/shared/platform/content/;

https://reviewboard.mozilla.org/r/79336/#review78522
Attachment #8792116 - Flags: review?(gtatum) → review+
Comment on attachment 8792991 [details]
Bug 1265796 - fix mach devtools-css-db for linux, debug builds;

https://reviewboard.mozilla.org/r/79804/#review78576

Thanks for getting this working on Linux!

::: devtools/shared/css/generated/generate-properties-db.js:19
(Diff revision 1)
>  // Output JSON
>  dump(JSON.stringify({
>    cssProperties: cssProperties(),
>    pseudoElements: pseudoElements()
>  }));
> +// In a debug build, xpcshell might print extra debugging information,

Nice.
Attachment #8792991 - Flags: review?(gtatum) → review+
Comment on attachment 8792117 [details]
Bug 1265796 - remove dummy document from rule view;

https://reviewboard.mozilla.org/r/79338/#review78606

Hmm... I think we may be able to get rid dummyDocument all together here. Thoughts?

::: devtools/client/inspector/rules/rules.js:793
(Diff revision 3)
>        // ::before and ::after do not have a namespaceURI
>        let namespaceURI = this.element.namespaceURI ||
>            document.documentElement.namespaceURI;
>        this._dummyElement = document.createElementNS(namespaceURI,
>                                                     this.element.tagName);
>        document.documentElement.appendChild(this._dummyElement);

If the non-chrome side of the devtools/shared/platform code is run, then this adds a DOM element to the current document, not a dummy document, which I don't think we want. The dummy element is used in [devtools/client/inspector/rules/models/text-property.js](https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/rules/models/text-property.js#83-86) and code appears to work fine without actually adding the element to the page. Do we even need a dummy document here? Could we just get the panel's reference to the current document? It would be functionally the same unless I'm missing something.

e.g. open the webconsole and run:

```js
var dummyElement = document.createElement('div');
var dummyStyle = dummyElement.style;
dummyStyle.cssText = "";
dummyStyle.setProperty("margin", "1em", "");
console.log(dummyStyle.getPropertyValue("margin-top"));
// "1em"
```
Comment on attachment 8792118 [details]
Bug 1265796 - add CSS subproperty info to CssProperties;

https://reviewboard.mozilla.org/r/79340/#review78608

LGTM!
Attachment #8792118 - Flags: review?(gtatum) → review+
Iteration: 51.3 - Sep 19 → 52.1 - Oct 3
> Do we even need a dummy document here? Could
> we just get the panel's reference to the current document? It would be
> functionally the same unless I'm missing something.

I don't know.  However, I tried removing it all and everything seems to
work fine.  In particular all the rule-view tests pass.
Comment on attachment 8792117 [details]
Bug 1265796 - remove dummy document from rule view;

https://reviewboard.mozilla.org/r/79338/#review78874

::: devtools/client/inspector/rules/rules.js
(Diff revision 4)
> - * engine, we will set properties on a dummy element and observe
> - * how their .style attribute reflects them as computed values.
> - * This function creates the document in which those dummy elements
> - * will be created.
> - */
> -var gDummyPromise;

Nice, this simplifies the code quite a bit. I'm glad this looks like it will work out.
Comment on attachment 8792117 [details]
Bug 1265796 - remove dummy document from rule view;

https://reviewboard.mozilla.org/r/79338/#review78876

Sweet, thanks for figuring out if that would work.
Attachment #8792117 - Flags: review?(gtatum) → review+
It turns out that removing the dummy element promise caused a regression...
lots of subtle ordering dependencies in this code.

This latest version restores the promise and fixes an eslint nit.
Green try run here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ecdd1157c58b85409ef53392ea57a0ee178856bf
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cc97d85a2b35
fix mach devtools-css-db for linux, debug builds; r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/13215faddd07
add .eslintrc to devtools/shared/platform/content/; r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/627e15d6dd96
remove dummy document from rule view; r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/cf4dc9616855
add CSS subproperty info to CssProperties; r=gregtatum
Depends on: 1308467
Blocks: 1311743
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.