1142207 - inspector displays variables incorrectly

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Tom Tromey :tromey]

Attachment: css-t2.html

Open the attached HTML and then open the inspector.
In the rule view, the value of the "background" property is
totally mangled.  See the attached screen shot.

Comment 1

[Tom Tromey :tromey]

Attachment: value.png

Comment 2

[Tom Tromey :tromey]

Attachment: add "^" to REGEX_CSS_VAR

The parsing loop only wants to match at the start, but REGEX_CSS_VAR
was missing the leading "^".  This fixes the immediate bug; which I
think is a regression, as my system firefox doesn't have this problem.

REGEX_CSS_VAR also does not handle the defaulted form like
var(--something, default value).  However this can't be matched with a
regexp, so I think will need to wait until I reimplement this to use
the tokenizer.

Comment 3

[Tom Tromey :tromey]

(In reply to Tom Tromey :tromey from comment #2)

> REGEX_CSS_VAR also does not handle the defaulted form like
> var(--something, default value).  However this can't be matched with a
> regexp, so I think will need to wait until I reimplement this to use
> the tokenizer.

I tried a case like this and couldn't make it cause a problem.
So I think perhaps this patch is ready.

Comment 4

[Tom Tromey :tromey]

Comment on attachment 8576667 [details] [diff] [review]
add "^" to REGEX_CSS_VAR

I wasn't sure if this required a test case.
Nothing in toolkit/devtools seems to test this code,
but maybe it is indirectly tested elsewhere?

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8576667 [details] [diff] [review]
add "^" to REGEX_CSS_VAR

Review of attachment 8576667 [details] [diff] [review]:
-----------------------------------------------------------------

Code change looks good to me.
Yeah as you suspected, this is tested somewhere else: /browser/devtools/styleinspector/test
This folder contains tests for the rule-view and computed-view (and style-inspector in general).
Most tests are integration UI tests, so they test the output-parser indirectly, but, we do have one unit-test-like test in there: browser_styleinspector_output-parser.js
As you can see in this test, there's a list of test cases with a css property name, a value, and a callback that's executed when the output-parser has finished parsing the value and returned a document fragment.
At the very least, you could add one item in the test case list:
    {
      name: "background",
      value: "rgb(255, var(--g-value), 192)",
      test: fragment => {
        is(fragment.textContent, "rgb(255, var(--g-value), 192)");
      }
    }
But I suspect you've found a lot of edge cases were variables weren't supported correctly, so feel free to add more test cases there.

Comment 6

[Tom Tromey :tromey]

Attachment: add "^" to REGEX_CSS_VAR

Thanks for pointing out the tests.  I'm sorry I didn't find them
earlier.

Here's a new patch with a couple of new tests.  I added one for
default arguments -- which I expected not to work, but which did, so
hurray.

I haven't found other corner cases in this exact area.  I had thought
perhaps bug 1142194 was, but it appears to originate elsewhere.

Comment 7

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=99a7a3edf14b

Comment 8

[Patrick Brosset <:pbro>]

Comment on attachment 8578044 [details] [diff] [review]
add "^" to REGEX_CSS_VAR

Review of attachment 8578044 [details] [diff] [review]:
-----------------------------------------------------------------

Cool, thanks.

Comment 9

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/83645820fc28

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/83645820fc28