Closed Bug 205872 Opened 22 years ago Closed 18 years ago

domNode viewer should use one dialog for setting new/editing attributes

Categories

(Other Applications :: DOM Inspector, defect)

Product:
Other Applications
Component:
DOM Inspector
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla1.9alpha

People

(Reporter: WeirdAl, Assigned: sdwilsh)

References

Details

Attachments

(5 files, 11 obsolete files)

oninput Test Case
673 bytes, text/plain
Details
v2.8 - Final
25.05 KB, patch
sdwilsh
: review+
sdwilsh
: superreview+
Details | Diff | Splinter Review
Fixed typo
1.15 KB, patch
Details | Diff | Splinter Review
Fix for disappearing attribute bug v2
1.37 KB, patch
timeless
: review+
neil
: superreview+
Details | Diff | Splinter Review
l10n fix v1.0
1.63 KB, patch
Pike
: review+
Details | Diff | Splinter Review
Spun off of bug 193724. domNode.js uses a promptFor() function for setting a new attribute; it calls two seperate prompts, and only allows for null-namespaced attributes. This bug is to create a single dialog box to handle attributes in general, including those with non-null namespaces.
Product: Core → Other Applications
Edit has the same issue, and should probably be part of this bug.
Reassigning to comrade, who says he wants to work on this.
Assignee: ajvincent → comrade693
Status: NEW → ASSIGNED
So, I would imagine we need attribute name, attribute value, and namespaceURI at a minimum, and probably a checkbox for a null namespace that would disable the namespaceURI field. Would we be wanting anything else in this dialog?
I'd think you'd want the namespace to be a drop-down with null namespace by default, and also including any namespaces available in the chosen element. Assuming I understand namespaces and attributes correctly, I'm a little concerned that people adding HTML attributes will add them to the HTML namespace rather than the null namespace.
I can provide code for telling when we have a HTML doc (and not a XHTML doc). We are supposed to disable namespaces for non-XML documents.
I recommend we default to the null namespace, anyway - that's where 99% of attributes live.
Attached patch v1 (obsolete) — — Splinter Review
This patch implements the dialog for editing an attribute. Works with null namespaces and non-null namespaces. Known issues: Parts of the diff are unrelated to this but are part of Bug 341816 that has not been committed. Does not yet work for inserting new attributes. I'm looking for feedback on what I have so far.
Attached patch v2 (obsolete) — — Splinter Review
Adds support for inserting a new attribute. Known issues (including some forgotten from last patch): XUL file is not fully localized. Changing namespaces from not null to null doesn't work. I'd like someone else to take a look at the code and make sure everything looks ok before I fix those. I'm also unsure on what to do with the license block: * The Initial Developer of the Original Code is Do I put my name, or what do I put there? Feedback is greatly appreciated.
Attachment #228549 - Attachment is obsolete: true
Comment on attachment 228604 [details] [diff] [review] v2 ><HTML><HEAD/><BODY ap:processed="true"><PRE>Index: jar.mn I'd like to know how this happened. :) >+ * The Original Code is mozilla.org code. I think you can be a little more specific: say it's for DOM Inspector. >+ * The Initial Developer of the Original Code is Generally speaking, if this is done for a company, the company's name goes here, and your name goes in as a contributor. If you as an individual are contributing this, your name goes here. The standard format is: Your Name <your.email@address.foo> >Index: resources/content/viewers/domNode/domNodeDialog.xul >=================================================================== >RCS file: resources/content/viewers/domNode/domNodeDialog.xul >diff -N resources/content/viewers/domNode/domNodeDialog.xul >--- /dev/null 1 Jan 1970 00:00:00 -0000 >+++ resources/content/viewers/domNode/domNodeDialog.xul 9 Jul 2006 16:44:30 -0000 >@@ -0,0 +1,39 @@ >+<?xml version="1.0"?> >+<!DOCTYPE page [ >+ <!ENTITY % dtd1 SYSTEM "chrome://inspector/locale/inspector.dtd"> %dtd1; >+ <!ENTITY % dtd2 SYSTEM "chrome://inspector/locale/viewers/domNode.dtd"> %dtd2; >+]> I think we should see a license here too, even if it's removed in preprocessing. >+ <script type="application/x-javascript" src="chrome://inspector/content/viewers/domNode/domNodeDialog.js"/> application/javascript, now. Also, to save on line length you may consider putting the src attribute on a new line. >+ <row> >+ <label value="&namespaceURI.label;" control="namespaceURI"/> >+ <hbox> >+ <textbox id="tx_namespace"/> >+ <checkbox id="ck_namespace" label="Null"/> >+ </hbox> >+ </row> Hmm, I wonder about this. I agree with Jason that this should be an editable menulist with several options available by default (XMLNS, XML, XLink come to mind). >Index: resources/content/viewers/domNode/domNode.js >+ window.openDialog("chrome://inspector/content/viewers/domNode/domNodeDialog.xul", >+ title, "chrome,modal,centerscreen", viewer, out); >+ >+ if (out.name && out.value) { I think, onaccept, you could set another property of out to indicate the dialog was accepted. >+ if (this.previousNamespaceURI == this.newNamespaceURI) { >+ this.subject.setAttribute(attr.nodeName, out.value); >+ } else { >+ this.subject.removeAttribute(attr.nodeName); >+ this.subject.setAttributeNS(out.namespaceURI, >+ attr.nodeName, >+ out.value); >+ } I think this is wrong. setAttribute is DOM Level 1, and may generate unpredictable results. Always use setAttributeNS.
(In reply to comment #9) > (From update of attachment 228604 [details] [diff] [review] [edit]) > ><HTML><HEAD/><BODY ap:processed="true"><PRE>Index: jar.mn > > I'd like to know how this happened. :) Err...me too. Where do you see that? > I think we should see a license here too, even if it's removed in > preprocessing. Are you referring to Bug 338097? As for the other stuff, I'll be submitting a patch later tonight/tomorrow morning to address those.
> > ><HTML><HEAD/><BODY ap:processed="true"><PRE>Index: jar.mn > > > > I'd like to know how this happened. :) > > Err...me too. Where do you see that? By clicking on "Edit", and then "Edit attachment as comment."
Attached patch v2.1 (obsolete) — — Splinter Review
As per Comment 9. I was using an editable menulist, but it just didn't look right, and I wasn't happy with it. I ended up have a custom option in the menulist and have it activate the text box below it. I was also looking into using an autocomplete textbox, but realized that that was a lot of code to do if it ended up that nobody else liked that idea. According to neil/mcsmurf on irc we can do that and it won't be an issue with toolkit/xpfe with the DOM Inspector being used in different projects. I still haven't localized this, but pending approval of the UI I can do this and then get this patch reviewed.
Attachment #228604 - Attachment is obsolete: true
Comment on attachment 228679 [details] [diff] [review] v2.1 ><HTML><HEAD/><BODY ap:processed="true"><PRE>Index: jar.mn :) A few observations (not a formal review) : >+ <menuitem label="HTML" value="http://www.w3.org/TR/html4"/> This makes absolutely no sense to me for two different reasons. (1) HTML 4 documents have no XML namespaces. (2) There is no such thing as a HTML namespace. Reference comment 5. >+ this.subject.removeAttributeNS(this.previousNamespaceURI, >+ attr.nodeName); >+ this.subject.setAttributeNS(out.namespaceURI, >+ attr.nodeName, >+ out.value); Why remove and then set? Why not just set? (I don't mind, I just want to know the reasoning.)
Hm, I think the HTML tags at the beginning of the patch are actually a Bugzilla bug, so you can ignore those.
(In reply to comment #13) > ><HTML><HEAD/><BODY ap:processed="true"><PRE>Index: jar.mn I honestly don't get that. I did what you said and it didn't show up :) > This makes absolutely no sense to me for two different reasons. (1) HTML 4 > documents have no XML namespaces. (2) There is no such thing as a HTML > namespace. Reference comment 5. Yeah...not sure what I was thinking there. As for some of those namespaces, I couldn't find namespace urls for them in my five minutes of looking, so if someone would care to point me in the right direction, I'd be happy to add those in. > Why remove and then set? Why not just set? (I don't mind, I just want to > know the reasoning.) I removed the old one if the namespace changed because otherwise you would end up with too attributes of the same name, but in different namespaces, right? (I didn't test it, but figured that is what would happen so I should probably test it). When I do put this in for a formal review, who should I have do it? I'm still new with all this, so I don't know where to look to find out.
Attached patch v2.2 (obsolete) — — Splinter Review
This provides the changes for localization. The only thing missing from this is the code mentioned by Alex in Comment 5 (currently the function just returns true to show the namespace box).
Attachment #228679 - Attachment is obsolete: true
Attachment #229500 - Flags: review?
Attachment #229500 - Flags: review? → review?(timeless)
Attached patch v2.3 (obsolete) — — Splinter Review
Helps when I grab the right dtd, right? I also added the code in for the namespace function as per conversation on irc with Alex.
Attachment #229500 - Attachment is obsolete: true
Attachment #229515 - Flags: review?(timeless)
Attachment #229500 - Flags: review?(timeless)
OS: Windows 98 → All
Hardware: PC → All
Summary: domNode viewer should use one dialog for setting new attributes → domNode viewer should use one dialog for setting new/editing attributes
Comment on attachment 229515 [details] [diff] [review] v2.3 >+DomNodeDialog.prototype = >+ initialize: function initialize() >+ nodeName.value = this.mData.name ? this.mData.name : ""; >+ nodeName.disabled = this.mData.name == null ? false : true; >+ nodeValue.value = this.mData.value ? this.mData.value : ""; Personally, i'd do: nodeName.value = this.mData.name || ""; (similarly for nodeValue). nodeName.disabled = this.mData.name != null; I'd include braces for multiline blocks: >+ for (var i = 0; i < menuitems.length; ++i) >+ if (menuitems[i].value == namespace.value) { >+ found = true; >+ menulist.selectedIndex = i; >+ } I'd probably write <functionName> instead of "Function". At least, I think that's the way we've been doing it: >+ * Function toggles the namespace textbox based on the null namespace checkbox Probably add a period too... this function name sounds strange: >+ enableNamespace: function enableNamespace(aDoc) allowNamespaces, enableNamespaceEditing, enableNamespaces, allowNamespaceEditing ... just a thought. >+ <menuitem label="Default" id="mi_namespace"/> >+ <menuitem label="XHTML" value="http://www.w3.org/1999/xhtml"/> >+ <menuitem label="XUL" value="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"/> >+ <menuitem label="Custom" value="custom"/> These should probably be entities. Even if you think that no one will ever want to localize them, in which case the dtd file should have localization notes suggesting that it not be translated.
Attachment #229515 - Flags: superreview?(neil)
Attachment #229515 - Flags: review?(timeless)
Attachment #229515 - Flags: review-
Attached patch v2.4 (obsolete) — — Splinter Review
Fixed remarks from Comment #18 > These should probably be entities. Even if you think that no one will ever want > to localize them, in which case the dtd file should have localization notes > suggesting that it not be translated. I decided that people might actually want to localize most of those so I added them.
Attachment #229515 - Attachment is obsolete: true
Attachment #230661 - Flags: superreview?(neil)
Attachment #230661 - Flags: review?(timeless)
Attachment #230661 - Flags: approval-l10n?
Attachment #229515 - Flags: superreview?(neil)
Comment on attachment 230661 [details] [diff] [review] v2.4 >+ var nodeName = document.getElementById("tx_nodeName"); >+ var nodeValue = document.getElementById("tx_nodeValue"); >+ var namespace = document.getElementById("tx_namespace"); >+ var menulist = document.getElementById("ml_namespace"); You might want to cache these as properties of the dialog variable. >+ if (this.mDoc) >+ defaultNS.value = this.mDoc.documentElement.namespaceURI; >+ if (defaultNS.value == '') >+ defaultNS.parentNode.removeChild(defaultNS); I'm not sure what the usefulness of this is. >+ menulist.addEventListener("ValueChange", this.updateNamespace, false); You should use the command event, rather than the ValueChange event. In fact, you should use an inline oncommand event attribute to call dialog.updateNamespace(); as that's the easist way to get the correct "this". >+ // Selecting proper index >+ menulist.selectedIndex = -1; >+ if (namespace.value != null) { >+ var found = false; >+ for (var i = 0; i < menuitems.length; ++i) >+ if (menuitems[i].value == namespace.value) { >+ found = true; >+ menulist.selectedIndex = i; >+ } >+ if (!found) { >+ menulist.selectedIndex = menuitems.length - 1; >+ namespace.value = this.mData.namespaceURI; >+ } >+ } else { >+ menulist.selectedIndex = 0; >+ } menulist.value = namespace.value; if (!menulist.selectedItem) menulist.selectedIndex = 0; >+ document.getElementById("ml_namespace") >+ .removeEventListener("ValueChange", this.updateNamespace, false); Won't need this with an inline event attribute. >+ namespace.disabled = !(menulist.selectedItem.value == "custom"); menulist.value != "custom" >+ namespace.value = namespace.disabled ? menulist.selectedItem.value : ''; I don't see the point of setting this. >+ enableNamespaces: function enableNamespaces(aDoc) >+ { >+ return this.mDoc.contentType != "text/html"; >+ } Not sure this is true. Try asking a content peer (jst/sicking/bz)? >+ if (this.previousNamespaceURI == this.newNamespaceURI) { I don't think changing an attribute's NS makes sense. After all, you don't provide the opportunity to change its name, do you?
Comment on attachment 230661 [details] [diff] [review] v2.4 >+ if (this.mDoc) >+ defaultNS.value = this.mDoc.documentElement.namespaceURI; >+ if (defaultNS.value == '') >+ defaultNS.parentNode.removeChild(defaultNS); I'm not convinced of the utility of this, but you should at least check the other predefined namespaces too, speaking of which, you should support some of the other namespaces listed in nsNameSpaceManager.cpp, and I'm not convinced that the namespace names are localisable. >+ namespace.value = namespace.disabled ? menulist.selectedItem.value : ''; OK, I guess this helps you see which namespace you selected. >+ minwidth="200"> I don't like this. You should style a width in em if you really need it. >+ <menulist id="ml_namespace"> ... >+ <textbox id="tx_namespace"/> I'm wondering whether they should both be on the same line. You should also disable the OK button if the attribute name is empty.
Attachment #230661 - Flags: superreview?(neil) → superreview+
Attached patch v2.5 (obsolete) — — Splinter Review
Fixed a minor bug that let editing happen when no element was selected. Fixed all of what Neil commented on, except as explained below. Neil: > I'm wondering whether they should both be on the same line. It looked funny on the same line. I tried it before. > I don't think changing an attribute's NS makes sense. After all, you don't > provide the opportunity to change its name, do you? After talking to timeless and bz on irc, we are going to leave it in. > Not sure this is true. Try asking a content peer It does in fact work, but I couldn't get any response from any of them on irc on it being "right" > menulist.value = namespace.value; > if (!menulist.selectedItem) > menulist.selectedIndex = 0; For whatever reason, that doesn't seem to work right. It's annoying that I have to use that massive loop. Comment added. I know you are going to wonder as to why I use an "input" event listener instead of oninput. oninput doesn't fire when something like backspace is fired, but the input event does. They could enter a character and then press backspace and with oninput the OK button would be enabled, but with the input event listener it goes back to being disabled. I'm asking for your review again because a decent amount changed.
Attachment #230661 - Attachment is obsolete: true
Attachment #230929 - Flags: superreview?(neil)
Attachment #230929 - Flags: review?(timeless)
Attachment #230661 - Flags: review?(timeless)
Attachment #230661 - Flags: approval-l10n?
(In reply to comment #22) >> menulist.value = namespace.value; >> if (!menulist.selectedItem) >> menulist.selectedIndex = 0; >For whatever reason, that doesn't seem to work right. It's annoying that I >have to use that massive loop. Sorry, I hadn't actually tested but I'm sure I used something like that before. Let me look it up and find out what it really should be :-) >oninput doesn't fire when something like backspace is >fired, but the input event does. That should totally not happen; oninput="foo" should fire whenever the input event listener does. Can you write a minimal test case?
Comment on attachment 230929 [details] [diff] [review] v2.5 >+ this.namespace.value = this.mData.namespaceURI; You've got a problem here because your subsequent call to toggleNamespace() overwrites this, but in fact I think it would be better to make selecting a custom namespace reset the textfield to the original namespace anyway. >+ // Selecting proper index - UI wise the simple way doesn't work right, >+ // so we do this the long and dumb way :( >+ this.menulist.selectedIndex = -1; >+ if (this.namespace.value != "") { >+ var found = false; >+ for (var i = 0; i < menuitems.length; ++i) >+ if (menuitems[i].value == this.namespace.value) { >+ found = true; >+ this.menulist.selectedIndex = i; >+ } >+ if (!found) { >+ this.menulist.selectedIndex = menuitems.length - 1; >+ this.namespace.value = this.mData.namespaceURI; >+ } >+ } else { >+ this.menulist.selectedIndex = 1; >+ } OK, so I found an even better solution for this: 1. Set value="" on the (none) menuitem. This means that setting .value will make it find that menuitem when the attribute has no namespace. 2. Either set selected="true" on the (custom) menuitem or set value="custom" on the menulist. This will make it default when there is no matching value. 3. Just set this.menulist.value = this.mData.namespaceURI; here.
Comment on attachment 230929 [details] [diff] [review] v2.5 >+ var accept = document.documentElement.getButton("accept"); Unused? >+ this.nodeName.addEventListener("input", this.toggleAccept, false); oninput seemd to work fine for me. >+ * toggleNamespace toggles the namespace textbox based on the null namespace >+ * checkbox. Checkbox? >+ dialog.namespace.value = dialog.namespace.disabled ? >+ dialog.menulist.selectedItem.value : ''; I toyed with the idea of setting the custom namespace's value to the original namespace. Advantages 1) always works to set the initial selection correctly, you don't even have to set selected="true" 2) dialog.namespace.value = dialog.menulist.value; always works here. Disadvantage: harder to tell when "Custom" item is selected. Maybe set an id on the custom item, and use dialog.namespace.disabled = dialog.menulist.selectedItem.id != "custom"; > case "cmdEditEdit": >- return this.mAttrTree.view.selection.count == 1; >+ return this.selectedAttribute != null && >+ this.mAttrTree.view.selection.count == 1; How can this.selectedAttribute be null if the count is 1? >+ this.subject.removeAttributeNS(this.previousNamespaceURI, >+ attr.nodeName); >+ this.subject.setAttributeNS(out.namespaceURI, >+ attr.nodeName, >+ out.value); By my reading of some documentation, which I freely admit was not the correct documentation, removeAttributeNS takes a localName parameter while setAttributeNS takes a qualifiedName parameter. The upshot is that if I call setAttributeNS(XLINK_NS, "xlink:href", "http://www.mozilla.org/") then I don't know whether I need to remove it using removeAttributeNS(XLINK_NS, "xlink:href") or removeAttributeNS(XLINK_NS, "href")...
Attachment #230929 - Flags: review?(timeless) → review-
Attached file oninput Test Case —
Neil, Type in any amount of characters, then delete them all. At least for me on Windows (1.5.0.5 and Trunk), it does not disable the OK button, but leaves it enabled. The input event with an event handler did for me (I can attach a test case for that as well to demonstrate if need be).
Attached patch v2.6 (obsolete) — — Splinter Review
(In reply to comment #24) > OK, so I found an even better solution for this: > 1. Set value="" on the (none) menuitem. This means that setting .value will > make it find that menuitem when the attribute has no namespace. > 2. Either set selected="true" on the (custom) menuitem or set value="custom" > on the menulist. This will make it default when there is no matching value. > 3. Just set this.menulist.value = this.mData.namespaceURI; here. That looks so much more elegant. :) (In reply to comment #25) > >+ var accept = document.documentElement.getButton("accept"); > Unused? used on the very next line :) > >+ this.nodeName.addEventListener("input", this.toggleAccept, false); > oninput seemd to work fine for me. See testcase (Attachment 230987 [details]). > >+ * toggleNamespace toggles the namespace textbox based on the null namespace > >+ * checkbox. > Checkbox? whoops...that was old code. > >+ dialog.namespace.value = dialog.namespace.disabled ? > >+ dialog.menulist.selectedItem.value : ''; > I toyed with the idea of setting the custom namespace's value to the original > namespace. Advantages 1) always works to set the initial selection correctly, > you don't even have to set selected="true" 2) dialog.namespace.value = > dialog.menulist.value; always works here. Disadvantage: harder to tell when > "Custom" item is selected. Maybe set an id on the custom item, and use > dialog.namespace.disabled = dialog.menulist.selectedItem.id != "custom"; I liked this idea a lot better than the existing code. Easier to follow. > > case "cmdEditEdit": > >- return this.mAttrTree.view.selection.count == 1; > >+ return this.selectedAttribute != null && > >+ this.mAttrTree.view.selection.count == 1; > How can this.selectedAttribute be null if the count is 1? Got me, but without checking if it is null, the Edit command was available in the menu. > >+ this.subject.removeAttributeNS(this.previousNamespaceURI, > >+ attr.nodeName); > >+ this.subject.setAttributeNS(out.namespaceURI, > >+ attr.nodeName, > >+ out.value); > By my reading of some documentation, which I freely admit was not the correct > documentation, removeAttributeNS takes a localName parameter while > setAttributeNS takes a qualifiedName parameter. The upshot is that if I call > setAttributeNS(XLINK_NS, "xlink:href", "http://www.mozilla.org/") then I don't > know whether I need to remove it using removeAttributeNS(XLINK_NS, > "xlink:href") or removeAttributeNS(XLINK_NS, "href")... The documentation you read is in fact correct. After looking at the source code and W3C spec, removeAttributeNS takes a localName and setAttributeNS takes a qualifiedName. So, I'm using attr.localName for removing now which should take care of everything as expected. ---------- One issue with this patch that perhaps someone else can shed some light on. When setting an attribute namespace to XMLNS it seems that I get this error: Error: uncaught exception: [Exception... "An attempt was made to create or change an object in a way which is incorrect with regard to namespaces" code: "14" nsresult: "0x8053000e (NS_ERROR_DOM_NAMESPACE_ERR)" location: "chrome://inspector/content/viewers/domNode/domNode.js Line: 490"] This happens every time, and I'm not sure why. This is the only namespace that it seems to do this as well. The line of code is: 488 > this.subject.setAttributeNS(out.namespaceURI, 489 > attr.nodeName, 490 > out.value);
Attachment #230929 - Attachment is obsolete: true
Attachment #231427 - Flags: superreview?(neil)
Attachment #231427 - Flags: review?(timeless)
Attachment #230929 - Flags: superreview?(neil)
(In reply to comment #27) > When setting an attribute namespace to XMLNS it seems that I get this error: > Error: uncaught exception: [Exception... "An attempt was made to create or > change an object in a way which is incorrect with regard to namespaces" code: > "14" nsresult: "0x8053000e (NS_ERROR_DOM_NAMESPACE_ERR)" location: > "chrome://inspector/content/viewers/domNode/domNode.js Line: 490"] SetPrefix http://lxr.mozilla.org/seamonkey/source/content/base/src/nsDOMAttribute.cpp#456 calls IsValidNodeName http://lxr.mozilla.org/seamonkey/source/content/base/src/nsContentUtils.cpp#3102 which seems to include relevant comments.
(In reply to comment #28) > (In reply to comment #27) > > When setting an attribute namespace to XMLNS it seems that I get this error: > > Error: uncaught exception: [Exception... "An attempt was made to create or > > change an object in a way which is incorrect with regard to namespaces" code: > > "14" nsresult: "0x8053000e (NS_ERROR_DOM_NAMESPACE_ERR)" location: > > "chrome://inspector/content/viewers/domNode/domNode.js Line: 490"] > > SetPrefix > http://lxr.mozilla.org/seamonkey/source/content/base/src/nsDOMAttribute.cpp#456 > calls IsValidNodeName > http://lxr.mozilla.org/seamonkey/source/content/base/src/nsContentUtils.cpp#3102 > which seems to include relevant comments. > So, that means XMLNS shouldn't be an option for us to set as a namespace, correct? It seems like it should never be that based on that code.
Comment on attachment 230987 [details] oninput Test Case Changing the mimetype so that bugzilla will let me comment on it...
Attachment #230987 - Attachment mime type: application/vnd.mozilla.xul+xml → text/plain
Comment on attachment 230987 [details] oninput Test Case > function initialize() > { > var accept = document.documentElement.getButton("accept"); > var txtbox = document.getElementById("txt"); > accept.disabled = txtbox.value == ""; > } > > function toggleAccept() > { > var accept = document.documentElement.getButton("accept"); > accept.disabled = this.value == ""; > } > </script> > <textbox id="txt" oninput="toggleAccept();"/> Obviously adding toggleAccept as an event listener works because the event listener fires in the context of the textbox element. However as it stands this code is incorrect because the event listener is now the anonymous function(event) { toggleAccept(); } created by the event attribute, and of course that function calls toggleAccept in the scope in which it was defined, which is the window, not the textbox. Fortunately this example is easily corrected by changing it to call initialize (i.e. oninput="initialize();").
(In reply to comment #25) >dialog.namespace.disabled = dialog.menulist.selectedItem.id != "custom"; Looking at the patch I think I should have suggested "mi_custom" as the id.
>So, that means XMLNS shouldn't be an option for us to set as a namespace, >correct? It seems like it should never be that based on that code. No, it means that the following: xmlns:xmlns is illegal xmlns and xmlns:* are only legal in and for the XMLNS namespace *:* is illegal in the null namespace (SetAttribute permits it, go figure) xml:* is only legal in the XML namespace Everything else is legal Clear? ;-)
Comment on attachment 231427 [details] [diff] [review] v2.6 Outstanding issues: 1. "Custom" not automatically preselected (you forgot to set the value of the custom menuitem) 2. "Custom" menuitem id should be "mi_custom" (ok that was my fault) 3. Still need to switch to using oninput attribute. 4. Still don't understand the selectedAttribute null test - the edit menuitem already works for me. 5. removeAttributeNS only takes two parameters. 6. No need for spacing out "var doc =" 7. Make out.accepted default to false, not null. 8. (Re)do doesn't handle a namespace change.
Attachment #231427 - Flags: superreview?(neil) → superreview-
(In reply to comment #33) > Clear? ;-) Err...I think so. I'm just not clear on whether or not I need to do anything for it, or just let the user screw up. (In reply to comment #34) > Outstanding issues: > 1. "Custom" not automatically preselected (you forgot to set the value of the > custom menuitem) > 2. "Custom" menuitem id should be "mi_custom" (ok that was my fault) > 3. Still need to switch to using oninput attribute. All fixed in next patch > 4. Still don't understand the selectedAttribute null test - the edit menuitem > already works for me. Alright, so here's the deal with that. I did some dumping of variables to the debugger. this.selectedAttribute is null, as expected, however, this.mAttrTree.view.selection.count is 1. This is with no attributes in the viewer. How to reproduce: 1) Navigate to http://www.mozilla.org/projects/minefield/ 2) Open the DOM Inspector, and click on HTML 3) In the Object - DOM Node pane, right click. Edit should be selectable here. This is why I'm checking for null. I'm open to suggestions though. > 5. removeAttributeNS only takes two parameters. Whoops... > 6. No need for spacing out "var doc =" Fixed in next patch > 7. Make out.accepted default to false, not null. Fixed in next patch - also allows for the removal of the oncancel function. > 8. (Re)do doesn't handle a namespace change. Good catch. I think I completely neglected that. --------- I'll wait for feedback, finish modifying the files, and then submit a new patch.
Comment on attachment 231427 [details] [diff] [review] v2.6 >+<!-- Chances are you don't want to localize these --> You'll want to use the localization note syntax, otherwise the localizers probably won't notice. > #LOCALIZATION NOTE (root.title) label displayed for the tree head of the JavaScript Object tree >+ accept.disabled = this.mData.name == null; >+ this.nodeName.value = this.mData.name || ""; >+ this.nodeName.disabled = this.mData.name != null; !accept.disabled ? no point in recalculating a value you already have. >+ this.nodeValue.value = this.mData.value || ""; >+ this.menulist.disabled = !this.enableNamespaces(); >+ defaultNS.value = this.mDoc.documentElement.namespaceURI; >+ this.menulist.value = this.mData.namespaceURI; >+ <menulist id="ml_namespace" oncommand="dialog.toggleNamespace();"> >+ <menupopup> >+ <menuitem label="&namespaceTitle.null.label;" value=""/> >+ <menuitem label="&namespaceTitle.XMLNS.label;" >+ value="http://www.w3.org/2000/xmlns/"/> >+ <menuitem label="&namespaceTitle.XML.label;" >+ value="http://www.w3.org/XML/1998/namespace"/> >+ <menuitem label="&namespaceTitle.XHTML.label;" >+ value="http://www.w3.org/1999/xhtml"/> >+ <menuitem label="&namespaceTitle.XLink.label;" >+ value="http://www.w3.org/1999/xlink"/> >+ <menuitem label="&namespaceTitle.XSLT.label;" >+ value="http://www.w3.org/1999/XSL/Transform"/> >+ <menuitem label="&namespaceTitle.XBL.label;" >+ value="http://www.mozilla.org/xbl"/> >+ <menuitem label="&namespaceTitle.MathML.label;" >+ value="http://www.w3.org/1998/Math/MathML"/> >+ <menuitem label="&namespaceTitle.RDF.label;" >+ value="http://www.w3.org/1999/02/22-rdf-syntax-ns"/> >+ <menuitem label="&namespaceTitle.XUL.label;" >+ value="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"/> >+ <menuitem label="&namespaceTitle.SVG.label;" >+ value="http://www.w3.org/2000/svg"/> >+ <menuitem label="&namespaceTitle.XMLEvents.label;" >+ value="http://www.w3.org/2001/xml-events"/> >+ <menuitem label="&namespaceTitle.WAIRoles.label;" >+ value="http://www.w3.org/2005/01/wai-rdf/GUIRoleTaxonomy"/> >+ <menuitem label="&namespaceTitle.WAIProperties.label;" >+ value="http://www.w3.org/2005/07/aaa"/> >+ <menuitem label="&namespaceTitle.default.label;" >+ id="mi_namespace"/> >+ <menuitem label="&namespaceTitle.custom.label;" >+ id="custom"/> >+ </menupopup> >+ </menulist> I think the menupopup should be id'd so people can overlay it and add children. > doCommand: function() > { i know it wasn't your code, but this should be rewritten to: if (!this.attr) return this.promptFor(); this.subject.setAttributeNS(...) return false; multiline ifs with elses without braces for the if line are bad. and there's no point in doing else return in general, since you can rewrite it as if (!original_condition) return ...; original_if_content note that it wasn't as much of a problem in the original code because it was a single line if statement (it still wasn't great code, but...). > if (this.attr) >- this.subject.setAttribute(this.attr.nodeName, this.attr.nodeValue); >+ this.subject.setAttributeNS(this.attr.namespaceURI, >+ this.attr.nodeName, >+ this.attr.nodeValue); > else > return this.promptFor(); > return false; > }, > doCommand: function() > { > if (this.attr) same comment >- this.subject.setAttribute(this.attr.nodeName, this.newValue); >+ this.subject.setAttributeNS(this.newNamespaceURI, >+ this.attr.nodeName, >+ this.newValue); > else > return this.promptFor(); > return false; > },
Attachment #231427 - Flags: review?(timeless) → review-
Attached patch v2.7 (obsolete) — — Splinter Review
> I think the menupopup should be id'd so people can overlay it and add children. I went ahead and id'd everything so that they can also control where it goes in the menu. I put comments dictating special placement of things as well. ---------- I believe I addressed everything that was brought up by the reviewers.
Attachment #231427 - Attachment is obsolete: true
Attachment #232563 - Flags: superreview?
Attachment #232563 - Flags: review?(timeless)
Attachment #232563 - Flags: superreview? → superreview?(neil)
(In reply to comment #35) >(In reply to comment #33) >>Clear? ;-) >Err...I think so. I'm just not clear on whether or not I need to do anything >for it, or just let the user screw up. I guess we'll let the user screw up for now ;-) >(In reply to comment #34) >>4. Still don't understand the selectedAttribute null test - the edit menuitem >>already works for me. >Alright, so here's the deal with that. I did some dumping of variables to the >debugger. this.selectedAttribute is null, as expected, however, >this.mAttrTree.view.selection.count is 1. This is with no attributes in the >viewer. >This is why I'm checking for null. I'm open to suggestions though. Yeah, this can happen when the current index is -1, so I'd change that part of the test to this.mAttrTree.view.currentIndex >= 0
Comment on attachment 232563 [details] [diff] [review] v2.7 sr=me as per comment 38.
Attachment #232563 - Flags: superreview?(neil) → superreview+
Attachment #232563 - Flags: review?(timeless) → review+
Attached patch v2.8 - Final — — Splinter Review
Neil - your code wasn't quite right. It ended up being this.mAttrTree.currentIndex >= 0. This can be checked in. sr=neil,r=timeless
Attachment #232563 - Attachment is obsolete: true
Attachment #232785 - Flags: superreview+
Attachment #232785 - Flags: review+
Whiteboard: [checkin needed]
extensions/inspector/resources/content/viewers/domNode/domNodeDialog.js 1.1 extensions/inspector/resources/locale/en-US/inspector.properties 1.8 extensions/inspector/resources/content/viewers/domNode/domNodeDialog.xul 1.1 extensions/inspector/resources/content/viewers/domNode/domNode.js 1.22 extensions/inspector/resources/locale/en-US/viewers/domNode.dtd 1.5
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Target Milestone: --- → mozilla1.9alpha
The original checkin missed one file, oops! Thanks to Dave Townsend for pointing it out. mozilla/extensions/inspector/jar.mn 1.23
Attached patch Fixed typo — — Splinter Review
Whoops...good catch :)
Checked in the typo fix. mozilla/extensions/inspector/resources/locale/en-US/viewers/domNode.dtd 1.6
Because this works by deleting and creating attributes then even once bug 345982 is fixed this still changes the order of the attributes in the list and additionally the changed attribute is no longer selected.
(In reply to comment #46) > Because this works by deleting and creating attributes then even once bug > 345982 is fixed this still changes the order of the attributes in the list > and additionally the changed attribute is no longer selected. Well, I don't seem to see what is wrong with the current code. For editing, which is where we have the problem, the only difference is the call to setAttribute and setAttributeNS. I call each with the same parameters, and get the namespace of the element with the namespaceURI property. I'm at a loss here.
It looks like the dialog returns "" for the null namespace. Now setAttributeNS doesn't care whether you pass null or "" but you first try to compare the namespace to the old null value which fails.
Alright, this fixes that issue for me - Neil you may wish to nit some of the formatting, so I'm requesting review.
Attachment #236853 - Flags: review?(neil)
Comment on attachment 236853 [details] [diff] [review] Fix for disappearing attribute bug I think you could write out.namespaceURI || null; and aren't you supposed to ask timeless for review and me for sr?
(In reply to comment #50) > I think you could write out.namespaceURI || null; and aren't you supposed to > ask timeless for review and me for sr? I hadn't expected that to work actually, but I suppose it makes sense (and tests indicate that it does work).
Attachment #236853 - Attachment is obsolete: true
Attachment #236886 - Flags: superreview?(neil)
Attachment #236886 - Flags: review?(timeless)
Attachment #236853 - Flags: review?(neil)
Attachment #236886 - Flags: superreview?(neil) → superreview+
Attachment #236886 - Flags: review?(timeless) → review+
(In reply to comment #18) <...> > > >+ <menuitem label="Default" id="mi_namespace"/> > >+ <menuitem label="XHTML" value="http://www.w3.org/1999/xhtml"/> > >+ <menuitem label="XUL" value="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"/> > >+ <menuitem label="Custom" value="custom"/> > > These should probably be entities. Even if you think that no one will ever want > to localize them, in which case the dtd file should have localization notes > suggesting that it not be translated. > That's wrong. Don't add strings to l10n just for fun. If you think that it's a good idea to have those in a separate file, put a dtd file into content instead of l10n. One could argue that "Custom" is not generic, though.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch l10n fix v1.0 (obsolete) — — Splinter Review
As per conversation on irc.
Attached patch l10n fix v1.0 — — Splinter Review
As per our conversation on irc.
Attachment #240514 - Attachment is obsolete: true
Attachment #240515 - Flags: review?(l10n)
Comment on attachment 240515 [details] [diff] [review] l10n fix v1.0 Well, XUL is not a w3c spec, but it's close enough. I thought about not putting the abbreviations in there for a moment, but there may be locales that want to transcribe the latin chars, so let's let them do that. r=me for the change of the localization note, and moving "custom" out of that category.
Attachment #240515 - Flags: review?(l10n) → review+
the l10n patch needs to be checked in still so this bug can be closed.
Whiteboard: [checkin needed]
l10n fix checked in by smaug.
Status: REOPENED → RESOLVED
Closed: 18 years ago → 18 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Depends on: 366287
QA Contact: timeless → dom-inspector
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: