2068483 - registered custom property declaration shows Invalid at computed value time error icon when value uses attr()

Status: RESOLVED FIXED

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

Description

[Jake Archibald [:jakea]]

Attachment: screenshot

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

Here, devtools tells me that attr(data-foo) isn't a string. but it is.

attr(data-foo raw-string) also gives me the warning.

Comment 1

[Julian Descottes [:jdescottes]]

Hi Jake,

Is this a duplicate of Bug 2068479? I can't reproduce the issue here, maybe the STRs are not correct?

Comment 2

[Jake Archibald [:jakea]]

Attachment: video

I'm pretty sure this is a different issue. It surfaces in different UI.

I think maybe it wasn't clear from the screenshot. Hopefully this helps.

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

This is where we compute the invalid at computed value time information https://searchfox.org/firefox-main/rev/61c7b6d2fe5fc598e573df6528447ab48bc027b3/devtools/server/actors/style-rule.js#606-619

if (
  registeredProperty &&
  // For now, we don't handle variable based on top of other variables. This would
  // require to build some kind of dependency tree and check the validity for
  // all the leaves.
  !decl.value.includes("var(") &&
  !InspectorUtils.valueMatchesSyntax(
    targetDocument,
    decl.value,
    registeredProperty.syntax
  )
) {
  // if the value doesn't match the syntax, it's invalid
  decl.invalidAtComputedValueTime = true;

As hinted in the comment, we're already bailing out if the declaration value is using var() because we would need to get the substituted value, which we couldn't easily have at the time. In the case of attr(), we're not bailing out, and InspectorUtils.valueMatchesSyntax returns false.

For the CSS explainers tooltip, we do handle substitution functions (added in Bug 2041622), and so maybe we could spawn off another InspectorUtils method that would give us the substituted value (Bug 2070166), which we could then pass to InspectorUtils.valueMatchesSyntax.

An alternative would be to do the substitution directly in InspectorUtils.valueMatchesSyntax, but I think there's purpose on having a dedicated method to get the substituted value (see Bug 2064405)

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Actually, let's add another guard here, like we do for var(), as this is an easy fix and would remove this incorrect information
We'll handle this properly in Bug 2070169, which might take a bit more time to do

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 2068483 - [devtools] Don't check registered property syntax for declaration value using substitution functions. r=#devtools.

In the StyleActor, we're checking for invalid at computed value time registered property declarations.
We were already skipping such check if the declaration was using var().
This patch extends the guard so we won't check for invalid syntax if
the value is using any of the substitution function (var()/attr()/env()).
A test is added to make sure we're not displaying the IACVT warning icon
in such case.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/284799d203f7
https://hg.mozilla.org/integration/autoland/rev/b4c42fb38b8d
[devtools] Don't check registered property syntax for declaration value using substitution functions. r=devtools-reviewers,bomsy.

Comment 7

[agoloman]

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