1069829 - Properties validity shouldn't be checked on the front-end (aka stop using domUtils.cssPropertyIsValid in the client code)

Status: RESOLVED FIXED

(DevTools :: Inspector: Rules, enhancement, P1)

Description

[Patrick Brosset <:pbro>]

Being remotely connected to a debuggee means that the toolbox shouldn't be making too much assumptions about what the debuggee is. Especially in light of addons like fxdt-adapters that let you connect to webkit and blink debuggees.

One such assumption is in the rule-view, when checking of a given property/value is valid:

  /**
   * Validate this property. Does it make sense for this value to be assigned
   * to this property name? This does not apply the property value
   *
   * @param {string} [aValue]
   *        The property value used for validation.
   *        Defaults to the current value for this.prop
   *
   * @return {bool} true if the property value is valid, false otherwise.
   */
  isValid: function(aValue) {
    let name = this.prop.name;
    let value = typeof aValue == "undefined" ? this.prop.value : aValue;
    let val = parseSingleValue(value);

    let style = this.doc.createElementNS(HTML_NS, "div").style;
    let prefs = Services.prefs;

    // We toggle output of errors whilst the user is typing a property value.
    let prefVal = prefs.getBoolPref("layout.css.report_errors");
    prefs.setBoolPref("layout.css.report_errors", false);

    let validValue = false;
    try {
      style.setProperty(name, val.value, val.priority);
      validValue = style.getPropertyValue(name) !== "" || val.value === "";
    } finally {
      prefs.setBoolPref("layout.css.report_errors", prefVal);
    }
    return validValue;
  }

This is front-end code, meaning that it will always run in a Gecko browser, which means that properties like "-webkit-transition" will always be invalid.

Properties validity should always be checked on the server-side.

Comment 1

[Tom Tromey :tromey]

The as-authored series (bug 984880) added a few more instances of this.
There are more in the rule view, but also one in the rule rewriting code
and also some in output-parser.js.

Comment 2

[Patrick Brosset <:pbro>]

Filter on CLIMBING SHOES

Comment 3

[Patrick Brosset <:pbro>]

This bug is now about removing usages of domUtils.cssPropertyIsValid in the rule-view. The code in comment 0 does not exist anymore and has been replaced with domUtils.cssPropertyIsValid.

Here are all the usages in the tree: https://dxr.mozilla.org/mozilla-central/search?q=domUtils.cssPropertyIsValid&redirect=false&case=false
So, basically, all related to the inspector (mostly css rule view).

We need to go over each one and decide:

- does it need to be moved to the server-side
if the returned value we expect from cssPropertyIsValid should depend on the browser currently hosting the debuggee page, then  yes, this should move to the server, and therefore become async, which could be complex, but needed,

- or can it stay on the client-side
one example of this is _findCompatibleUnits here:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/inplace-editor.js#592
I don't expect this to behave differently depending on the browser engine running the page, so it is safe to keep this on the client-side, but replace it with a JS implementation (see bug 1263287).

Comment 4

[Tom Tromey :tromey]

The instance in the rule rewriter can easily be made async.

Another implementation option is to have the server send a property database to the client at startup.  This would avoid difficulties making anything async.

Comment 5

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1069829 - 2 - Don't use cssPropertyIsValid in the rule-view UI, get the info from the server instead; r?tromey

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

Comment 6

[Patrick Brosset <:pbro>]

Not sure how much of this I'll be able to do, so I'm assigning this bug to me just yet. But here's a first (very simple) part.
For this particular usage of cssPropertyIsValid I believe there's no need to go to the server, so I implemented a simple property validation using a DOM element locally.
I did not want to move this to a shared helper function just because I don't think we should reuse this in many places. Most of the times, the right answer is to go to the server.

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8741775 [details]
MozReview Request: Bug 1069829 - 2 - Don't use cssPropertyIsValid in the rule-view UI, get the info from the server instead; r?tromey

Potentially supported units could change I guess, but as you say in Comment 3 it's probably safe across platforms.  Forwarding review to Tom though for a second opinion.

Comment 8

[Tom Tromey :tromey]

Comment on attachment 8741775 [details]
MozReview Request: Bug 1069829 - 2 - Don't use cssPropertyIsValid in the rule-view UI, get the info from the server instead; r?tromey

https://reviewboard.mozilla.org/r/46745/#review43407

Looks good, thanks.

Comment 9

[Patrick Brosset <:pbro>]

Pending try push for part 1: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b415f588c933&group_state=expanded

Comment 10

[Pulsebot]

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

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/99fbe8621fa7

Comment 12

[Patrick Brosset <:pbro>]

Sorry, forgot to add the leave-open keyword when I landed part 1.
The first patch only removed one instance of cssPropertyIsValid in the code, there are other ones in the rule-view, filterWidget and output-parser.
The one in the rule-view is the most important and complex one I think. It is used to display a warning sign next to properties that are not valid. Obviously this varies across browsers and should therefore come from the server.
Our actor only returns the full css text for a rule, which means that we must parse them into property:value pairs on the client and check the validity of each pair on the client.
I checked the chromedevtools' protocol, and the corresponding method not only returns the css text but also the parsed declarations and information about their validity.
I believe we should do the same here.

Comment 13

[Patrick Brosset <:pbro>]

I started working on part 2, making the StyleRuleActor's form contain the parsed declarations.

Comment 14

[Patrick Brosset <:pbro>]

Comment on attachment 8741775 [details]
MozReview Request: Bug 1069829 - 2 - Don't use cssPropertyIsValid in the rule-view UI, get the info from the server instead; r?tromey

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46745/diff/1-2/

