Editing an attribute that contains '&' fails

RESOLVED FIXED in Firefox 22

Status

P2
normal
RESOLVED FIXED
6 years ago
4 months ago

People

(Reporter: jruderman, Assigned: miker)

Tracking

({testcase})

Trunk
Firefox 22
testcase
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 711964 [details]
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"/></
neat!

Comment 2

6 years ago
related: bug 835964

Updated

6 years ago
Blocks: 831711
Assignee: nobody → mratcliffe
Priority: -- → P2
Created attachment 718935 [details] [diff] [review]
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, "&amp;"); before adding the attribute value to the dummy.
Attachment #718935 - Flags: review?(jwalker)
OS: Mac OS X → All
Hardware: x86_64 → All
Whiteboard: [has-patch][also-fixes-835964]
(Reporter)

Comment 4

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

Comment 5

6 years ago
About quotes: see bug 705846 and bug 814154 (not saying it should not happen here, just for reference).
(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 &quot;.
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.
Attachment #718935 - Flags: review?(jwalker)
Created attachment 722175 [details] [diff] [review]
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
Attachment #718935 - Attachment is obsolete: true
Attachment #722175 - Flags: review?(jwalker)
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.
paul encounters an error with patch v2 on OSX:
label is undefined at Toolbox.jsm:431
^^^ Sorry, wrong bug
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.
Attachment #722175 - Flags: review?(jwalker)
Whiteboard: [has-patch][also-fixes-835964] → [also-fixes-835964]
Attachment #722175 - Attachment is obsolete: true
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.
Attachment #722175 - Flags: review?(jwalker)
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 &).
Attachment #722175 - Attachment is obsolete: false
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?
Attachment #722175 - Flags: review?(jwalker) → review+
(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.
Created attachment 727621 [details] [diff] [review]
Patch v3

Addressed reviewer's comments.
Attachment #722175 - Attachment is obsolete: true
Whiteboard: [also-fixes-835964] → [land-in-fx-team][also-fixes-835964]
https://hg.mozilla.org/integration/fx-team/rev/2f3b16c3a8c6
Whiteboard: [land-in-fx-team][also-fixes-835964] → [fixed-in-fx-team][also-fixes-835964]
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/2f3b16c3a8c6
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team][also-fixes-835964]
Target Milestone: --- → Firefox 22

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.