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

RESOLVED FIXED in Firefox 61

Status

defect
P2
normal
RESOLVED FIXED
2 years ago
8 months ago

People

(Reporter: botond, Assigned: rcaliman, Mentored)

Tracking

unspecified
Firefox 61

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

2 years ago
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.
Priority: -- → P2
Duplicate of this bug: 1444121
Reporter

Updated

a year ago
Summary: Devtools says "Invalid property value" whe it's the property name that's invalid → Devtools says "Invalid property value" when it's the property name that's invalid
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.
Mentor: pbrosset
Assignee

Updated

a year ago
Assignee: nobody → rcaliman

Comment 4

a year ago
mozreview-review
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).
Attachment #8964572 - Flags: review?(pbrosset)
Comment hidden (mozreview-request)

Comment 6

a year ago
mozreview-review
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
Attachment #8964572 - Flags: review?(pbrosset) → review+
Assignee

Updated

a year ago
Keywords: checkin-needed

Comment 7

a year ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed

Comment 8

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a867d0b8c58e
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
QA Whiteboard: [good first verify]

Updated

11 months ago
Product: Firefox → DevTools
No longer blocks: top-inspector-bugs
You need to log in before you can comment on or make changes to this bug.