1411664 - Devtools says "Invalid property value" when it's the property name that's invalid

Status: RESOLVED FIXED

(DevTools :: Inspector: Rules, defect, P2)

Description

[Botond Ballo [:botond]]

STR:
  1. Open Inspector on any page and select any element
  2. In the Rules tab, enter rules with:
      a. an invalid property name, like "shiny: yes"
      b. a valid property name but invalid value, like "overflow: yes"

The warning message in both cases is "Invalid property value". I think in case (a) the warning message should be "Invalid property name", to make it clear that it's the property name that's invalid.

Comment 2

[Patrick Brosset <:pbro>]

Here's how I think we should fix this:

Right now, we use CSS.supports(declaration) to test the validity of a given declaration. This is done here: https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/devtools/server/actors/styles.js#1131
However this does not tell us anything about whether the name or the value were invalid.

So I think we should add one more line like:

decl.isNameValid = CSS.supports(decl.name, "initial");

Indeed, initial is a value that all property support, so if that returns false, then it means decl.name is the one that's invalid.

Now, the following changes will be needed to put this approach in place:
- (optional clean up step): we have some obsolete backward compatibility code in devtools\client\inspector\rules\models\text-property.js' isValid function, which we could clean up because we'll be making changes to this file. In particular, the first if is only useful for pre-49 versions, which we don't support anymore (we support down to the last ESR: 52).
- In the same file, add a isNameValid function, that sort of does the same as isValid, but checks if the isNameValid property even exists, and if it does returns its value (if not, defaults to true).
- In devtools\client\inspector\rules\views\text-property-editor.js, we need to have 2 different warning signs now. We have one right now that has a title tooltip that says "Invalid property value". We also need one that says "Invalid property name". We could just use the same warning sign and just set the title attribute when needed. In any case, we need a new string for it in devtools\shared\locales\en-US\styleinspector.properties (see the rule.warning.title string).
- In this file, the updatePropertyState function is the one responsible for updating this warning sign. So the code there should be changed. In there we should call isNameValid too and if it's false, update the title attribute for the warning sign.

This, plus a new test would be good. We have this test: devtools\client\inspector\rules\test\browser_rules_invalid.js which seems partly related. We should add more checks in there for invalid values, and invalid names.

Comment 3

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1411664 - Check if property name is invalid in rule view and change warning icon title text accordingly.

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

Comment 4

[Patrick Brosset <:pbro>]

Comment on attachment 8964572 [details]
Bug 1411664 - Check if property name is invalid in rule view and change warning icon title text accordingly.

https://reviewboard.mozilla.org/r/233282/#review238964

Thank you Razvan for working on this. I like the approach. We just need so figure out the right way to handle backward compatibility.

::: devtools/client/inspector/rules/models/text-property.js:207
(Diff revision 1)
>      if (!this.rule.domRule.declarations[selfIndex]) {
>        return true;
>      }
>  
> -    return this.rule.domRule.declarations[selfIndex].isValid;
> +    return checkNameOnly
> +      ? this.rule.domRule.declarations[selfIndex].isNameValid

We need to take backward compatibility into account here. Why? Because you can use a certain version of Firefox (say 61) to connect remotely to another version of Firefox (say 52, running on a remote device).
When that happens, the servers runs on the remote device, which means that the actor will not send the `isNameValid` property back with the list of declarations in the protocol response.
We still maintain backward compatibility up to the latest ESR version (last one is 52, next one will be 60).
So, you need to make the code work even if `isNameValid` is not defined.
I'm guessing defaulting to true is the right thing to do here.

One idea is to abstract all this away into the devtools/shared/fronts/styles.js file. This is the front-end side of the StyleRuleActor. Maybe it could expose a `getDeclaration(index)` method, or something like this, that knows how to deal with older versions of the actor.

Note that it already does this somehow with `get declarations()` which returns an empty array when the list of declarations isn't returned by the actor
(which, btw, we should remove because now we don't support FF49 anymore, which added this declarations array).

Comment 5

[Razvan Caliman [:rcaliman]]

Comment on attachment 8964572 [details]
Bug 1411664 - Check if property name is invalid in rule view and change warning icon title text accordingly.

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

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8964572 [details]
Bug 1411664 - Check if property name is invalid in rule view and change warning icon title text accordingly.

https://reviewboard.mozilla.org/r/233282/#review239266

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a867d0b8c58e
Check if property name is invalid in rule view and change warning icon title text accordingly. r=pbro

Comment 8

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/a867d0b8c58e