839640 - Editing an attribute that contains '&' fails

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase

1. Load the testcase.
2. Using the Inspector, try to change the <iframe src> attribute from http to https.

Result: it changes back, and the error console says

Timestamp: 2/8/13 1:35:33 PM
Error: not well-formed
Source Code:
<div xmlns="http://www.w3.org/1999/xhtml"><div src="http://vid.ly/7y2d9p?content=video&format=webm"/></

Comment 1

[Rob Campbell [:rc] (:robcee)]

neat!

Comment 2

[Paul Rouget [:paul]]

related: bug 835964

Comment 3

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

Attachment: Patch

This fix turned out to be extremely simple. Because we create a dummy node and get the values from that node all that we needed was aValue = aValue.replace(/&/g, "&"); before adding the attribute value to the dummy.

Comment 4

[Jesse Ruderman]

Please also test that it works correctly for attribute values containing ' and "

Comment 5

[Paul Rouget [:paul]]

About quotes: see bug 705846 and bug 814154 (not saying it should not happen here, just for reference).

Comment 6

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

(In reply to Jesse Ruderman from comment #4)
> Please also test that it works correctly for attribute values containing '
> and "

We allow adding multiple attributes e.g.
src="some.path"
double-click "some.path" and change it to src="some.path" style="color:red"

So quotes are valid as long as there is an even number and they are in the correct positions. We already test this.

I will add a test for ' though and should probably test ".

Comment 7

[Joe Walker [:jwalker]]

Comment on attachment 718935 [details] [diff] [review]
Patch

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

It looks to me like there is more work needed with ' and ", but that we're going well so far. Perhaps we should also include tests for <, >, / and ' ', especially in attributes without quotes.

Comment 8

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

Attachment: Patch v2

(In reply to Jesse Ruderman from comment #4)
> Please also test that it works correctly for attribute values containing '
> and "

Fixed

(In reply to Joe Walker [:jwalker] from comment #7)
> Comment on attachment 718935 [details] [diff] [review]
> Patch
> 
> Review of attachment 718935 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It looks to me like there is more work needed with ' and ", but that we're
> going well so far. Perhaps we should also include tests for <, >, / and ' ',
> especially in attributes without quotes.

We now parse the attribute escaping attribute values as appropriate. The cases you mention above all work correctly now although the attributes still need to be quoted. Personally I think it would be better if double-clicking a name would open an editor just for the name and double-clicking a value would open an editor just for the value.

Just sayin

Comment 9

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

Now that the entity issue is fixed we have a new error (bug 848731 - try running the original test case) but this bug does fix the entity issue.

The new issue is probably only present when changing the src of iframes.

Comment 10

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

paul encounters an error with patch v2 on OSX:
label is undefined at Toolbox.jsm:431

Comment 11

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

^^^ Sorry, wrong bug

Comment 12

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

Comment on attachment 722175 [details] [diff] [review]
Patch v2

We have decided to use one editor for a name and one for a value if possible. I will check how much work this will be and if it is reasonably simple we can do it. We gain a lot more reliability this way.

Comment 13

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

Changing this would mean more work than we have time for at the moment. For now we should land this and the bug for editing just the name or value has been moved over to bug 852508.

Comment 14

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

Comment on attachment 722175 [details] [diff] [review]
Patch v2

In light of Paul's comments in bug 852508 and the fact that it is a large job to allow editing of just the attribute name or value we should land this patch soon.

Without it users cannot edit src urls containing multiple get params (or any attribute containing &).

Comment 15

[Joe Walker [:jwalker]]

Comment on attachment 722175 [details] [diff] [review]
Patch v2

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

::: browser/devtools/markupview/MarkupView.jsm
@@ +1423,5 @@
> + *
> + * @param  {String} attr
> + *         The attributes for which the values are to be escaped.
> + * @return {Array}
> + *         An array of attribute names and there escaped values.

"their escaped values".

I *hate* myself. *hate* *hate* *hate*.

@@ +1508,5 @@
> +function simpleEscape(value) {
> +  return value.replace(/</g, "&lt;")
> +              .replace(/>/g, "&gt;")
> +              .replace(/"/g, "&quot;")
> +              .replace(/'/g, "&#39;");

&apos; ?

Also - I can't help thinking that there should be a way to get the browser to do this. I can't think of it now though, so unless you have a flash of genius?

Comment 16

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

(In reply to Joe Walker [:jwalker] from comment #15)
> Comment on attachment 722175 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 722175 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/markupview/MarkupView.jsm
> @@ +1423,5 @@
> > + *
> > + * @param  {String} attr
> > + *         The attributes for which the values are to be escaped.
> > + * @return {Array}
> > + *         An array of attribute names and there escaped values.
> 
> "their escaped values".
> 

Did I really do that? Wow, what has happened to my grammar, in my defense I was very tired ... honest.

> I *hate* myself. *hate* *hate* *hate*.
> 

Less of the self loathing please ;o)

> @@ +1508,5 @@
> > +function simpleEscape(value) {
> > +  return value.replace(/</g, "&lt;")
> > +              .replace(/>/g, "&gt;")
> > +              .replace(/"/g, "&quot;")
> > +              .replace(/'/g, "&#39;");
> 
> &apos; ?
> 

Will fix.

> Also - I can't help thinking that there should be a way to get the browser
> to do this. I can't think of it now though, so unless you have a flash of
> genius?

I have a way to let the browser do this but it would escape everything ... which may not be what the user wants. These are the only entities that actually cause issues.

Comment 17

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

Attachment: Patch v3

Addressed reviewer's comments.

Comment 18

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/2f3b16c3a8c6

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2f3b16c3a8c6