Last Comment Bug 112775 - DOM Inspector to create nodes
: DOM Inspector to create nodes
Status: RESOLVED FIXED
: helpwanted
Product: Other Applications
Classification: Client Software
Component: DOM Inspector (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: ---
Assigned To: Dave Townsend [:mossop]
:
Mentors:
Depends on: 193726
Blocks: 109682
  Show dependency treegraph
 
Reported: 2001-11-29 23:19 PST by Alex Vincent [:WeirdAl]
Modified: 2007-07-06 00:45 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch modifying files, plus two new files not in the patch (6.34 KB, application/octet-stream)
2002-11-07 17:12 PST, Alex Vincent [:WeirdAl]
no flags Details
Patch, including new files and lookupNamespaceURI() (22.21 KB, patch)
2002-11-09 10:59 PST, Alex Vincent [:WeirdAl]
no flags Details | Diff | Splinter Review
patch v3, following suggestions of timeless and caillon (23.68 KB, patch)
2002-11-09 12:16 PST, Alex Vincent [:WeirdAl]
no flags Details | Diff | Splinter Review
patch, v4, with the correct license this time (23.68 KB, patch)
2002-11-09 17:12 PST, Alex Vincent [:WeirdAl]
no flags Details | Diff | Splinter Review
patch, v5 (26.36 KB, patch)
2002-11-18 18:01 PST, Alex Vincent [:WeirdAl]
no flags Details | Diff | Splinter Review
Demo of proposed namespaces feature (work in progress) (22.20 KB, application/vnd.mozilla.xul+xml)
2003-01-27 08:12 PST, Alex Vincent [:WeirdAl]
no flags Details
Demo, v2 (CSS stylesheet) (27.66 KB, text/css)
2003-02-10 08:48 PST, Alex Vincent [:WeirdAl]
no flags Details
Real CSS stylesheet for demo (309 bytes, text/css)
2003-02-10 08:50 PST, Alex Vincent [:WeirdAl]
no flags Details
Demo, v2 (XUL document standalone) (27.71 KB, application/vnd.mozilla.xul+xml)
2003-02-10 08:55 PST, Alex Vincent [:WeirdAl]
no flags Details
patch, v6 (68.50 KB, text/plain)
2003-02-17 11:13 PST, Alex Vincent [:WeirdAl]
no flags Details
patch, v7 (93.40 KB, text/plain)
2003-02-24 18:12 PST, Alex Vincent [:WeirdAl]
no flags Details
patch, v7.1 (97.57 KB, patch)
2003-02-25 16:52 PST, Alex Vincent [:WeirdAl]
no flags Details | Diff | Splinter Review
Demo of a much simpler UI (run from chrome) (2.21 KB, application/vnd.mozilla.xul+xml)
2004-06-12 13:08 PDT, Alex Vincent [:WeirdAl]
no flags Details
Advanced demo of create content widget (6.38 KB, application/vnd.mozilla.xul+xml)
2005-01-20 00:40 PST, Alex Vincent [:WeirdAl]
no flags Details
Patch for element and text node creation. (28.35 KB, patch)
2007-02-04 10:43 PST, Dave Townsend [:mossop]
neil: superreview-
Details | Diff | Splinter Review
patch rev 2 (31.41 KB, patch)
2007-02-06 13:37 PST, Dave Townsend [:mossop]
no flags Details | Diff | Splinter Review
Screenshot of current behaviour (51.18 KB, image/jpeg)
2007-02-06 13:39 PST, Dave Townsend [:mossop]
no flags Details
patch rev 3 (31.36 KB, patch)
2007-02-11 12:16 PST, Dave Townsend [:mossop]
db48x: review+
neil: superreview+
Details | Diff | Splinter Review

Description Alex Vincent [:WeirdAl] 2001-11-29 23:19:41 PST
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.
Comment 1 Alex Vincent [:WeirdAl] 2002-09-03 19:11:54 PDT
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.
Comment 2 Alex Vincent [:WeirdAl] 2002-11-05 19:31:38 PST
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.  
Comment 3 Alex Vincent [:WeirdAl] 2002-11-07 17:12:59 PST
Created attachment 105515 [details]
patch modifying files, plus two new files not in the patch

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.
Comment 4 Alex Vincent [:WeirdAl] 2002-11-07 17:38:05 PST
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.
Comment 5 Alex Vincent [:WeirdAl] 2002-11-09 10:59:39 PST
Created attachment 105708 [details] [diff] [review]
Patch, including new files and lookupNamespaceURI()

I'm not sure if I formatted the --- and +++ lines correctly, but otherwise it
is a working patch.
Comment 6 Alex Vincent [:WeirdAl] 2002-11-09 12:16:38 PST
Created attachment 105714 [details] [diff] [review]
patch v3, following suggestions of timeless and caillon

Hopefully this one is better.
Comment 7 Alex Vincent [:WeirdAl] 2002-11-09 17:12:27 PST
Created attachment 105730 [details] [diff] [review]
patch, v4, with the correct license this time

Thanks, timeless.
Comment 8 Christopher Aillon (sabbatical, not receiving bugmail) 2002-11-15 14:32:02 PST
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...
Comment 9 Alex Vincent [:WeirdAl] 2002-11-15 19:57:05 PST
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 Christopher Aillon (sabbatical, not receiving bugmail) 2002-11-15 20:55:40 PST
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.
Comment 11 Alex Vincent [:WeirdAl] 2002-11-18 18:01:40 PST
Created attachment 106744 [details] [diff] [review]
patch, v5

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.
Comment 12 Alex Vincent [:WeirdAl] 2002-12-07 12:21:10 PST
Tree frozen for 1.3a.  It'd be nice if I could get a r=.
Comment 13 Christian :Biesinger (don't email me, ping me on IRC) 2002-12-07 12:46:16 PST
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 Christian :Biesinger (don't email me, ping me on IRC) 2002-12-07 12:47:50 PST
Comment on attachment 106744 [details] [diff] [review]
patch, v5

requesting from the right address this time (on behalf of Alex Vincent)
Comment 15 Alex Vincent [:WeirdAl] 2002-12-11 16:24:03 PST
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.
Comment 16 Alex Vincent [:WeirdAl] 2002-12-28 15:42:52 PST
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.

Comment 17 Alex Vincent [:WeirdAl] 2003-01-20 19:13:41 PST
Not going to make 1.3 beta, unless we get a miracle.
Comment 18 Alex Vincent [:WeirdAl] 2003-01-27 08:12:18 PST
Created attachment 112754 [details]
Demo of proposed namespaces feature (work in progress)

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).
Comment 19 Alex Vincent [:WeirdAl] 2003-02-10 08:48:57 PST
Created attachment 114021 [details]
Demo, v2 (CSS stylesheet)
Comment 20 Alex Vincent [:WeirdAl] 2003-02-10 08:50:43 PST
Created attachment 114022 [details]
Real CSS stylesheet for demo

The preceding attachment is the XUL document -- one second while I fix it...
Comment 21 Alex Vincent [:WeirdAl] 2003-02-10 08:55:33 PST
Created attachment 114023 [details]
Demo, v2 (XUL document standalone)

Much-improved over original version.  Looking for feedback, particularly on
whether you'd appreciate using something like this in the create nodes widget.
Comment 22 Alex Vincent [:WeirdAl] 2003-02-10 17:07:04 PST
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.
Comment 23 Alex Vincent [:WeirdAl] 2003-02-14 16:21:11 PST
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?
Comment 24 Alex Vincent [:WeirdAl] 2003-02-14 17:46:16 PST
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.
Comment 25 Alex Vincent [:WeirdAl] 2003-02-17 11:13:28 PST
Created attachment 114694 [details]
patch, v6

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
Comment 26 Alex Vincent [:WeirdAl] 2003-02-19 08:06:39 PST
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;
  }
Comment 27 Alex Vincent [:WeirdAl] 2003-02-24 18:12:36 PST
Created attachment 115449 [details]
patch, v7

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.
Comment 28 Alex Vincent [:WeirdAl] 2003-02-24 18:15:54 PST
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.
Comment 29 Alex Vincent [:WeirdAl] 2003-02-25 11:32:55 PST
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.
Comment 30 Alex Vincent [:WeirdAl] 2003-02-25 15:36:00 PST
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.
Comment 31 Alex Vincent [:WeirdAl] 2003-02-25 15:43:55 PST
Comment on attachment 115449 [details]
patch, v7

patch is broken; blame the floppy.
Comment 32 Alex Vincent [:WeirdAl] 2003-02-25 16:52:33 PST
Created attachment 115578 [details] [diff] [review]
patch, v7.1

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.
Comment 33 Alex Vincent [:WeirdAl] 2003-03-11 19:19:35 PST
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.
Comment 34 Alex Vincent [:WeirdAl] 2003-03-11 19:26:13 PST
Comment on attachment 115578 [details] [diff] [review]
patch, v7.1

Correction:  after checking with caillon, sr= requested from bz.
Comment 35 Boris Zbarsky [:bz] 2003-03-11 21:45:19 PST
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....
Comment 36 Jonas Sicking (:sicking) No longer reading bugmail consistently 2003-03-12 07:37:40 PST
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)
Comment 37 Alex Vincent [:WeirdAl] 2003-03-12 10:27:56 PST
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?
Comment 38 Boris Zbarsky [:bz] 2003-03-12 10:31:27 PST
Um... peterv?  dmose?  jst?  heikki?  jag?  hewitt? (depends on what the patch
mostly does).
Comment 39 Alex Vincent [:WeirdAl] 2003-03-12 11:03:37 PST
Comment on attachment 115578 [details] [diff] [review]
patch, v7.1

