Closed Bug 904049 Opened 6 years ago Closed 6 years ago

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

Categories

(DevTools :: Inspector, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: ttaubert, Assigned: bgrins)

References

Details

Attachments

(1 file, 2 obsolete files)

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");"
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.
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.
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.
(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
Depends on: 899375
Assignee: nobody → bgrinstead
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.
Attached patch 904049-inspector-quotes.patch (obsolete) — Splinter Review
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"
Attached patch 904049-inspector-quotes-1.patch (obsolete) — Splinter Review
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).
Attachment #789648 - Attachment is obsolete: true
Attachment #789773 - Flags: review?(mratcliffe)
Blocks: 893677
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.
Attachment #789773 - Flags: review?(mratcliffe) → review+
Cool kid updates :)
Attachment #789773 - Attachment is obsolete: true
Attachment #790780 - Flags: review+
Mike, can you please land this when you get a chance?
Flags: needinfo?(mratcliffe)
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/4687781bea6b
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/4687781bea6b
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26
Flags: needinfo?(mratcliffe)
Duplicate of this bug: 705846
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.