1272208 - Add new rule button fails on SVG element

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Nicolas Chevobbe [:nchevobbe]]

STR: 

1. Open `data:text/html;charset=utf-8,%0A%3Chtml%3E%3Chead%3E%3Cmeta charset%3D"utf-8"%3E%0A%0A%3Cstyle%3E%0Asvg %7B%0A  width%3A 200px%3B%0A  height%3A 200px%3B%0A  background-color%3A green%3B%0A%7D%0A%3C%2Fstyle%3E%0A%3C%2Fhead%3E%0A%3Cbody%3E%0A%3Csvg viewBox%3D"0 0 10 10"%3E%0A  %3CclipPath%3E%0A    %3Crect x%3D"0" y%3D"0" width%3D"10" height%3D"5"%3E%3C%2Frect%3E%0A  %3C%2FclipPath%3E%0A  %3Ccircle cx%3D"5" cy%3D"5" r%3D"5" fill%3D"blue" clip-path%3D"url(%23clip)"%3E%3C%2Fcircle%3E%0A%3C%2Fsvg%3E%0A%0A%3C%2Fbody%3E%3Cstyle type%3D"text%2Fcss"%3E%3C%2Fstyle%3E%3C%2Fhtml%3E%0A` in a tab.

2. Right-click on the blue circle, and chose "Inspect"
3. In the rule view, click the "Add new rule" button

Expected result : 
A new `circle` rule is displayed

Actual result :
Nothing happens in the UI.
In the js-console, there is an error message :
`
A promise chain failed to handle a rejection. Did you forget to '.catch', or did you forget to 'return'?
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

Date: Wed May 11 2016 23:57:26 GMT+0200 (CEST)
Full Message: Protocol error (unknownError): An invalid or illegal string was specified
Full Stack: JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: PendingErrors.register :: line 195
JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: this.PromiseWalker.completePromise :: line 718
JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: Handler.prototype.process :: line 973
JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: this.PromiseWalker.walkerLoop :: line 816
`

Comment 1

[Patrick Brosset <:pbro>]

Inspector bug triage (filter on CLIMBING SHOES).

Comment 2

[Patrick Brosset <:pbro>]

This is another case of el.className not working the same when el is an HTML or an SVG element.
We've this problem in the past in other parts of the code.

Basically, what seems to be happening is that when we try to add a new CSS rule for the element, we try to figure out a good selector for this new rule. If the element has an id, we use #id for the selector, otherwise if the element has a class, we use .class for the selector, and finally, we use the tagName.

You can see the code here: https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/styles.js#889-896
Copied here because it's short:

    let selector;
    if (rawNode.id) {
      selector = "#" + CSS.escape(rawNode.id);
    } else if (rawNode.className) {
      selector = "." + [...rawNode.classList].map(c => CSS.escape(c)).join(".");
    } else {
      selector = rawNode.tagName.toLowerCase();
    }

The problem starts when rawNode is an SVG element, which is the case in this bug.
For SVG elements, className isn't a string, it's an object of type SVGAnimatedString.
Therefore, the test 'if (rawNode.className)' isn't correct. It's only ever going to be false when the element is HTML and has an empty className. For SVG elements, whether the element has a class or not, the className property will always be an object.

So what happens here is, even if the element has no class attribute, the test is truthy and we execute:
  selector = "." + [...rawNode.classList].map(c => CSS.escape(c)).join(".");

which basically sets selector to "." because there are no classes in classList.
And "." isn't a valid selector. This explains the "An invalid or illegal string was specified" error message in comment 0.

Now, if you change the test case a bit and add a class to the circle SVG element, then it works fine. So it's really just for SVG elements that have no class attributes.

We need to change the if.
'[...rawNode.classList].length' seems to work fine I think.

This is an easy bug for someone who wants to get started. I believe there is enough information above for anyone with some JS knowledge to fix this bug.
I will set myself as a mentor if that helps.

Make sure you first go through our contribution guide to first be comfortable building/running firefox: https://developer.mozilla.org/en-US/docs/Tools/Contributing

This is a P1 though, so if no-one takes it in the coming days, I'll just fix it myself.

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

We could add a test case in https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/rules/test/browser_rules_add-rule_01.js (or one of its siblings), with a camel cased svg tag (e.g. clipPath) so we make sure to cover fixes from Bug 1270215 too

Comment 4

[Chris Frey]

Attachment: fix selector building for SVG

See attached hg patch with the fix you suggested and a test case for it.

I'm not sure if the test case is ideal, or if it should go in another file?

I also noticed this bug is mentioned in Bug 1270215, but I don't think the changes there address this?

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8754439 [details] [diff] [review]
fix selector building for SVG

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

This looks great thanks! 
Transferring this to a proper review request. I'll spend more time tomorrow or monday looking at the code and test and make sure that everything works as expected, but so far it looks great.

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8754439 [details] [diff] [review]
fix selector building for SVG

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

Code changes look great. The test runs for me locally fine. I've pushed the patch to TRY so all other tests run and we can make sure there are no regressions before landing this change:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8041224f4740

Could you please add a commit message to the patch however? Right now it's just a diff that doesn't seem to have a commit message:
https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Also, the changes done in bug 1270215 will need to be merged in (you need to rebase your patch on top of the latest fx-team, or m-c, depending on which repo you've used).

I'm going to R+ this now. Please do upload a new rebased patch with the commit message when you have a chance and mark it as R+ yourself (also marking this one as obsolete at the same time), and when the tests on TRY are done and green, we'll land this change.

Thanks a lot!

Comment 7

[Chris Frey]

Attachment: commit after updating from mozilla-central

See attached patch; it's committed on top of the latest rev in the HG mozilla-central repo.

(In the future I can work against fx-team if that would make your lives easier. :) )

Comment 8

[Patrick Brosset <:pbro>]

Looks good thanks.
Also, try is green, so let's ask for a check-in.
Please let me know if you're looking for other bugs to fix in devtools! http://firefox-dev.tools might prove useful.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/60435b82caee

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/60435b82caee

Comment 11

[Nazir Ahmed Sabbir [:NaSb]]

Successfully reproduce this bug on Nightly 49.0a1 (2016-05-11) (Build ID: 20160511030221) on Linux,

This Bug's Fix is now verified on Latest Firefox Beta 49.0b3

Build ID: 20160811031722
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0
OS: Linux 4.4.0-2-deepin-amd64

Comment 12

[Md Maruf Rahman]

I have reproduced this bug with Nightly 49.0a1 (2016-05-11) on Windows 7 , 64 Bit!

This bug's fix is verified with latest  Beta!
Build ID    20160811031722
User Agent  Mozilla/5.0 (Windows NT6.1; Win64; x64; rv:49.0) Gecko/20100101 Firefox/49.0

[bugday-20160817]