Closed
Bug 705846
Opened 13 years ago
Closed 11 years ago
[markup view] smart quoting
Categories
(DevTools :: Inspector, defect)
Tracking
(Not tracked)
RESOLVED
DUPLICATE
of bug 904049
People
(Reporter: groovecoder, Unassigned)
References
()
Details
(Whiteboard: [good first bug][mentor=paul][lang=js])
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
Comment 1•13 years ago
|
||
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•13 years ago
|
||
We could use single quotes for the attributes. Would that fix the problem? Any downside?
Reporter | ||
Comment 3•13 years ago
|
||
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•12 years ago
|
||
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).
Summary: Inspector mis-match from source on attributes → [markup view] smart quoting
Whiteboard: [good first bug][mentor=paul][lang=js]
Comment 5•12 years ago
|
||
Hi Paul, I'm interested in working on this. Are you available for mentoring?
Comment 6•12 years ago
|
||
(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 8•12 years ago
|
||
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?
Attachment #721667 -
Flags: review?(paul)
Comment 9•12 years ago
|
||
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•12 years ago
|
||
(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.
Updated•12 years ago
|
Assignee: nobody → sachinhosmani2
Comment 11•12 years ago
|
||
Attachment #721667 -
Attachment is obsolete: true
Attachment #723065 -
Flags: review?(paul)
Comment 12•12 years ago
|
||
Didn't mark the earlier one as a patch.
Attachment #723065 -
Attachment is obsolete: true
Attachment #723065 -
Flags: review?(paul)
Attachment #723066 -
Flags: review?(paul)
Comment 13•12 years ago
|
||
Comment on attachment 723066 [details] [diff] [review] Addresses the comments by Paul. Sorry, I'm running out of time. Handing the review to Joe.
Attachment #723066 -
Flags: review?(paul) → review?(jwalker)
Comment 14•12 years ago
|
||
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?
Attachment #723066 -
Flags: review?(jwalker)
Comment 15•12 years ago
|
||
(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 17•12 years ago
|
||
(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="'""/> 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('"', """, "g"); } } And then we'd need to convert the " 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 19•12 years ago
|
||
(In reply to Paul Rouget [:paul] from comment #18) > > Will appear as: > > > > value="'"" > > Won't it appear as: > value="'"" > > ? No. I tried it.
Comment 21•11 years ago
|
||
You should probably test with all these entities: " " " ' ' '
Updated•11 years ago
|
Assignee: sachinhosmani2 → nobody
Comment 23•11 years ago
|
||
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.
Updated•11 years ago
|
Flags: needinfo?(paul)
Comment 25•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
Duping this as I'm pretty sure it is now working as expected.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → DUPLICATE
Updated•11 years ago
|
Flags: needinfo?(paul)
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•