705846 - [markup view] smart quoting

Status: RESOLVED DUPLICATE of bug 904049

(DevTools :: Inspector, defect)

Description

[Luke Crouch [:groovecoder]]

http://pastebin.mozilla.org/1388749

Comment 1

[Paul Rouget [:paul]]

View Source:

<source src="https://s3.amazonaws.com/godjango-videos/GoDjango-03-south-migrations.mp4" type='video/mp4; codecs="avc1.42E01E, mp4a.40.2"' />

HTML inspector:

<source type="video/mp4; codecs="avc1.42E01E, mp4a.40.2"" src="https://s3.amazonaws.com/godjango-videos/GoDjango-03-south-migrations.mp4"></source>

Note the type="" in the inspector which makes it look like a malformed codecs parameter.

Comment 2

[Paul Rouget [:paul]]

We could use single quotes for the attributes. Would that fix the problem? Any downside?

Comment 3

[Luke Crouch [:groovecoder]]

Maybe. But what if the source uses single quotes? Then the inspector will show type='video/mp4; codecs='avc1.42E01E, mp4a.40.s''

It will still mis-match with the source.

Is it possible for the inspector to use the exact same marks as the source?

Comment 4

[Paul Rouget [:paul]]

This sill occurs, even with the new Markup View.

> Is it possible for the inspector to use the exact same marks as the source?

No.

