Adding a new rule for a node whose id contains invalid character throws

RESOLVED FIXED in Firefox 41



Developer Tools: Inspector
4 years ago
3 years ago


(Reporter: Optimizer, Assigned: gl)


Firefox 41

Firefox Tracking Flags

(firefox41 fixed)



(1 attachment, 1 obsolete attachment)



4 years ago
with this exception :

DOMException [SyntaxError: "An invalid or illegal string was specified"
code: 12
nsresult: 0x8053000c
location: resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/styles.js:590]

And then nothing happens on the UI.

I think if the id is invalid, we should go ahead and use class next, or maybe use the unqiue selector


4 years ago
Assignee: nobody → gabriel.luong

Comment 1

4 years ago
Any examples on how to reproduce this?
Flags: needinfo?(scrapmachines)

Comment 2

4 years ago
One easy way: Go to facebook, open inspect, type # in search box and you will see some id with ":" in them and likewise. Select those nodes (no idea how, manual search only) and then add rule.

Or else, have a dummy web page with a node with an invalid id like "asd@asd:asd"
Flags: needinfo?(scrapmachines)


3 years ago
OS: Windows 7 → All
Hardware: x86_64 → All


3 years ago

Comment 4

3 years ago
We introduce a try catch similar to what we did in modifySelector because there is a chance for a failure in insertRule when you insert a rule with syntax error (invalid selector name). We try to handle such error by finding an unique css name.
Comment on attachment 8613282 [details] [diff] [review]

Review of attachment 8613282 [details] [diff] [review]:

::: toolkit/devtools/server/actors/styles.js
@@ +883,5 @@
> +    let index;
> +    try {
> +      index = sheet.insertRule(selector + " {}", cssRules.length);
> +    } catch(e) {
> +      index = sheet.insertRule(CssLogic.findCssSelector(rawNode) + " {}",

Using findCssSelector here is not such a bad idea after all, we could use this to replace the whole if/elseif/else above that checks for ids and then classes and then tagnames, because I believe that's what CssLogic.findCssSelector does, and it does it better probably.

My problem with this patch though is that I think you should either fix the if/elseif/else block so it never throws, or using findCssSelector instead of it, but not use both with a try catch.

Option 1, fix the existing code:

if ( {
  selector = "#" + CSS.escape(;
} else if (rawNode.className) {
  selector = "." + rawNode.className.split(" ").map(c => CSS.escape(c)).join(".");
} else {
  selector = rawNode.tagName.toLowerCase();

Or something like the above anyway, indeed the problem is simply that the id and classes aren't being escaped properly.

Option 2, use findCssSelector instead of the existing code:

let selector = CssLogic.findCssSelector(rawNode);

and remove the whole if/elseif/else and no need for a try/catch.

Up to you what you choose, I like findCssSelector, but it may have side effects I didn't see coming. Using CSS.escape is such a small and quick fix that I'd R+ a patch with just this + the test.
Attachment #8613282 - Flags: review?(pbrosset)

Comment 6

3 years ago
Created attachment 8613643 [details] [diff] [review]
1040199.patch [1.0]

findCssSelector does have some unexpected side effects since it tries to find an unique CSS selector. I stuck with option 1.
Attachment #8613282 - Attachment is obsolete: true
Attachment #8613643 - Flags: review?(pbrosset)
Comment on attachment 8613643 [details] [diff] [review]
1040199.patch [1.0]

Review of attachment 8613643 [details] [diff] [review]:

Looks good to me now.
Attachment #8613643 - Flags: review?(pbrosset) → review+


3 years ago
Keywords: checkin-needed
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
You need to log in before you can comment on or make changes to this bug.