Closed
Bug 623372
Opened 14 years ago
Closed 14 years ago
Node type and namespace fields get in the way when using the Insert Node dialog
Categories
(Other Applications :: DOM Inspector, enhancement)
Other Applications
DOM Inspector
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: crussell, Assigned: crussell)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
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•14 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.
Comment 2•14 years ago
|
||
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.
Assignee | ||
Comment 3•14 years ago
|
||
It looks like the patches for that bug are about the DOM Nodes viewer.
Comment 4•14 years ago
|
||
(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•14 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•14 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•14 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-
Assignee | ||
Comment 8•14 years ago
|
||
(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•14 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.
Assignee | ||
Comment 10•14 years ago
|
||
(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.
Comment 11•14 years ago
|
||
(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•14 years ago
|
Attachment #516989 -
Flags: review?(neil) → review+
Assignee | ||
Updated•14 years ago
|
Blocks: DOMi2.0.10
Assignee | ||
Comment 12•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•