Closed Bug 1069829 Opened 5 years ago Closed 4 years ago

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

Categories

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

enhancement

Tracking

(firefox49 fixed)

RESOLVED FIXED
Firefox 49
Iteration:
49.1 - May 9
Tracking Status
firefox49 --- fixed

People

(Reporter: pbro, Assigned: pbro)

References

(Depends on 2 open bugs)

Details

(Whiteboard: [btpp-2016-Q3] [devtools-html])

Attachments

(1 file)

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.
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.
Filter on CLIMBING SHOES
Priority: -- → P2
Whiteboard: [btpp-2016-Q3]
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).
Component: Developer Tools: Inspector → Developer Tools: CSS Rules Inspector
Summary: [rule view] Properties validity shouldn't be checked on the front-end → Properties validity shouldn't be checked on the front-end (aka stop using domUtils.cssPropertyIsValid in the client code)
Severity: normal → enhancement
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.
Priority: P2 → --
Whiteboard: [btpp-2016-Q3] → [btpp-2016-Q3] [devtools-html]
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 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.
Attachment #8741775 - Flags: review?(bgrinstead) → review?(ttromey)
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.
Attachment #8741775 - Flags: review?(ttromey) → review+
https://hg.mozilla.org/mozilla-central/rev/99fbe8621fa7
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: qe-verify?
Priority: -- → P2
I started working on part 2, making the StyleRuleActor's form contain the parsed declarations.
Assignee: nobody → pbrosset
Status: REOPENED → ASSIGNED
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/
Attachment #8741775 - Attachment description: MozReview Request: Bug 1069829 - 1 - Remove a usage of domUtils.cssPropertyIsValid in inplace-editor; r=bgrins → MozReview Request: Bug 1069829 - 2 - Don't use cssPropertyIsValid in the rule-view UI, get the info from the server instead; r?tromey
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.
Attachment #8741775 - Flags: review+ → review?(ttromey)
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).
Attachment #8741775 - Flags: review?(ttromey) → review+
Flags: qe-verify? → qe-verify-
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 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 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.
Attachment #8741775 - Flags: review+ → review?(ttromey)
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 "(".
Attachment #8741775 - Flags: review?(ttromey) → review+
Iteration: --- → 49.1 - May 9
Priority: P2 → P1
No longer blocks: top-inspector-bugs
Duplicate of this bug: 1188614
See Also: → 1327615
See Also: → 1328016
Depends on: 1328017
Depends on: 1328019
Depends on: 1374092
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.