Closed Bug 112775 Opened 23 years ago Closed 17 years ago

DOM Inspector to create nodes

Categories

(Other Applications :: DOM Inspector, enhancement)

enhancement
Not set
normal

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+
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.
OS: Windows 98 → All
Hardware: PC → All
Blocks: 109682
Status: NEW → ASSIGNED
Target Milestone: --- → Future
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.
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
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.
Keywords: patch, review
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.
I'm not sure if I formatted the --- and +++ lines correctly, but otherwise it
is a working patch.
Attachment #105515 - Attachment is obsolete: true
Hopefully this one is better.
Attachment #105708 - Attachment is obsolete: true
Thanks, timeless.
Attachment #105714 - Attachment is obsolete: true
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...
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.
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
Attached patch patch, v5 (obsolete) — Splinter Review
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
Target Milestone: Future → mozilla1.3alpha
Tree frozen for 1.3a.  It'd be nice if I could get a r=.
Target Milestone: mozilla1.3alpha → mozilla1.3beta
Attachment #106744 - Flags: review?(caillon)
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 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)
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)
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.

Not going to make 1.3 beta, unless we get a miracle.
Target Milestone: mozilla1.3beta → mozilla1.4alpha
Depends on: 190399
No longer depends on: 190399
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).
Attached file Demo, v2 (CSS stylesheet) (obsolete) —
Attached file Real CSS stylesheet for demo (obsolete) —
The preceding attachment is the XUL document -- one second while I fix it...
Attached file Demo, v2 (XUL document standalone) (obsolete) —
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
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.
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?
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.
Depends on: 193726
Attached file patch, v6 (obsolete) —
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
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
Attached file patch, v7 (obsolete) —
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.
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
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.
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 on attachment 115449 [details]
patch, v7

patch is broken; blame the floppy.
Attachment #115449 - Attachment is obsolete: true
Attachment #115449 - Attachment is patch: false
Attached patch patch, v7.1 (obsolete) — Splinter Review
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 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)
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)
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)
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
Um... peterv?  dmose?  jst?  heikki?  jag?  hewitt? (depends on what the patch
mostly does).
Comment on attachment 115578 [details] [diff] [review]
patch, v7.1

per e-mail with bz.
Attachment #115578 - Flags: superreview?(bzbarsky) → superreview?(jaggernaut)
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?
:(
Target Milestone: mozilla1.4beta → mozilla1.5alpha
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)
Target Milestone: mozilla1.5alpha → mozilla1.6alpha
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.
Something like this will be far easier for the DOM Inspector user to use, and
of course to maintain in the future.
Product: Core → Other Applications
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
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 → ---
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 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 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 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 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?
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 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-
Attached patch patch rev 2 (obsolete) — Splinter Review
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)
Attached image Screenshot of current behaviour (obsolete) —
(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
Forgot to add there's also a minor typo from domNodeDialog.xul corrected in this patch.
(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>
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.
(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.
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.
(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...
Attached patch patch rev 3Splinter Review
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)
(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 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+
(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 on attachment 254748 [details] [diff] [review]
patch rev 3

awesome. sorry the review took so long. r=db48x
Attachment #254748 - Flags: review?(db48x) → review+
Whiteboard: [checkin needed]
checked in
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
QA Contact: timeless → dom-inspector
You need to log in before you can comment on or make changes to this bug.