Node type and namespace fields get in the way when using the Insert Node dialog

RESOLVED FIXED

Status

Other Applications
DOM Inspector
--
enhancement
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: crussell, Assigned: crussell)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Created attachment 501497 [details] [diff] [review]
Default to deriving the namespace from originalNode for Insert; remember namespaces for Custom; allow namespaces for "text/html"

In some bug, Neil made mention (which I can no longer find) of the namespace field in the Insert Dialog defaulting to something reasonable.

Additionally, I've found that my use case for the Insert Node dialog is overwhelmingly to insert a node which is:
  i) an element, and
 ii) in the same namespace as the reference node.

I think that it's probably true for most others as well.  The dialog should default to inserting an element and focus the name field, so when the dialog appears you will be able to type the name and press enter to handle almost all use cases.  Inserting a text node is so rare that when that's what you really want to do, you can tab to the Node Type menulist or click on it and select the appropriate item.

This patch also removes the restriction on specifying the namespace when the document content-type is "text/html" because:
  i) I couldn't work out how the affected code in initialize was supposed to deal with enableNamespaces being false, and
 ii) it seems that disallowing the namespace for "text/html" is in error.

A side effect of the changes in this patch is that the Custom namespace menuitem also now remembers the textbox value, so if you select another menuitem after typing a custom namespace in the textbox, then reselect the Custom menuitem, the textbox value is reset to what you had originally typed.
Attachment #501497 - Flags: review?(neil)

Comment 1

7 years ago
(In reply to comment #0)
> This patch also removes the restriction on specifying the namespace when the
> document content-type is "text/html" because:
>   i) I couldn't work out how the affected code in initialize was supposed to
> deal with enableNamespaces being false, and
>  ii) it seems that disallowing the namespace for "text/html" is in error.
Well, I believe that it makes no difference any more, but I think there was some edge case between createElement and createElementNS at some point.
This all was added in bug 205872 (which may be the first Mozilla bug I ever fixed...).  Might be history there that is worth reading.
It looks like the patches for that bug are about the DOM Nodes viewer.
(In reply to comment #3)
> It looks like the patches for that bug are about the DOM Nodes viewer.
I think I misread this bug.  My bad!

Comment 5

7 years ago
(In reply to comment #1)
> (In reply to comment #0)
> > This patch also removes the restriction on specifying the namespace when the
> > document content-type is "text/html" because:
> >   i) I couldn't work out how the affected code in initialize was supposed to
> > deal with enableNamespaces being false, and
> >  ii) it seems that disallowing the namespace for "text/html" is in error.
> Well, I believe that it makes no difference any more, but I think there was
> some edge case between createElement and createElementNS at some point.
[Note: I don't know when the switch occurred, but my understanding is that DOMi supports both Gecko 1.9.1 and Gecko 2 so that suffices for this purpose.]

The problem with Gecko 1.9.1 is that HTML elements have a blank namespaceURI but you can't call createElementNS with a blank namespaceURI because you get a non-namespaced element rather than an (X)HTML element.

And even with Gecko 2 you have to be careful because although all (X)HTML nodes have a namespaceURI, createElement creates a case-insensitive HTML element but createElementNS creates a case-sensitive XHTML element.

[This discussion only applies to text/html documents of course; XML (XHTML, XUL, XBL, SVG etc.) documents can only contain XHTML elements.]

Comment 6

7 years ago
Comment on attachment 501497 [details] [diff] [review]
Default to deriving the namespace from originalNode for Insert; remember namespaces for Custom; allow namespaces for "text/html"

>+    var menulist = this.menulist;
>+    var uri = this.mData.namespaceURI;
>+    if (uri) {
>+      menulist.value = uri;
>+      if (!menulist.selectedItem) {
>+        this.customNS.value = uri;
>+        menulist.selectedItem = this.customNS;
>+      }
>+    }
IMHO it would be better to write this as
this.menulist.value = this.customNS.value = uri;

Comment 7

7 years ago
Comment on attachment 501497 [details] [diff] [review]
Default to deriving the namespace from originalNode for Insert; remember namespaces for Custom; allow namespaces for "text/html"

r- as per comment #5 for not supporting createElement.
Attachment #501497 - Flags: review?(neil) → review-
Created attachment 516989 [details] [diff] [review]
Don't eschew with createElement for "text/html"

(In reply to comment #6)
> Comment on attachment 501497 [details] [diff] [review]
> Default to deriving the namespace from originalNode for Insert; remember
> namespaces for Custom; allow namespaces for "text/html"
> 
> >+    var menulist = this.menulist;
> >+    var uri = this.mData.namespaceURI;
> >+    if (uri) {
> >+      menulist.value = uri;
> >+      if (!menulist.selectedItem) {
> >+        this.customNS.value = uri;
> >+        menulist.selectedItem = this.customNS;
> >+      }
> >+    }
> IMHO it would be better to write this as
> this.menulist.value = this.customNS.value = uri;

The problem there is that it behaves in a weird way when the namespace matches one of the items in the menulist.
Attachment #501497 - Attachment is obsolete: true
Attachment #516989 - Flags: review?(neil)

Comment 9

7 years ago
(In reply to comment #8)
> (In reply to comment #6)
> > (From update of attachment 501497 [details] [diff] [review])
> > >+    var menulist = this.menulist;
> > >+    var uri = this.mData.namespaceURI;
> > >+    if (uri) {
> > >+      menulist.value = uri;
> > >+      if (!menulist.selectedItem) {
> > >+        this.customNS.value = uri;
> > >+        menulist.selectedItem = this.customNS;
> > >+      }
> > >+    }
> > IMHO it would be better to write this as
> > this.menulist.value = this.customNS.value = uri;
> The problem there is that it behaves in a weird way when the namespace matches
> one of the items in the menulist.
Could you be more specific? I didn't notice anything weird when I tried it.
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #6)
> > > (From update of attachment 501497 [details] [diff] [review])
> > > >+    var menulist = this.menulist;
> > > >+    var uri = this.mData.namespaceURI;
> > > >+    if (uri) {
> > > >+      menulist.value = uri;
> > > >+      if (!menulist.selectedItem) {
> > > >+        this.customNS.value = uri;
> > > >+        menulist.selectedItem = this.customNS;
> > > >+      }
> > > >+    }
> > > IMHO it would be better to write this as
> > > this.menulist.value = this.customNS.value = uri;
> > The problem there is that it behaves in a weird way when the namespace matches
> > one of the items in the menulist.
> Could you be more specific? I didn't notice anything weird when I tried it.

From an XHTML node, set the menulist to something like XMLNS.  Now switch it to Custom.  The contents of the namespace textbox reverts to the XHTML namespace.  That's weird to me.  I'd expect the textbox value to either a) not change at all except for being enabled, or b) be the empty string.
(In reply to comment #10)
> That's weird to me.  I'd expect the textbox value to either a) not change at
> all except for being enabled, or b) be the empty string.
Fair enough. Personally I thought that the parent node's name space was a reasonable default for the custom name space.

Updated

7 years ago
Attachment #516989 - Flags: review?(neil) → review+
Blocks: 635356
Pushed: http://hg.mozilla.org/dom-inspector/rev/1ee66cda9147
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.