904049 - Inspector doesn't handle quotes in attributes and/or their escaping correctly

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

The newtab page (about:newtab) sets its thumbnails via style.backgroundImage like so:

> thumbnail.style.backgroundImage = "url(" + thumbnailURL + ")";
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/newtab/sites.js#147

If you now open about:newtab, right-click one of the sites and take a look at the style rule that defines the background image, for some reason it's displayed as:

> style="background-image: url("moz-page-thumb://thumbnail?url=http%3A%2F%2Fwww.mozilla.org%2F");"

The double quotes certainly don't look correct. Even worse, if you double click the attribute and then blur the input field, the inner quotes get escaped and the style becomes invalid and breaks the thumbnail:

> style="background-image: url("moz-page-thumb://thumbnail?url=http%3A%2F%2Fwww.mozilla.org%2F");"

Comment 1

[Brian Grinstead [:bgrins]]

Bug 899375 will fix some common inspector attribute issues.  However, it will not fix this case, it will simply break it in a different way. In fact, it will end up splitting up the whole style attribute.  The problem is that given a string like: 

> style="background-image: url("moz-page-thumb://thumbnail?url=http%3A%2F%2Fwww.mozilla.org%2F");"

The DOMParser will see the double quotes and split it into multiple attributes.

> style="background-image: url(" moz-page-thumb:=""

There are a couple of options I can think of here:

1) If an attribute value contains a ", switch the outer quotes on the editor to '.  The DOMParser will handle this markup better.  This doesn't resolve the problem if it contains both a " and a '.
2) If an attribute contains both a " and a ', then the only option I can think of is converting the double quotes to " once we enter the editable field.  This will be handled properly as well, though it would be a little ugly from an editing experience, and if you manually entered a " while editing, it would break again.  There isn't really a way to work around that issue.

I think that 1) and 2) will handle most of the cases here.  If you manually enter name="val"ue" there really isn't anything we can do to help at that point.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Please keep in mind that the quotes were added by the inspector itself. about:newtab sets the URL without any quotes. OTOH you will need the fixes that you mentioned anyway in case someone does this intentionally.

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

We also need to remember that adding an attribute by extending the current input still needs to work:
e.g. |"somevalue" hidden="true"|

Alternatively, we could avoid this issue by changing how it is done.

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #3)
> We also need to remember that adding an attribute by extending the current
> input still needs to work:
> e.g. |"somevalue" hidden="true"|
> 
> Alternatively, we could avoid this issue by changing how it is done.

The 'extending' is very nice behavior that I wouldn't want to remove.  I think that extending will still work even if we modified the wrapper quotes.  So given

> name="va"lue"

We would convert into this once editing starts: 

> name='val"ue'

You could extend the editing adding a new attribute after this:

> name='val"ue2' hidden="true"

Then the final displayed values would be
* Viewing: http://i.imgur.com/so9ABUu.png
* Editing: http://i.imgur.com/L1nuNks.png

Comment 5

[Brian Grinstead [:bgrins]]

Try push with fix: https://tbpl.mozilla.org/?tree=Try&rev=dc75563023bc.  Still waiting on Bug 899375 to land before generating a patch, but here is the diff: https://hg.mozilla.org/try/rev/a364a604c6b2.

Comment 6

[Brian Grinstead [:bgrins]]

Attachment: 904049-inspector-quotes.patch

Double quotes need to be handled specially to prevent DOMParser failing.
 * name="v"a"l"u"e" when editing -> name='v"a"l"u"e"'
 * name="v'a"l'u"e" when editing -> name="v'a"l'u"e"

Comment 7

[Brian Grinstead [:bgrins]]

Attachment: 904049-inspector-quotes-1.patch

Try push: https://tbpl.mozilla.org/?tree=Try&rev=7944c7acb945.

Had to remove the `if this.attrs.hasOwnProperty(aAttr.name)` block in _createAttribute that meant it wouldn't need to recreate the editableField / DOM node for the attribute, because the `initial` property on the editableField needs to be set after every change (in case quotes got added / removed into the attribute value).

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 789773 [details] [diff] [review]
904049-inspector-quotes-1.patch

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

r+ with or without changes.

::: browser/devtools/markupview/markup-view.js
@@ +1231,5 @@
> +    // Double quotes need to be handled specially to prevent DOMParser failing.
> +    // name="v"a"l"u"e" when editing -> name='v"a"l"u"e"'
> +    // name="v'a"l'u"e" when editing -> name="v'a"l'u"e"
> +    let editValueDisplayed = aAttr.value;
> +    let hasDoubleQuote = editValueDisplayed.indexOf('"') > -1;

You could use:
editValueDisplayed.contains('"')

Your choice though.

@@ +1299,3 @@
>  
> +    // Figure out where we should place the attribute.
> +    let before = aBefore || null;

Another optional thing...
You could set aBefore to a default of null in the function declaration:
_createAttribute: function EE_createAttribute(aAttr, aBefore=null)

Just 'cos all the cool kids are doing it.

Comment 9

[Brian Grinstead [:bgrins]]

Attachment: 904049-inspector-quotes-2.patch

Cool kid updates :)

Comment 10

[Brian Grinstead [:bgrins]]

Mike, can you please land this when you get a chance?

Comment 11

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/4687781bea6b

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4687781bea6b