2068479 - attr() suggests var() in an attribute isn't valid, but it is

Status: RESOLVED FIXED

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

Description

[Jake Archibald [:jakea]]

Attachment: screenshot

https://random-stuff.jakearchibald.com/attr-prop-test/

Devtools suggests this value is invalid, but it's valid.

Comment 1

[Julian Descottes [:jdescottes]]

Will test again with the patch from Bug 2040912, just in case this helps.

Comment 2

[Jake Archibald [:jakea]]

It's possible, but this also impacts types other than <color>.

Comment 3

[Julian Descottes [:jdescottes]]

(In reply to Julian Descottes [:jdescottes] from comment #1)

Will test again with the patch from Bug 2040912, just in case this helps.

And it doesn't help anyway. (hopefully :nchevobbe will have a better sense if there are existing bugs on file for this next week)

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

This is where we look into any type mismatch https://searchfox.org/firefox-main/rev/61c7b6d2fe5fc598e573df6528447ab48bc027b3/devtools/client/shared/output-parser.js#950-957

if (!InspectorUtils.valueMatchesSyntax(this.#doc, attrValue, syntax)) {
  fallbackValueIsUsed = true;
  attrTypeMismatchText = STYLE_INSPECTOR_L10N.getFormatStr(
    "rule.attributeUnmatchedType",
    `"${attrValue}"`,
    `"${syntax}"`
  );
}

for the STR, attrValue is var(--foo), and so InspectorUtils.valueMatchesSyntax returns false. We should instead have the substituted text from the attribute value. This could be a bit tricky, as we're getting the attribute value from the nodeFront directly https://searchfox.org/firefox-main/rev/61c7b6d2fe5fc598e573df6528447ab48bc027b3/devtools/client/inspector/rules/views/text-property-editor.js#622,628-630,635-636

getAttributeValue: attrName => {
...
  const attribute = nodeFront.attributes.find(
    attr => attr.name === attrName
  );
...
  return attribute.value;
},

which ultimately come from https://searchfox.org/firefox-main/rev/61c7b6d2fe5fc598e573df6528447ab48bc027b3/devtools/server/actors/inspector/node.js#455,465-468

writeAttrs() {
...
  return [...this.rawNode.attributes].map(attr => {
    return { namespace: attr.namespace, name: attr.name, value: attr.value };
  });
}

maybe we could return a substitutedValue property for attributes using substitution functions, using the function we'll add in Bug 2070166
We could have a more immediate fix, not checking the syntax if the attribute value contains substitution functions, like we're doing in Bug 2068483, and then have another bug to properly handle those.

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

let's go with the simple fix here, and handle things properly in Bug 2070443

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 2068479 - [devtools] Don't check for type mismatch in attr() when attribute value uses substitution functions. r=#devtools.

This is just a quick fix to avoid false negative type mismatch
when the attribute value uses var()/attr()/env().
This also mean we won't properly flag when the fallback value
is being used, in case the attribute substituted value
doesn't match the expected syntax, but I guess it's less likely
to happen.
A proper fix will be done in Bug 2070443.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/a006eddcd784
https://hg.mozilla.org/integration/autoland/rev/ecdda0a77ad8
[devtools] Don't check for type mismatch in attr() when attribute value uses substitution functions. r=devtools-reviewers,bomsy.

Comment 8

[Sandor Molnar[:smolnar]]

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