39919 - DOM_L2: Does the editor rely on old incorrect DOM Level 1 behavior

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P1)

Description

[Johnny Stenback (:jst)]

This bug is for tracking if and how the editor breaks when the old incorrect
way of manipulating a DOM document containing XML namespace elements
in mozilla is replaced with the new correct DOM Level 2 way of dealing with
elements in a XML namespace aware application.

The old incorrect implementation allows for code like this to work:

  document.createElement("html:input");
  document.getElementsByTagName("html:checkbox");
  element.setAttribute("rdf:resource", "http://...");
  element.getAttribute("rdf:resource");
  if (element.tagName == "BODY") ... // element is a HTML element in XUL
  if (element.nodeName == "TD") ... // element is a HTML element in XUL

The correct new DOM Level 2 way of doing the above would be something like this:

  document.createElementNS("http://www.w3.org/TR/REC-html40", "html:input");
  document.getElementsByTagNameNS("http://www.w3.org/TR/REC-html40",
                                  "html:checkbox");
  element.setAttributeNS("http://www.w3.org/1999/02/22-rdf-syntax-ns#",
                         "rdf:resource", "http://...");
  element.getAttributeNS("http://www.w3.org/1999/02/22-rdf-syntax-ns#",
                         "rdf:resource");
  if (element.localName == "BODY" && // element is a HTML element in XUL
      element.namespaceURI == "http://www.w3.org/TR/REC-html40") ...
  if (element.localName == "TD" && // element is a HTML element in XUL
      element.namespaceURI == "http://www.w3.org/TR/REC-html40") ...

One of the perhaps biggest changes, and the thing that is probably hardest to
find, is that .nodeName and .tagName on elements, and .name on attribute nodes,
contains the qualified name (ie "html:TD") in DOM Level 2 if element has a
"html" prefix in the file, or if it was dynamically created with a prefix,in the
old incorrect DOM Level 1 implementation .nodeName, .tagName and .name contained
only the "local" name, and no prefix.

For more information about using DOM Level 2 in XML namespace aware
applications, such as the mozilla XUL, see:

  http://www.w3.org/TR/DOM-Level-2/core.html

I'm hoping that I can check in code that lets people enable and disable this new 
behavior by setting a preference, this should be helpful in checking what relies
on the old behavior, the code I'd like to check in also prints out warning
messages on the console when code that assumes the old incorrect implementation
is executed, this will not catch all places, but it should offer some help.

I suspect that most of the changes are in the JS and XUL files in mozilla, but
there could of course be C++ code that relies on the old DOM code too.

The PDT team is aware of this change and a carpool will be arranged where the
DOM Level 2 updates will be turend on permanently and the changes needed to
fix this bug should be checked in, this bug will be updated once the date for
the carpool is set. Mozilla does currently have most of, if not all, the code
necessay to do the changes, but the old way still works.

Comment 1

[rubydoo123]

assigning to Kin for review

Comment 2

[Johnny Stenback (:jst)]

Argh! My original comment contains two errors! The second argument to the
getXxxNS() methods should be the local name only, not the qualified name, IOW:

document.getElementsByTagNameNS("http://www.w3.org/TR/REC-html40",
"checkbox");
element.getAttributeNS("http://www.w3.org/1999/02/22-rdf-syntax-ns#",
"resource");

is the correct way.

Comment 3

[kinmoz]

Accepting bug. Cc'ing [e-mail].

Comment 4

[leger]

Putting on [nsbeta2+] radar for beta2 fix. 

Comment 5

[rubydoo123]

m16

Comment 6

[rubydoo123]

going to reassign this one to cmanske for review

Comment 7

[Johnny Stenback (:jst)]

The temporary code that lets you turn on the new behavior by setting a pref is
now checked in, to enable the new behavior, add this to all.js

pref("temp.DOMLevel2update.enabled", true);

With this pref enabled you'll see output like this:

Possible DOM Error: CreateElement("html:input") called, use CreateElementNS()
in stead!

if the old incorrect behavior is expected. This temporary code does it's best to
issue warnings when possible, but it's not quaranteed to find all the problems
for you.

Comment 8

[Charles Manske]

Kathy: You replaced all instances of CreateElement used to create XUL elements
with CreateElementNS in our JS files, so I don't see anything else that needs to 
be done for this bug, correct?

Comment 9

[Charles Manske]

I don't see anything else to do for this bug

Comment 10

[sujay]

code level fix. marking verified.

Comment 11

[Simon Fraser [no longer active]]

Reopening. With this pref in all.js:

pref("temp.DOMLevel2update.enabled", true);

we get a bunch of warnings about DOM Level 2 stuff.