We need to be smart when we quote the attribute (check if there's any un-escaped quotes already).

Comment 5

[Srinath N [:srinath]]

Hi Paul,

I'm interested in working on this. Are you available for mentoring?

Comment 6

[Paul Rouget [:paul]]

(In reply to Srinath N [:srinath] from comment #5)
> Hi Paul,
> 
> I'm interested in working on this. Are you available for mentoring?

Yes (sorry for the very late reply).

The quote are visible here:
https://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/markup-view.xhtml

(look at template-attribute)

Template code is used here:
https://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/MarkupView.jsm#80

This is what I would do:
- look at the content of the attribute value
- if there's a `"`, I would wrap the attribute with a `'`
- if there's a `'`, I would wrap the attribute with a `"`
- if there's both... well, I don't know. Could that happen?

Comment 7

[Sachin Hosmani [:sachin]]

Attachment: Uses single quotes when the attribute value contains double quote(s).

Comment 8

[Paul Rouget [:paul]]

Comment on attachment 721667 [details] [diff] [review]
Uses single quotes when the attribute value contains double quote(s).

Looks good. Can you add a test (see markupview/test) and address the comments below.

>diff -r 0fbccb582d43 browser/devtools/markupview/MarkupView.jsm
>--- a/browser/devtools/markupview/MarkupView.jsm	Fri Mar 01 17:31:03 2013 +0530
>+++ b/browser/devtools/markupview/MarkupView.jsm	Wed Mar 06 18:38:37 2013 +0530
>@@ -77,16 +77,27 @@ this.MarkupView = function MarkupView(aI
> MarkupView.prototype = {
>   _selectedContainer: null,
> 
>   template: function MT_template(aName, aDest, aOptions={stack: "markup-view.xhtml"})
>   {
>     let node = this.doc.getElementById("template-" + aName).cloneNode(true);
>     node.removeAttribute("id");
>     template(node, aDest, aOptions);
>+    if(aName === "attribute") {
>+      let val = aDest.attrValue;
>+      if(/"/.test(val) === true) {

if (aDest.attrValue.contains('"')

>+        /*
>+          The attribute contains double quote(s).
>+          Must use single quotes around attribute value
>+        */

Use double slashes for such comments.

Maybe you want to have 2 templates:
template-attribute and template-attribute-with-quote,
and then move the quote check earlier in the process (where template() is called).

What do you think?

Comment 9

[Sachin Hosmani [:sachin]]

Sure, I'll address all those issues.
But, I'm not clear about what a test should exactly look like.
Will an html file containing nodes with attributes that need smart quoting, suffice?

Comment 10

[Paul Rouget [:paul]]

(In reply to Sachin Hosmani from comment #9)
> But, I'm not clear about what a test should exactly look like.
> Will an html file containing nodes with attributes that need smart quoting,
> suffice?

Don't worry about the test, I'll take care of it.

Comment 11

[Sachin Hosmani [:sachin]]

Attachment: Addresses the comments by Paul.

Comment 12

[Sachin Hosmani [:sachin]]

Attachment: Addresses the comments by Paul.

Didn't mark the earlier one as a patch.

Comment 13

[Paul Rouget [:paul]]

Comment on attachment 723066 [details] [diff] [review]
Addresses the comments by Paul.

Sorry, I'm running out of time. Handing the review to Joe.

Comment 14

[Joe Walker [:jwalker]]

Comment on attachment 723066 [details] [diff] [review]
Addresses the comments by Paul.

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

::: browser/devtools/markupview/MarkupView.jsm
@@ +1091,5 @@
>        // Create the template editor, which will save some variables here.
>        let data = {
>          attrName: aAttr.name,
>        };
> +      if(aAttr.value.contains('"') === true) {

So I think this will fail when people have used ' and quoted their ".

    blah='foo\"'

So perhaps we should use a regexp with [^\]"  ?
What do you think?

Comment 15

[Paul Rouget [:paul]]

(In reply to Joe Walker [:jwalker] from comment #14)
> So I think this will fail when people have used ' and quoted their ".
> 
>     blah='foo\"'

Not sure to understand why.

Comment 16

[Paul Rouget [:paul]]

(afaik, we can't escape quotes in HTML)

Comment 17

[Joe Walker [:jwalker]]

(In reply to Paul Rouget [:paul] from comment #15)
> (In reply to Joe Walker [:jwalker] from comment #14)
> > So I think this will fail when people have used ' and quoted their ".
> > 
> >     blah='foo\"'
> 
> Not sure to understand why.
> (afaik, we can't escape quotes in HTML)

Sorry, I got the wrong quoting syntax, but the point is valid:

    <input id=foo value="'&quot;"/>

Will appear as:

    value="'""

Perhaps we need to say:

    if (aAttr.value.contains('"') === true) {
      // The attribute contains double quote(s).
      // Must use single quotes around attribute value
      this.template("attribute-with-quote", data);

      if (aAttr.value.contains("'") === true) {
        aAttr.value = aAttr.value.replace('"', "&quot;", "g");
      }
    }

And then we'd need to convert the &quot; to " on entry, but it's probably a bug that we're not doing this already.

I wouldn't be sad if we did this in a follow-up bug.

Comment 18

[Paul Rouget [:paul]]

> Will appear as:
> 
>     value="'""

Won't it appear as:
      value="'""

?

Comment 19

[Joe Walker [:jwalker]]

(In reply to Paul Rouget [:paul] from comment #18)
> > Will appear as:
> > 
> >     value="'""
> 
> Won't it appear as:
>       value="'""
> 
> ?

No. I tried it.

Comment 20

[Sachin Hosmani [:sachin]]

So, is the " bug to be fixed here itself?

Comment 21

[Hallvord R. M. Steen [:hallvors]]

You should probably test with all these entities:
" " " 
' ' '

Comment 22

[Sachin Hosmani [:sachin]]

I'm sorry, I won't be able to work on this.

Comment 23

[Alexey Novak]

I see that this bug is very old (last changes were made 2013-03-27).
Is it still an issue? If yes then I could take care of it.

Comment 24

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

Paul? You have a volunteer!

Comment 25

[Alexey Novak]

I was reading the bug. And let me know if I understand it correctly:

The problem is user defined attributes which might contain " or ' or "
They would looks "odd" in the HTML inspector and might confuse the user.

So if a user has some HTML element with type attribute:
video/mp4; codecs="avc1.42E01E, mp4a.40.2" OR  video/mp4; codecs="avc1.42E01E, mp4a.40.2"

HTML inspector will use single quotes to escape the attribute:
<source type='video/mp4; codecs="avc1.42E01E, mp4a.40.2"' />


But then my few questions are:
- What would happen if an attribute has both single quotes and double quotes?
- Would it be confusing for the user to see some of the attributes in single quotes, and some of them in double quotes? (it is still better than what happens right now)

Also what would be an appropriate file to add tests related to this fix?

Comment 26

[Brian Grinstead [:bgrins]]

I am guessing this was resolved by Bug 904049, which introduced DOMParser for parsing the input strings.

> if there's both... well, I don't know. Could that happen?

You can see here: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/markup-view.js#1814.  If it has both single and double quotes its wrapping in double quotes and replacing with ".

Comment 27

[Brian Grinstead [:bgrins]]

Duping this as I'm pretty sure it is now working as expected.