Closed
Bug 112775
Opened 23 years ago
Closed 17 years ago
DOM Inspector to create nodes
Categories
(Other Applications :: DOM Inspector, enhancement)
Other Applications
DOM Inspector
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: WeirdAl, Assigned: mossop)
References
(Blocks 1 open bug)
Details
(Keywords: helpwanted)
Attachments
(2 files, 16 obsolete files)
6.38 KB,
application/vnd.mozilla.xul+xml
|
Details | |
31.36 KB,
patch
|
db48x
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
I love DOM Inspector already, from the November 9, 2001 Developer's Day. But one feature I'm looking for is the ability to create nodes from scratch, and paste them into a document or document fragment.
Updated•23 years ago
|
OS: Windows 98 → All
Hardware: PC → All
Updated•23 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → Future
Reporter | ||
Comment 1•22 years ago
|
||
hewitt, I'm working on a DOM-I-like XUL+JS called Node Creator. If you want to assign this bug to me, go ahead. We can mind-meld DOM-I and NC later.
Reporter | ||
Comment 2•22 years ago
|
||
brantgurga wants RFE deprecated. I'm happy to oblige him. That's not why I'm writing now. I've gone back to my offline computer and tinkered with a 1.1a+ build using Gerv's deprecated Patch Maker. I've modified the following files: content/viewers/dom/dom.xul content/viewers/dom/dom.js content/editingOverlay.xul I think I've modified this file: resources/locale/en-US/editing.dtd And I've created two new files, currently located locally at chrome://inspector/content/viewers/dom/creatorDialog.xul chrome://inspector/content/viewers/dom/creatorDialog.js In the files I modified, all I did was add code. Bonsai indicates the only one of these files to undergo any significant change is dom.js -- and looking at the source code, it appears at first glance that the parts in question that I'm looking at have not changed significantly. In other words, the patch I have (which I will have to make available later) appears compatible with Mozilla 1.2b+. (Translation: though I built the patch on old code, the code hasn't bitrotted in the most important parts to the patch.) The current version adds a submenu where you right-click on an element, above the "Delete" menu item. The submenu is titled "Insert node...", and offers five options: before this node, after this node, in place of this node (replace), as first child, and as last child. The current version allows you to create and insert elements, text nodes, CDATA nodes, comments and processing instructions. I organized the JS to actually follow the pattern of the current cmdEditBlahBlah functions, also, so undo works too. As it is right now, it's probably good enough to set a milestone of 1.3 alpha, and I recommend so. Only thing I don't like about it is that it can't edit and/or split text nodes -- but that's a different bug, I think. Patch for the modified files and static versions of the new files coming in a few days via ZIP, (ie, when I have a floppy disk with me). I'd like advice on where the two new files should really be placed.
Summary: RFE: DOM Inspector to create nodes → DOM Inspector to create nodes
Reporter | ||
Comment 3•22 years ago
|
||
Patch Maker doesn't allow me to create files, so I had to include them separately. The diff and the two new files are included as a ZIP. Also, there's one specific part about this patch that I do not like: I hard-coded a few namespaces in. See notes in the patch.
Reporter | ||
Updated•22 years ago
|
Reporter | ||
Comment 4•22 years ago
|
||
8) Bug 98815 almost certainly solves the problem I had with the patch. Maybe next week I can have a version which uses DOM 3 methods. That being said, the patch is otherwise good.
Reporter | ||
Comment 5•22 years ago
|
||
I'm not sure if I formatted the --- and +++ lines correctly, but otherwise it is a working patch.
Attachment #105515 -
Attachment is obsolete: true
Reporter | ||
Comment 6•22 years ago
|
||
Hopefully this one is better.
Attachment #105708 -
Attachment is obsolete: true
Reporter | ||
Comment 7•22 years ago
|
||
Thanks, timeless.
Attachment #105714 -
Attachment is obsolete: true
Comment 8•22 years ago
|
||
Comment on attachment 105730 [details] [diff] [review] patch, v4, with the correct license this time This is not a full-fledged review: I am just pointing out some things. 0. I intend to redo the UI and I'm strongly considering going with a UI approach similar to what venkman uses. It may be confusing at first glance, but after getting into it, it is very highly modular and configurable for an app which uses several views at once and needs to do many things in different situations, like inspector does. So this work might be obsoleted. There's fair warning. ;-) That said, 1. Your coding style needs some attention. Binary operators have spaces before and after them. Your indentation is wacky in some places (why on earth are your comments breaking the indentation rules of the blocks they are contained in? Also why are they in the middle of the block if they apply to the whole function?). Major blocks (e.g. functions) get extra newlines after them. Please indent the stuff within your cases... In your XUL, one element per line please. etc. 2. I don't really like your error handling. Why do you let the user click on a menu item which pops open a window, then pops up an alert to say "you can't do that" and then closes both windows? That is extremely lame UI. Disable the menu item in that case. 3. What's up with the code in several places that does this: >+ var node = this.node ? this.node : viewer.selectedNode; >+ this.node = node; >+ var insertedNode = this.insertedNode; Do if (!this.node) { this.node = viewer.selectedNode; } And there's also no need for the temp var for insertedNode. Just use it off of |this|. 4. Script elements should precede the document element. 5. Don't execute JS in the global scope. Use an onload handler and a function. The less stuff in the global scope, the less potential conflicts we have. I also have a few implementation questions that I don't have the time to write down now, as I've gotten swamped in the last few hours actually, and I have a date at Hogwarts coming up soon. I'll ping you later about them. In the meanwhile, could you post a screenshot of some stuff? I get a general idea from your xul, but pictures are worth more than my interpretations of code...
Reporter | ||
Comment 9•22 years ago
|
||
I'll get back to you on all that. Coding style I'm not sure how to clean up for mozilla.org conventions. Script element before the document element -- wouldn't that violate XML well-formedness? The rest of this, I think I can accomplish without much trouble at all. I'll zip up some screenshots for you of the dialog box and the modified context menu.
Comment 10•22 years ago
|
||
Err, um yeah. I was finishing up the comments in haste and I've no idea where that came from. I meant to say that the indentation level of scripts should not be the same as the document element.
Assignee: hewitt → ajvincent
Status: ASSIGNED → NEW
Reporter | ||
Comment 11•22 years ago
|
||
Hopefully I got everything caillon asked for. I think I did. I also did a little more work to prevent wrong situations from happening (like an text/html document receiving a CDATA section, bug 27403 ). So the overall result is fairly smooth.
Attachment #105730 -
Attachment is obsolete: true
Reporter | ||
Updated•22 years ago
|
Target Milestone: Future → mozilla1.3alpha
Reporter | ||
Comment 12•22 years ago
|
||
Tree frozen for 1.3a. It'd be nice if I could get a r=.
Target Milestone: mozilla1.3alpha → mozilla1.3beta
Reporter | ||
Updated•22 years ago
|
Attachment #106744 -
Flags: review?(caillon)
Comment 13•22 years ago
|
||
Comment on attachment 106744 [details] [diff] [review] patch, v5 const kClipboardHelperCID = "@mozilla.org/widget/clipboardhelper;1"; +const nsIDOMNode = Components.interfaces.nsIDOMNode; What about lining up the = with the one in the lines above? + this.dialogTitle = "Insert node as first child of this node"; shouldn't this be localizable? similar in other places + // XUL documents do not allow creation of ProcessingInstruction nodes -- known bug. care to cite a bugnumber here? + alert("Aborted: Qualified names must have at most one colon.\n" + qname); IIRC you're supposed to not use alert in chrome, but instead use nsIPromptService... Also, this is not localizable. + var element_data = parseQName(qname); isn't it normal style to use interCaps? It would look better if you indented the text after case: labels...
Comment 14•22 years ago
|
||
Comment on attachment 106744 [details] [diff] [review] patch, v5 requesting from the right address this time (on behalf of Alex Vincent)
Attachment #106744 -
Flags: review?(caillon) → review?(caillon)
Reporter | ||
Comment 15•22 years ago
|
||
Comment on attachment 106744 [details] [diff] [review] patch, v5 Patch needs work. I answered biesi's concerns (localization is spotty at best in DOM Inspector as currently implemented), and then caillon and I entered into a debate regarding prefix vs namespace URI vs both, and I'm trying to figure out the best route now.
Attachment #106744 -
Attachment is obsolete: true
Attachment #106744 -
Flags: review?(caillon)
Reporter | ||
Comment 16•22 years ago
|
||
Doubtful I'll be able to get this in before 1.3 beta -- I'm on vacation, and don't have a Mozilla build to test my ideas on. I return home on Jan 18, but that only leaves four days for caillon to review. I wonder if they'll allow this for checkin to 1.3 itself. If not, we'll have to bump this back to 1.4 alpha. I'd really rather avoid that.
Reporter | ||
Comment 17•22 years ago
|
||
Not going to make 1.3 beta, unless we get a miracle.
Target Milestone: mozilla1.3beta → mozilla1.4alpha
Reporter | ||
Comment 18•21 years ago
|
||
caillon asked for the new feature to be 100-200 lines... the bad news is it weighs in currently at almost exactly 300 lines (XUL + JS), and there's still a few bugs in it. (I've included documentation in a textbox.) I'm providing this attachment mainly as a proof-of-concept, and I'd like feedback (not a formal review, but if this works out, the majority of this code will end up in my next patch for this bug).
Reporter | ||
Comment 19•21 years ago
|
||
Reporter | ||
Comment 20•21 years ago
|
||
The preceding attachment is the XUL document -- one second while I fix it...
Reporter | ||
Comment 21•21 years ago
|
||
Much-improved over original version. Looking for feedback, particularly on whether you'd appreciate using something like this in the create nodes widget.
Attachment #112754 -
Attachment is obsolete: true
Attachment #114021 -
Attachment is obsolete: true
Reporter | ||
Comment 22•21 years ago
|
||
biesi identified a bug in the insertNode() function -- calling setAttributeNS() on a Document node when it should call on the documentElement. "earthsound" identified a few errors when this widget runs in Mozilla 1.3a; recent nightly doesn't have those errors.
Reporter | ||
Comment 23•21 years ago
|
||
I've started porting the XUL/JS/CSS to the creator dialog box, and I've encountered a glitch: if the user brings up the dialog box and then hits Cancel, undo/redo is broken. By that I mean a new command has been added to the undo history, but nothing happened. This is unacceptable because if the user had done other commands, undid them, and called an insert node method, and then canceled, there is no way for the user to redo the commands they undid. I have two options at this point: hack the undo history in Inspector to allow for "temporary stack" commands, or change my approach and make the create nodes feature a panel for the right side. caillon: which do you prefer?
Reporter | ||
Comment 24•21 years ago
|
||
If I were to hack the history stack, it would be done by splitting execCommand() from inspector.xml: http://lxr.mozilla.org/seamonkey/source/extensions/inspector/resources/content/inspector.xml#207 into two separate functions -- with the part inside the "if (!noPush) {...}" brackets going into a new method.
Reporter | ||
Comment 25•21 years ago
|
||
This one's a doozy: 69KB. Note this patch depends on attachment 114685 [details] [diff] [review] of bug 193726 being checked in. This patch includes: * new commands for dom.js and editingOverlay.xul, with labels in editing.dtd and a slight modification to dom.xul * documentation of the creatorDialog widget in a help file you can reach from the widget * CSS stylesheet for certain effects of the widget, no XBL bindings included * JavaScript functions for basic operation, namespace handling and on-the-fly validation (if invalid, the ok button is disabled) * Scrolling boxes for multiple attributes * Zero known JavaScript errors or strict warnings
Attachment #114022 -
Attachment is obsolete: true
Attachment #114023 -
Attachment is obsolete: true
Reporter | ||
Comment 26•21 years ago
|
||
Comment on attachment 114694 [details]
patch, v6
Patch needs more work. biesi gave me some QA love, (he and I appear to have
switched roles for this bug), and helped me discover the patch is no good for
HTML documents. (Namespaces. Nuff said.)
Needless to say, that is quite annoying.
The solution I have right now is to add a separate version of the widget
specifically for HTML documents, and to discriminate between the two. This
results in approximately 20KB (and three files) being added to the patch when
I'm done with it. That doesn't count changes to the help file.
It was either that or (1) load the whole XUL+JS+CSS, (2) add more spaghetti
code to the JS for when XML mode is on and when it's not, (3) lots of DOM
interaction to remove XML-specific code, and (4) creating another CSS
stylesheet anyway...
The current detection for XML documents will be a new method of the viewer
object of dom.js:
creatorDialog_fileName: function() {
var ifaces = Components.interfaces;
var response = "";
var bool = ((this.subject instanceof
ifaces.nsIDOMXMLDocument)||(this.subject instanceof ifaces.nsIDOMXULDocument));
if (bool) {
response =
"chrome://inspector/content/viewers/dom/creatorDialog_XML.xul";
} else {
response =
"chrome://inspector/content/viewers/dom/creatorDialog_HTML.xul";
}
return response;
}
Attachment #114694 -
Attachment is obsolete: true
Attachment #114694 -
Attachment is patch: false
Reporter | ||
Comment 27•21 years ago
|
||
Once again, you need to check in attachment 114685 [details] [diff] [review] as a prerequisite to checking in this patch. biesi warned me of a hang in Mozilla using the preceding patch with a certain number of steps. I have unfortunately discovered a more generalized testcase which hangs Mozilla 100% of the time with this patch. (1)Insert a node. (2)Append a child node to the inserted node. (3)Replace the node you inserted, and Mozilla hangs. I do not know why this hang happens. The commands I've inserted use only the Document Object Model to modify the original document. A standalone testcase replicating these results without DOM Inspector does not cause a hang. zach suggests (not unreasonably) that I should fix the hang before this patch is checked in. I however have no way of diagnosing where the actual hang happens. I have observed the dialog box closing before Mozilla hangs, so the added files are above suspicion. Also, since you can replace a node you have not inserted before without a hang, that suggests my modifications to the existing code are not at fault.
Reporter | ||
Comment 28•21 years ago
|
||
helpwanted: I need to find what exactly is causing the hang comment 27 describes. If you find it, and particularly if you can fix it, please let me know.
Keywords: helpwanted
Reporter | ||
Comment 29•21 years ago
|
||
Re comment 27: my code is definitely not at fault. The modified code executes doCommand() and thus modifies the DOM of the inspected document before the dialog box closes. The hang happens after the dialog box closes. Based on the known hang, I am inclined to ask for drivers' a= following a successful r= for a patch on this bug, even if it is not required.
Reporter | ||
Comment 30•21 years ago
|
||
Re patch v7, there are two VirtualAttr.prototype.toString() functions. I left them in there for diagnostic purposes, but unfortunately a bad disk sector on my floppy corrupted one of them, resulting in a JS error. You may safely remove them without ill effect, and I recommend doing so.
Reporter | ||
Comment 31•21 years ago
|
||
Comment on attachment 115449 [details]
patch, v7
patch is broken; blame the floppy.
Attachment #115449 -
Attachment is obsolete: true
Attachment #115449 -
Attachment is patch: false
Reporter | ||
Comment 32•21 years ago
|
||
thanks to biesi for bearing with me while I helped him fix the damage. Borrowed code from v6 of this patch, which hadn't changed by version.
Reporter | ||
Comment 33•21 years ago
|
||
Comment on attachment 115578 [details] [diff] [review] patch, v7.1 caillon took a quick glance at this. His commentary so far: >+ creatorDialog_fileName: function() { >+ var ifaces = Components.interfaces; + const ifaces = Components.interfaces; >+ this.isInStack = false; caillon prefers this.inStack, but he thinks this is nitpicking. I prefer isInStack. >+ <meta http-equiv="Content-Type" content="text/html"> >+ <title></title> caillon wants me to lose the meta tag and include a title. No problem. + <title>DOM Inspector Create Nodes Help Page</title> I have to change the line number count, though. >+ max-height: 150px; >+ height:150px; >+ max-width:600px; + max-height: 150px; + height: 150px; + max-width: 600px; >+[width="textfield"] { >+ min-width:250px; + min-width: 250px; >+#deck { >+ max-width:600px; + max-width: 600px; Later, in creatorDialog_XML.css, +#attrsBox { + overflow: -moz-scrollbars-vertical; + max-height: 150px; + height: 150px; + max-width: 600px; } + +#nsAttrsBox { + overflow: -moz-scrollbars-vertical; + max-height: 100px; + height: 100px; + max-width: 600px; +} + +[width="uri"] { + min-width: 250px; +} + +[width="prefix"] { + min-width: 80px; +} + +[width="textfield"] { + min-width: 73px; +} + +[width="nsTextfield"] { + min-width: 168px; +} + +#deck { + max-width: 600px; +} Think that's everything caillon saw at first glance. He recommended that if he wasn't available to get reviews from sicking and/or bz. Requesting r=sicking, sr=roc (because he was nice enough to respond on bug 193726 -- sorry, roc). Due to the known hang mentioned earlier and the short amount of time left for 1.4a, asking for drivers' a= is still an option in my book.
Attachment #115578 -
Flags: superreview?(roc+moz)
Attachment #115578 -
Flags: review?(bugmail)
Reporter | ||
Comment 34•21 years ago
|
||
Comment on attachment 115578 [details] [diff] [review] patch, v7.1 Correction: after checking with caillon, sr= requested from bz.
Attachment #115578 -
Flags: superreview?(roc+moz) → superreview?(bzbarsky)
Comment 35•21 years ago
|
||
Um... I won't be able to review this for 1.4a. I may not be able to review this for 1.4b. I have lots of high-priority layout reviews on my plate and very little time to spend on reviews....
same here, there is no way that i'll be able to r this for 1.4a :( Not sure if you'll be allowed to land this during beta or not (don't know how strict the no-new-features-during-beta rule is)
Reporter | ||
Comment 37•21 years ago
|
||
Heh, ok, ok. I figured 1.4a was a long shot, given I had to wait for another bug to be fixed. sicking: is 1.4b an option? bz: who should I ask sr= of?
Target Milestone: mozilla1.4alpha → mozilla1.4beta
Comment 38•21 years ago
|
||
Um... peterv? dmose? jst? heikki? jag? hewitt? (depends on what the patch mostly does).
Reporter | ||
Comment 39•21 years ago
|
||
Comment on attachment 115578 [details] [diff] [review] patch, v7.1 per e-mail with bz.
Attachment #115578 -
Flags: superreview?(bzbarsky) → superreview?(jaggernaut)
Comment 40•21 years ago
|
||
I'm not going to get to this by 1.4a. This seems like a rather big change for 1.4b... Should this perhaps go into 1.5a?
Reporter | ||
Comment 42•21 years ago
|
||
Comment on attachment 115578 [details] [diff] [review] patch, v7.1 patch will need localization.
Attachment #115578 -
Attachment is obsolete: true
Attachment #115578 -
Flags: superreview?(jaggernaut)
Attachment #115578 -
Flags: review?(bugmail)
Reporter | ||
Updated•21 years ago
|
Target Milestone: mozilla1.5alpha → mozilla1.6alpha
Reporter | ||
Comment 43•20 years ago
|
||
caillon: if you recall earlier screenshots of the dialog box my patch would pop up, it was a very detailed UI. However, I've recently come up with an idea which may greatly simplify this bug to a dialog box with only a multiline textbox and OK/Cancel buttons. var parser = new DOMParser(); var aDoc = parser.parseFromString("<foo>" + textbox.value + "</foo>", "application/xml"); if (aDoc.documentElement.tagName == "parsererror") return false; // the fragment was not well-formed if (aDoc.documentElement.childNodes.length == 1) return true; // we got a single node, probably a text node if (aDoc.documentElement.childNodes.length == 3) { if ((aDoc.documentElement.firstChild.nodeType == nsIDOMNode.TEXT_NODE) && (aDoc.documentElement.childNodes[1].nodeType == nsIDOMNode.ELEMENT_NODE) && (aDoc.documentElement.lastChild.nodeType == 3)) { // well, we probably got an element node with whitespace on both sides, check it in detail and clean it up } } What do you think? A simple textbox is nicer, and allows the user to create a lot more complex content on-the-fly. Are there security risks to that approach? A potential downside to this is the user adding in elements like <html:script/>, but I don't see how that could be a major concern for people who use DOM Inspector. Plus, localization would be a snap.
Reporter | ||
Comment 44•20 years ago
|
||
Something like this will be far easier for the DOM Inspector user to use, and of course to maintain in the future.
Updated•20 years ago
|
Product: Core → Other Applications
Reporter | ||
Comment 45•19 years ago
|
||
If you don't have XMLExtras (specifically, DOMParser) enabled in your builds, the demo will run marginally slower. Something very similar to this will be my next patch for DOM Inspector on this bug.
Attachment #150613 -
Attachment is obsolete: true
Reporter | ||
Comment 46•18 years ago
|
||
Reassigning DOM-I bugs which have stagnated in my buglist back to default owner. Hopefully someone will pick up some of these bugs and work on them. I'll continue to follow them.
Assignee: ajvincent → dom-inspector
Target Milestone: mozilla1.6alpha → ---
Assignee | ||
Comment 47•17 years ago
|
||
Personally I believe that this bug needs something far simpler to solve it. Attached is a patch that allows you to insert element and text nodes at any point in the tree with a very simple interface. You simply use the context menu to choose what position relative to the selected node you want to insert, then in the dialog either choose a namespace and tagname for the element, or enter some text for a text node. The dialog was based on the code from 205872, in particular the namespace picker. There are 4 commands for inserting nodes, depending on where they are inserted relative to the selected node. These 4 are written as just extensions of a generic command that handles the dialog displaying and node creation. The code is I think reasonably well written to allow for additional node types to be added using the same dialog if needed in the future.
Assignee: dom-inspector → mossop.bugzilla
Status: NEW → ASSIGNED
Attachment #253955 -
Flags: superreview?(neil)
Attachment #253955 -
Flags: review?(db48x)
Comment 48•17 years ago
|
||
Comment on attachment 253955 [details] [diff] [review] Patch for element and text node creation. >@@ -249,16 +257,24 @@ DOMViewer.prototype = > case "cmdEditPasteReplace": > return new cmdEditPasteReplace(); > case "cmdEditPasteFirstChild": > return new cmdEditPasteFirstChild(); > case "cmdEditPasteLastChild": > return new cmdEditPasteLastChild(); > case "cmdEditPasteAsParent": > return new cmdEditPasteAsParent(); >+ case "cmdEditInsertAfter": >+ return new cmdEditInsertAfter(); >+ case "cmdEditInsertBefore": >+ return new cmdEditInsertBefore(); >+ case "cmdEditInsertFirstChild": >+ return new cmdEditInsertFirstChild(); >+ case "cmdEditInsertLastChild": >+ return new cmdEditInsertLastChild(); > case "cmdEditDelete": > return new cmdEditDelete(); > } > return null; > }, This entire function body can be replaced with one line of code: return new window[aCommand]; Otherwise it doesn't look too bad on first glace. I'll have to apply it and test it before I can fully review it.
Comment 49•17 years ago
|
||
Comment on attachment 253955 [details] [diff] [review] Patch for element and text node creation. >+ if (/^cmdEditPaste/.test(aCommand) || /^cmdEditInsert/.test(aCommand)) { What about: if (/^cmdEdit[Paste|Insert]$/.test(aCommand)) { >+ onInsertPopupShowing: function onInsertPopupShowing(menupopup) { Generally (at least as of late), we've been trying to prefix function arguments with a, so I'd suggest aMenupopup here. General comments: A few places looked like you might have lines longer than 80 characters, and some of the files have a copyright date of 2006, and you might want 2007.
Comment 50•17 years ago
|
||
Comment on attachment 253955 [details] [diff] [review] Patch for element and text node creation. >+ case "cmdEditInsertAfter": >+ return (parentNode) && (parentNode.nodeType == nsIDOMNode.ELEMENT_NODE); >+ case "cmdEditInsertBefore": >+ return (parentNode) && (parentNode.nodeType == nsIDOMNode.ELEMENT_NODE); >+ case "cmdEditInsertFirstChild": >+ return (selectedNode) && (selectedNode.nodeType == nsIDOMNode.ELEMENT_NODE); >+ case "cmdEditInsertLastChild": >+ return (selectedNode) && (selectedNode.nodeType == nsIDOMNode.ELEMENT_NODE); Use instanceof Element for these tests (instanceof also null-checks). > var commandId = menupopup.childNodes[i].getAttribute("command"); > if (viewer.isCommandEnabled(commandId)) > document.getElementById(commandId).setAttribute("disabled", "false"); > else > document.getElementById(commandId).setAttribute("disabled", "true"); > } > }, > >+ onInsertPopupShowing: function onInsertPopupShowing(menupopup) { >+ for (var i = 0; i < menupopup.childNodes.length; i++) { >+ var commandId = menupopup.childNodes[i].getAttribute("command"); >+ if (viewer.isCommandEnabled(commandId)) >+ document.getElementById(commandId).setAttribute("disabled", "false"); >+ else >+ document.getElementById(commandId).setAttribute("disabled", "true"); >+ } >+ }, Aren't these two functions identical (in which case generically rename it)? > <!--<menuitem id="mnEditInsert"/>--> Shouldn't this be removed? >+ var rows = document.getElementsByTagName("row"); >+ for (var i=0; i<rows.length; i++) { >+ if (rows[i].hasAttribute("types")) >+ rows[i].hidden = rows[i].getAttribute("types").indexOf(dialog.nodeType.value)<0; >+ } Let me think about this, I'm not quite convinced yet. Nit: spaces around <
Comment 51•17 years ago
|
||
Comment on attachment 253955 [details] [diff] [review] Patch for element and text node creation. >+<dialog id="InsertNode" >+ xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" >+ title="" The attributes dialog sets its title at run time, but you don't... >+ <label value="&nodeType.label;" control="tx_nodeType"/> >+ <menulist id="ml_nodeType" oncommand="dialog.updateType();"> control="ml_nodeType" surely?
Assignee | ||
Comment 52•17 years ago
|
||
Yes sorry those last two are dumbass mistakes. The title I meant to come up with something sensible before I submitted "Insert Node" ok? (in the dtd of course), the other yeah silly copy and paste error.
Comment 53•17 years ago
|
||
Comment on attachment 253955 [details] [diff] [review] Patch for element and text node creation. >+ </row> >+ <row types="element"> >+ <spacer/> >+ <textbox id="tx_namespace"/> >+ </row> >+ <row types="element"> >+ <label value="&tagName.label;" control="tx_tagName"/> >+ <textbox id="tx_tagname" oninput="dialog.toggleAccept()"/> >+ </row> I think you should wrap these in their own <rows> which means that you will only have to show and hide one of them. sr- given this and previous comments.
Attachment #253955 -
Flags: superreview?(neil) → superreview-
Assignee | ||
Comment 54•17 years ago
|
||
This is an updated patch that addresses all of the previous comments. (In reply to comment #53) > I think you should wrap these in their own <rows> which means that you will > only have to show and hide one of them. sr- given this and previous comments. I'm not totally sure what you meant here. Using separate <rows> for the type chooser, text node options and element options won't work since multiple <rows> stack on top of each other rather than displaying one after the other. For this patch I have split it into two grids though that has the side effect of the node type chooser no longer totally lining up with the following controls. Screenshot coming
Attachment #253955 -
Attachment is obsolete: true
Attachment #254198 -
Flags: superreview?(neil)
Attachment #254198 -
Flags: review?(db48x)
Attachment #253955 -
Flags: review?(db48x)
Assignee | ||
Comment 55•17 years ago
|
||
Comment 56•17 years ago
|
||
(In reply to comment #55) > Created an attachment (id=254199) [details] > Screenshot of current behaviour I think that the whole "not lining up" issue looks really bad, fyi
Assignee | ||
Comment 57•17 years ago
|
||
Forgot to add there's also a minor typo from domNodeDialog.xul corrected in this patch.
Comment 58•17 years ago
|
||
(In reply to comment #54) >I'm not totally sure what you meant here. Using separate <rows> for the type >chooser, text node options and element options won't work since multiple <rows> >stack on top of each other rather than displaying one after the other. I meant something like this, with the second rows inside the first: <grid> <columns><column><column flex="1"></columns> <rows> <row><!-- node type menulist --></row> <rows id="elementView"> <row><!-- namespace menulist --></row> <row><!-- namespace textbox --></row> <row><!-- node name textbox --></row> </rows> <row id="textView" flex="1"><!-- multiline textbox --></row> </rows> </grid>
Comment 59•17 years ago
|
||
Dialog boxes are almost never the correct solution for things like this. http://www.plethora.net/~seebs/ops/ibm/cranky12.html doing it with in-line editing on the tree box would be much nicer.
Comment 60•17 years ago
|
||
(In reply to comment #59) > Dialog boxes are almost never the correct solution for things like this. > http://www.plethora.net/~seebs/ops/ibm/cranky12.html doing it with in-line > editing on the tree box would be much nicer. While I know you are correct, I'm of the opinion that we get this feature implemented first like this. Since everything else in DOMi uses dialogs it isn't a huge deal, but feel free to file a bug to start removing dialogs from DOMi.
Reporter | ||
Comment 61•17 years ago
|
||
What worries me about this patch (from the screenshot) is you can't really define the attributes of an element or any content in one shot. That's why in Verbosio I went with a text-based input where DOM-I would parse the contents. But we can save that issue for a future patch.
Comment 62•17 years ago
|
||
(In reply to comment #61) > What worries me about this patch (from the screenshot) is you can't really > define the attributes of an element or any content in one shot. That's why in > Verbosio I went with a text-based input where DOM-I would parse the contents. > But we can save that issue for a future patch. You can add attributes, but it just requires more dialog pop-ups. I don't think the DOMi is really supposed to be an editor per se, but this is hand to test a small fix out quickly. I think you may have hit on that though...
Assignee | ||
Comment 63•17 years ago
|
||
Wasn't aware of that trick so here's a more final patch. With this the fields line up ok, the only issue is that the width of the second column fluctuates depending on which type of node is being edited. I guess the only way to fix that would be to apply a fixed width though that would be a pain for localisers.
Attachment #254198 -
Attachment is obsolete: true
Attachment #254199 -
Attachment is obsolete: true
Attachment #254748 -
Flags: superreview?(neil)
Attachment #254748 -
Flags: review?(db48x)
Attachment #254198 -
Flags: superreview?(neil)
Attachment #254198 -
Flags: review?(db48x)
Comment 64•17 years ago
|
||
(In reply to comment #63) >With this the fields line up ok, the only issue is that the width of the >second column fluctuates depending on which type of node is being edited. I was thinking radio buttons, but what if someone wants to extend the dialog to add (say) comment nodes? So I decided against it.
Comment 65•17 years ago
|
||
Comment on attachment 254748 [details] [diff] [review] patch rev 3 >+ case "cmdEditInsertAfter": >+ return parentNode instanceof nsIDOMElement; >+ case "cmdEditInsertBefore": >+ return parentNode instanceof nsIDOMElement; >+ case "cmdEditInsertFirstChild": >+ return selectedNode instanceof nsIDOMElement; >+ case "cmdEditInsertLastChild": >+ return selectedNode instanceof nsIDOMElement; I seem to remember caillon didn't like instanceof Element either, so nsIDOMElement is fine. >+ if (aCommand in window) >+ return new window[aCommand](); Nice though db48x's method is, I'm not convinced it's foolproof. (Perhaps he can convince me, or come up with foolproof code.) >+ switch (dialog.nodeType.value) >+ { >+ case "text": >+ document.getElementById("row_text").hidden = false; >+ document.getElementById("row_element").hidden = true; >+ break; >+ case "element": >+ document.getElementById("row_text").hidden = true; >+ document.getElementById("row_element").hidden = false; >+ break; >+ } Why aren't these properties on the dialog object? Note: This way may be more extensible: dialog.rowText.hidden = dialog.nodeType.value != "text"; dialog.rowElement.hidden = dialog.nodeType.value != "element"; I wonder whether we should go for a different structure, maybe a deck? dialog.nodeDeck.selectedIndex = dialog.nodeType.selectedIndex; >+ <grid> You need to restore the flex, otherwise the textbox stops flexing. >+ </row> >+ <rows id="row_element"> Tabs crept in here. sr=me with this fixed.
Attachment #254748 -
Flags: superreview?(neil) → superreview+
Comment 66•17 years ago
|
||
(In reply to comment #65) > >+ if (aCommand in window) > >+ return new window[aCommand](); > Nice though db48x's method is, I'm not convinced it's foolproof. > (Perhaps he can convince me, or come up with foolproof code.) It's foolproof as long as the functions to be called are given the same names as the commands that should cause them to be called. Since that convention has been upheld so far it doesn't seem unreasonable to expect it to continue to be upheld. A comment to that effect wouldn't hurt though. What else is needed for it to be foolproof?
Comment 67•17 years ago
|
||
Comment on attachment 254748 [details] [diff] [review] patch rev 3 awesome. sorry the review took so long. r=db48x
Attachment #254748 -
Flags: review?(db48x) → review+
Updated•17 years ago
|
Whiteboard: [checkin needed]
Comment 68•17 years ago
|
||
checked in
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Updated•17 years ago
|
QA Contact: timeless → dom-inspector
You need to log in
before you can comment on or make changes to this bug.
Description
•