Comment 15

[Patrick Brosset <:pbro>]

Oh noes.. mozreview thinks this is a new version of the first patch, so it R+'d it and didn't ask for a new review.

Comment 16

[Tom Tromey :tromey]

Comment on attachment 8741775 [details]
MozReview Request: Bug 1069829 - 2 - Don't use cssPropertyIsValid in the rule-view UI, get the info from the server instead; r?tromey

https://reviewboard.mozilla.org/r/46745/#review46731

::: devtools/client/inspector/rules/test/browser_rules_select-and-copy-styles.js:67
(Diff revision 2)
>    info("Checking that _Copy() returns the correct clipboard value");
>  
>    let expectedPattern = "    margin: 10em;[\\r\\n]+" +
>                          "    font-size: 14pt;[\\r\\n]+" +
> -                        "    font-family: helvetica,sans-serif;[\\r\\n]+" +
> -                        "    color: rgb\\(170, 170, 170\\);[\\r\\n]+" +
> +                        "    font-family: helvetica, sans-serif;[\\r\\n]+" +
> +                        "    color: #AAA;[\\r\\n]+" +

I'm curious why the spelling of the color had to change here.  I couldn't readily tell from the patch.  Was this an existing authored styles bug?  Or is it a regression now?

::: devtools/server/actors/styles.js:1195
(Diff revision 2)
>    },
>  
>    // True if this rule supports as-authored styles, meaning that the
>    // rule text can be rewritten using setRuleText.
>    get canSetRuleText() {
> -    // Special case about:PreferenceStyleSheet, as it is
> +    return this.type === ELEMENT_STYLE ||

I have some vague worry about this.  I thought there was a problem that could arise if as-authored-style editing was done on element styles.  But, I can't remember what that was and I think perhaps always getting the authored text from the element may address whatever it was.

Maybe something like disabling and then re-enabling a property won't work?  Not sure.

::: devtools/server/actors/styles.js:1473
(Diff revision 2)
>  
> -    this.authoredText = newText;
> -    yield parentStyleSheet.update(cssText, false, UPDATE_PRESERVING_RULES);
> +      yield parentStyleSheet.update(cssText, false, UPDATE_PRESERVING_RULES);
> +    }
> +
> +    this.authoredText = newText;

Is it necessary to set the authored text here for an element style?

If so, I think a comment explaining why would be nice.

If not, I think it's better to just leave this out, since it's a bit confusing -- because when computing the form we always re-fetch the style from the element anyway (which is the right thing to do).

Comment 17

[Patrick Brosset <:pbro>]

Thanks Tom for the review. Sorry for the delay here, but making element style rule as authored made some tests fail, and I've been spending some time fixing those.
I have something that works locally. I'm going to ask for another review in a bit because these were not only obvious mistakes.

Comment 18

[Patrick Brosset <:pbro>]

Comment on attachment 8741775 [details]
MozReview Request: Bug 1069829 - 2 - Don't use cssPropertyIsValid in the rule-view UI, get the info from the server instead; r?tromey

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46745/diff/2-3/

Comment 19

[Patrick Brosset <:pbro>]

Comment on attachment 8741775 [details]
MozReview Request: Bug 1069829 - 2 - Don't use cssPropertyIsValid in the rule-view UI, get the info from the server instead; r?tromey

Tom, could you give this another look?
You will be mostly interested in this interdiff: https://reviewboard.mozilla.org/r/46745/diff/2-3/

- I had to change layout.js because when you edit margin, border or padding in this view, the element style is changed. Before my patch, this was using the RuleModificationList helper because element style was not as authored. Now it use the RuleRewriter, and that caused some tests to fail and I fixed all the issues.
Mostly, I had the problem of the RuleRewriter only supporting one change. So what I did was just sequence them in layout.js.

- browser_rules_add-property_02.js was failing intermittently (only when run in a test suite), so I cleaned it up a little bit (it was defining unused styles), and added some safe measures to avoid rejected promises. I'm still unsure if this is due to my changes though.

- browser_styleinspector_context-menu-copy-color_01.js this test uses both the rule-view and computed-view. Because the rule-view now uses authored styles for the element style rule, I just used the rgb format to make sure the test expected the same string in both views.

- test_styles-modify.html was failing also because we are now using the RuleRewriter for element styles. I fixed the test but made sure it tested the same thing as before.

- styles.js: I am not the author of the changes made to this file in this interdiff. Turns out they disabled the auto-hiding of rebase changes in mozreview recently, so I guess these changes are shown here because of that.

Comment 20

[Tom Tromey :tromey]

Comment on attachment 8741775 [details]
MozReview Request: Bug 1069829 - 2 - Don't use cssPropertyIsValid in the rule-view UI, get the info from the server instead; r?tromey

https://reviewboard.mozilla.org/r/46745/#review48167

Looks good to me.  Thank you.

::: devtools/server/actors/styles.js:978
(Diff revisions 2 - 3)
> +   *
> +   * @param {Document} document
> +   *        The document in which the style element should be appended.
>     * @returns DOMElement of the style tag
>     */
> -  get styleElement() {
> +  getStyleElement: function(document) {

I think eslint wants a space before the "(".

Comment 21

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c96822cd293f&group_state=expanded

Comment 22

[Pulsebot]

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

Comment 23

[Pulsebot]

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

Comment 24

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

https://hg.mozilla.org/mozilla-central/rev/eb721f4d509f
https://hg.mozilla.org/mozilla-central/rev/3b16eb206789