Closed Bug 1142194 Opened 5 years ago Closed 5 years ago

erroneous "Invalid property value" due to mis-parsing string

Categories

(DevTools :: Inspector, defect)

defect
Not set
normal

Tracking

(firefox39 fixed)

RESOLVED FIXED
Firefox 39
Tracking Status
firefox39 --- fixed

People

(Reporter: tromey, Assigned: tromey)

Details

Attachments

(2 files, 3 obsolete files)

Attached file css-t1.html (obsolete) —
Consider the attached HTML page.
It has an inline style:

	body:after {
	content: "quotes '\" gold";
	}

In the inspector, this is given an "Invalid property value" warning sign.

One issue here might be the way the regexp in output-parser.js is defined:

const REGEX_QUOTES = /^".*?"|^".*|^'.*?'|^'.*/;

This doesn't handle backslash quoting.

Note that the string is also displayed incorrectly in the view.
There it shows:

   content: "quotes '" gold";

... i.e., something is stripping out the backslash and not adding it back.
Attached file css-t1.html
This time without the unnecessary <script>
Attachment #8576156 - Attachment is obsolete: true
Assignee: nobody → ttromey
This turns out to be a fairly simple buglet in css-parsing-utils.js:quoteString.
I've removed this function for the as-authored series, but meanwhile it doesn't
hurt to fix it.
This changes quoteString to properly quote a double quote, and to
handle backslash as well.

It would be good to also handle \r, \n, and \f, but the tokenizer
currently in-tree doesn't like these, and since I'm upgrading the
tokenizer for a longer-term project, it seemed better to just wait
rather than try to fix it up.  (The new tokenizer is substantially
different, so it isn't a simple matter of backporting a bug fix.)
Attachment #8578097 - Flags: review?(pbrosset)
This update removes a comment from the patch.

I had been looking at the tokenization part of the grammar to
understand strings and didn't notice there is another section about
interpretation.  This section specifies that a backslash-newline
sequence in a string is to be ignored in CSS.  So, the comment was
incorrect.
Attachment #8578097 - Attachment is obsolete: true
Attachment #8578097 - Flags: review?(pbrosset)
Attachment #8578658 - Flags: review?(pbrosset)
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
Comment on attachment 8578658 [details] [diff] [review]
fix quoteString to correctly quote

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

::: browser/devtools/styleinspector/css-parsing-utils.js
@@ +21,3 @@
>    }
>  
> +  // Quote special characters as specified by the CSS grammar.

Could you link to the css grammar tokenization spec in this comment?

@@ +21,5 @@
>    }
>  
> +  // Quote special characters as specified by the CSS grammar.
> +  return quote +
> +    string.replace(/[\\"]/g, function(match, offset, string) {

You don't seem to be using offset and string in the function body, also you could use a fat arrow function here:

quote + string.replace(/[\\"]/g, match => {
  switch (match) {
   ...  
}) + quote;

::: toolkit/devtools/server/main.js
@@ +60,5 @@
>    dumpn.wantLogging = Services.prefs.getBoolPref(LOG_PREF);
>    dumpv.wantVerbose =
>      Services.prefs.getPrefType(VERBOSE_PREF) !== Services.prefs.PREF_INVALID &&
>      Services.prefs.getBoolPref(VERBOSE_PREF);
> +  dumpn.wantLogging = true;

This looks like something you forgot to remove before uploading the patch.
Attachment #8578658 - Flags: review?(pbrosset) → review+
Attachment #8578658 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/59645225466f
Status: ASSIGNED → RESOLVED
Closed: 5 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.