Closed Bug 1142207 Opened 7 years ago Closed 7 years ago

inspector displays variables incorrectly

Categories

(DevTools :: Inspector, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(firefox39 fixed)

RESOLVED FIXED
Firefox 39
Tracking Status
firefox39 --- fixed

People

(Reporter: tromey, Assigned: tromey)

Details

Attachments

(3 files, 1 obsolete file)

Attached file 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.
Attached image value.png
Assignee: nobody → ttromey
Attached patch add "^" to REGEX_CSS_VAR (obsolete) — Splinter Review
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.
(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 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?
Attachment #8576667 - Flags: review?(pbrosset)
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.
Attachment #8576667 - Flags: review?(pbrosset)
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.
Attachment #8576667 - Attachment is obsolete: true
Attachment #8578044 - Flags: review?(pbrosset)
Comment on attachment 8578044 [details] [diff] [review]
add "^" to REGEX_CSS_VAR

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

Cool, thanks.
Attachment #8578044 - Flags: review?(pbrosset) → review+
Keywords: checkin-needed
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/83645820fc28
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.