Comment 12

[Simon Fraser [no longer active]]

The warnings come from:
nsHTMEditor.cpp: IsCellNode(), the call to element->GetTagName().
nsEditor.cpp, IsNodeBlock(), call to element->GetTagName()

and maybe others.

Comment 13

[Charles Manske]

All instances of "GetTagName" and "GetNodeName" in editor code should be 
replaced with "GetLocalName". There are *lots* of them (about 35).

Comment 14

[Johnny Stenback (:jst)]

If you know that the node you're calling GetTagName() or GetNodeName() on is a
HTML element in an HTML document (ie the document that is being edited) there's
no need to change the code, only code that deals with elements that can possibly
originate from XUL, or some other XML document should be modified.

Comment 15

[Charles Manske]

Oh! Thanks for that information -- in that case, *none* of the "GetTagName"
and "GetNodeName" calls need to be changed, since we *never* deal with XUL
elements in the CPP code.
Only the "tagName" instances in the JavaScript code needs to be examined;
and a quick review shows that only the Advanced Properties dialog has such
instances.

Comment 16

[Simon Fraser [no longer active]]

But don't the GetTagName calls return an HTML-scoped tag: "HTML:br"? The code 
certainly needs to be changed to deal with these, if this is the case.

Comment 17

[Charles Manske]

That's what I thought as well -- is JST implying that the "html" isn't prefixed
if it's the default namespace?

Comment 18

[Johnny Stenback (:jst)]

HTML elements in HTML documents do *not* have prefixes unless they're created
dynamically with createElementNS on the document object in a HTML document.

If the elements you're dealing with come from the HTML parser they should
absolutely *not* have a "HTML" prefix, if they do in some case then that is a
bug that needs to be fixed, but you should assume that they don't have prefixes.
HTML elements in HTML documents should not have a namespace associated with the
elements either unless the document is a XHTML document, in which case elements
do have a namespace, but typically not a prefix.

Comment 19

[Mike]

M16 has been out for a while now, these bugs target milestones need to be 
updated.

Comment 20

[Charles Manske]

Someone changed many of our CreateElement() calls in editor to 
CreateElementNS() using  "http://www.w3.org/1999/xhtml" namespace. 
We shouldn't do that. 
Here's the explanation from Johnny:
As long as the document you're editing is a HTML document, created by
parsing a HTML file (which is normally the case AFAIK) then you should
use CreateElement(), you should try not to mix CreateElement() useage
with CreateElementNS() useage (or any other non-NS methods with NS
methods too). The main problem with using CreateElementNS() with the
"http://www.w3.org/1999/xhtml" ns URI (or any ns URI for that matter) is
that the element name is not upper cased as it is in normal HTML
elements (it still is, but expect that to change shortly).

Comment 21

[rubydoo123]

moving over to m17, adjusted priority and severity

Comment 22

[Joe Francis]

wait a minute...
I think Mike made the change to use CreateElementNS().  Here's the deal: when we 
had just CreateElement, then we were somehow getting xml nodes when we tried to 
create BRs inside EnderLites.  So textareas where not happy at all.

EnderLite can be inside a xul document, right?  So we dont know in advance that 
we are always in an html doc.

Mike, Hyatt, can either of you refresh my memory on this?

Comment 23

[Michael La Guardia]

Is this bug still a beta2 blocker?  Removing nsbeta2+ for reconsideration.  What
is the current user visible problem that we are trying to solve here?

Comment 24

[Charles Manske]

MORE BULLSHIT
This was approved and has to be done for nsbeta2.
Do you think arbitrarily removing nsbeta2+ is supposed to magically make us 
ship sooner? No, it will only lower the quality of the product.

Comment 25

[rubydoo123]

added fix by date in status and reassigning to anthonyd

Anthony -- please jump into this one asap -- thanks

Comment 26

[anthonyd]

Ive read the bug report, and the compilation of emails cmanske sent me, and I 
have also talked to mjudge (Mike) about this.  I guess my question is this; if 
CreateElementNS() is not what we are supposed to use, what are we supposed to 
use instead?

Comment 27

[Joe Francis]

I'm not sure about the details of this bug either, but I _think_ that one 
possible fix is to change all of our IsBreak(), IsList(), IsFoo()... to deal with 
elements that have no name space *and* deal with elements that have the html 
namespace.  

In other words, instead of trying to make all our code only generate elements in 
one of the name space possibilities, we could instead make all our code not care 
about the namespace.

Comment 28

[anthonyd]

fixed and checked in.  though i checked in under mjudge since i did the fix on 
his computer.

anthony

Comment 29

[sujay]

marking verified...code level fix.