Closed
Bug 1175661
Opened 7 years ago
Closed 7 years ago
'Add rule' fails on bugzilla <body> elements
Categories
(DevTools :: Inspector, defect)
DevTools
Inspector
Tracking
(firefox41 fixed)
RESOLVED
FIXED
Firefox 41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: bgrins, Assigned: gl)
References
Details
Attachments
(1 file)
2.30 KB,
patch
|
pbro
:
review+
|
Details | Diff | Splinter Review |
STR: Select the body element on a bug, like: https://bugzilla.mozilla.org/show_bug.cgi?id=1166959 Right click -> Add Rule Expected: A new rule is created with the class name Actual: There is an error: 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:904] protocol.js:907 It looks like maybe it's caused by some whitespace in the class name: <body onload="" class="bugzilla-mozilla-org skin-Mozilla bz_bug bz_status_ASSIGNED bz_product_Firefox bz_component_Developer_Tools:_Inspector bz_bug_1166959 bz_gravatar yui-skin-sam">
Comment 1•7 years ago
|
||
I think we have try/catches around almost all instances of el.querySelector, el.matches, sheet.insertRule, ... in the actor. Basically, anywhere we evaluate a selector that comes from user input. It looks like we don't have one in addNewRule. It might be all it takes to fix this issue. But of course, we don't really have a good way to report problems to the user.
Assignee | ||
Comment 2•7 years ago
|
||
(In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #1) > I think we have try/catches around almost all instances of el.querySelector, > el.matches, sheet.insertRule, ... in the actor. Basically, anywhere we > evaluate a selector that comes from user input. > It looks like we don't have one in addNewRule. It might be all it takes to > fix this issue. > But of course, we don't really have a good way to report problems to the > user. We originally thought escaping all the selectors will avoid all syntax error. We can add in a try catch and default to using the node tag as the selector.
Comment 3•7 years ago
|
||
I didn't check the code closely enough, the problems seems to come from this part of the AddNewRule method: selector = "." + rawNode.className.split(" ").map(c => CSS.escape(c)).join("."); This assumes that className contains a list of classes separated by exactly 1 space. We should replace this with: selector = "." + [...rawNode.classList].map(c => CSS.escape(c)).join("."); (but we should check if classList works in XUL and SVG documents first too).
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → gabriel.luong
Assignee | ||
Comment 4•7 years ago
|
||
Attachment #8624086 -
Flags: review?(pbrosset)
Assignee | ||
Comment 5•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=00288bf3bc97
Assignee | ||
Comment 6•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c70741ba70c
Comment 7•7 years ago
|
||
Comment on attachment 8624086 [details] [diff] [review] 1175661.patch Review of attachment 8624086 [details] [diff] [review]: ----------------------------------------------------------------- r=me if try is green *and* if this works with SVG nodes and in XUL documents (I have a vague memory of classList being weird in SVG, or maybe it was the className property, or class attribute, I can't remember).
Attachment #8624086 -
Flags: review?(pbrosset) → review+
Assignee | ||
Comment 8•7 years ago
|
||
(In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #7) > Comment on attachment 8624086 [details] [diff] [review] > 1175661.patch > > Review of attachment 8624086 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me if try is green *and* if this works with SVG nodes and in XUL documents > (I have a vague memory of classList being weird in SVG, or maybe it was the > className property, or class attribute, I can't remember). I did test classList with SVG and XUL and it works!
Assignee | ||
Updated•7 years ago
|
Status: NEW → ASSIGNED
Comment 10•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a36b8e56b6f7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Updated•4 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•