per e-mail with bz.
Comment 40 jag (Peter Annema) 2003-03-12 15:51:39 PST
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?
Comment 41 Alex Vincent [:WeirdAl] 2003-03-12 16:02:48 PST
:(
Comment 42 Alex Vincent [:WeirdAl] 2003-07-09 11:43:46 PDT
Comment on attachment 115578 [details] [diff] [review]
patch, v7.1

patch will need localization.
Comment 43 Alex Vincent [:WeirdAl] 2004-05-22 15:08:30 PDT
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.
Comment 44 Alex Vincent [:WeirdAl] 2004-06-12 13:08:22 PDT
Created attachment 150613 [details]
Demo of a much simpler UI (run from chrome)

Something like this will be far easier for the DOM Inspector user to use, and
of course to maintain in the future.
Comment 45 Alex Vincent [:WeirdAl] 2005-01-20 00:40:42 PST
Created attachment 171854 [details]
Advanced demo of create content widget

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.
Comment 46 Alex Vincent [:WeirdAl] 2006-06-30 23:16:45 PDT
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.
Comment 47 Dave Townsend [:mossop] 2007-02-04 10:43:50 PST
Created attachment 253955 [details] [diff] [review]
Patch for element and text node creation.

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.
Comment 48 Daniel Brooks [:db48x] 2007-02-04 12:08:20 PST
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 Shawn Wilsher :sdwilsh 2007-02-04 13:11:07 PST
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 neil@parkwaycc.co.uk 2007-02-05 04:21:33 PST
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 neil@parkwaycc.co.uk 2007-02-05 11:47:38 PST
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?
Comment 52 Dave Townsend [:mossop] 2007-02-05 17:08:51 PST
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 neil@parkwaycc.co.uk 2007-02-06 04:03:54 PST
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.
Comment 54 Dave Townsend [:mossop] 2007-02-06 13:37:49 PST
Created attachment 254198 [details] [diff] [review]
patch rev 2

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
Comment 55 Dave Townsend [:mossop] 2007-02-06 13:39:23 PST
Created attachment 254199 [details]
Screenshot of current behaviour
Comment 56 Shawn Wilsher :sdwilsh 2007-02-06 13:43:12 PST
(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
Comment 57 Dave Townsend [:mossop] 2007-02-06 13:46:35 PST
Forgot to add there's also a minor typo from domNodeDialog.xul corrected in this patch.
Comment 58 neil@parkwaycc.co.uk 2007-02-06 16:36:46 PST
(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 Michael Gall 2007-02-06 19:30:53 PST
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 Shawn Wilsher :sdwilsh 2007-02-06 19:46:45 PST
(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.
Comment 61 Alex Vincent [:WeirdAl] 2007-02-06 19:49:46 PST
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 Shawn Wilsher :sdwilsh 2007-02-06 19:53:26 PST
(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...
Comment 63 Dave Townsend [:mossop] 2007-02-11 12:16:00 PST
Created attachment 254748 [details] [diff] [review]
patch rev 3

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.
Comment 64 neil@parkwaycc.co.uk 2007-02-12 08:42:25 PST
(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 neil@parkwaycc.co.uk 2007-02-12 09:19:35 PST
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.
Comment 66 Daniel Brooks [:db48x] 2007-02-12 12:37:11 PST
(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 Daniel Brooks [:db48x] 2007-03-18 08:56:35 PDT
Comment on attachment 254748 [details] [diff] [review]
patch rev 3

awesome. sorry the review took so long. r=db48x
Comment 68 Daniel Brooks [:db48x] 2007-03-18 10:31:41 PDT
checked in

Note You need to log in before you can comment on or make changes to this bug.