280496 - when element is not in document then innerHTML works incorrect

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[alexander :surkov (:asurkov)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a5) Gecko/20041122
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a5) Gecko/20041122

I create html element and set 'innerHTML' property for it. Then I add created
element to document. Parsed elements from 'innerHTML' propery aren't recognized
as html elements.

Reproducible: Always

Steps to Reproduce:
var value="<span style='background-color: red'>44</span><button>hello</button>";
var elm=document.createElementNS("http://www.w3.org/1999/xhtml", "p");
var p=document.getElementById('p');

elm.innerHTML=value;
p.appendChild(elm);

Comment 1

[alexander :surkov (:asurkov)]

Attachment: testcase

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

let me note that none of the elements you created with innerHTML ended up as
HTML elements. the first ones in your testcase are XUL ones, the second ones are
in the null namespace. (as DOM Inspector shows)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Yep.  innerHTML pretends the nodes had been parsed in the place where the <p>'s
kids are, and since the <p> doesn't declare a default namespace they end up
being XUL...

The question is, should we push a namespace declaration for the <p> on the stack
here when building the context?  Perhaps we should do that if the node is in a
namespace but has no prefix?

Steve, any thoughts?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Perhaps like so

Comment 5

[Hixie (not reading bugmail)]

I suppose not supporting innerHTML in XML is not an option? :-)

Sounds to me like what we're doing now is correct, making this bug INVALID.

Comment 6

[Boris Zbarsky [:bzbarsky]]

This is a bigger issue than just innerHTML.  The real question is what our
createContextualFragment code (used for copy/paste, eg) should be doing....

Comment 7

[Hixie (not reading bugmail)]

on reading, or on writing?

On reading, we should create a standalone fragment, IMHO.

On writing, we should use the current context. If people mix using source markup
and DOM manipulation, they'll be burned, but I don't see that as a big problem.
I definitely can't see what we can do to make it more correct -- any "fixup" we
do on parsing is going to break someone. Best make it obvious and easy to
understand.

Comment 8

[Boris Zbarsky [:bzbarsky]]

There is no "reading".  createContextualFragment takes a string and returns a
DOMFragment.

> On writing, we should use the current context.

Sure, but what _is_ the "current context".  That's the question.

> If people mix using source markup and DOM manipulation, they'll be burned, but
> I don't see that as a big problem.

I do, since Editor uses DOM manipulation all over, and copy/paste uses source
markup to move data around.  So the two need to work together in a
semi-reasonable way.

See the patch and comment 3 for what I think we can do better -- treat nodes
with no prefix and a non-null namespace as having a default namespace decl for
their namespace.  This will certainly be true for nodes that actually _have_ a
default namespace decl, and will be true for kids of such nodes that are later
moved to a new place in the document...  Note that when serializing we will
generate a default namespace decl for such nodes as needed, so this is a
reasonable approach.

The other option would be to actually use our DOM serializer to create the
context.  That will insert all the appropriate namespace decls as needed...

Comment 9

[alexander :surkov (:asurkov)]

I was a little wrong. When I posted the bug then I had to formulate summary as
elements without namespace added by innerHTML should have namespace of parent.

boris, do you mean that such elements should have parent namespace or such
elements should be treated as elements with their parent namespace but actually
they should not have parent namespace? If you propose the second variant then I
guess it is more right.

Comment 10

[Boris Zbarsky [:bzbarsky]]

My proposal is that the context for createContextualFragment include a default
namespace declaration for every node that has a non-null namespace and no prefix.

What this does to the child nodes is exactly what would happen when such an XML
fragment is parsed and depends on whether the child nodes have namespace
prefixes, namespace declarations, etc.

Comment 11

[Hixie (not reading bugmail)]

Ok, could you explain the behaviour in exact terms so I can update the spec?
This affects more than just Mozilla.

Comment 12

[Boris Zbarsky [:bzbarsky]]

I don't think we've decided on the behavior in exact terms, actually.  There are
two possibilities, in my mind (when reading these, keep in mind that innnerHTML
is implemented via creating a context, appending the new HTML to it, parsing the
whole thing, then extracting the DocumentFragment corresponding to the new stuff
and inserting it in the document; there is no way to understand some of the
behavior of the function without this basic premise):

1)  (what that patch does):  Walk the chain from the root to the target element.
    For each node in that chain, add the start tag to the context.  Add any
    xmlns or xmlns:foo attributes the node has to the start tag.  If the result
    has no xmlns attribute, the node is in a non-null namespace, and has an
    empty namespace prefix, add an xmlns attribute for the node's namespace to
    the context.
2)  (another possibility):  Create a documentFragment that has exactly the
    clones of the ancestors of the target element (including itself).  Serialize
    this to a string.  Use that as the context.

Variant 2 makes the whole thing dependent on the serializer, of course... our
serializer makes sure that the result, when parsed, will give a DOM as close as
possible to the original one, so it inserts xmlns attributes as needed.  It also
handles cases where DOM manipulation has effectively led the same prefix being
assigned to two different namespaces and fixes one of the prefixes accordingly.
 Note that in the same situation variant 1 will actually produce invalid XML and
the whole thing will fail.

Comment 13

[Hixie (not reading bugmail)]

How about:

   When innerHTML is set, UAs must use the following algorithm (or act as if
   they did):

   Walk the DOM tree from the context element and for each element, take note of
   any prefixes that are defined which have not already been seen (including any 
   that are unbinding a prefix). Thus, get the prefixes that are visible at that
   point. Ignore what prefixes are actually used, only look at those that are 
   declared. Treat the default namespace as a prefix too.

   If the context element has no prefix and is from a namespace different than 
   the default namespace determined from the step above, change the default 
   namespace to be the namespace of the context element.

   Create an element that has all the prefixes that were bound to a namspace in
   the first step above, and that has the default namespace determined above, 
   and parse the string as if that start tag had just been seen and the matching
   end tag is at the end.

   Take the child nodes of that element after parsing, and insert them into the
   element as its children, replacing all existing child nodes.

Comment 14

[Boris Zbarsky [:bzbarsky]]

The part about only taking one element doesn't capture the behavior of innerHTML
with tables...

Also, I'm not sure about the wording of the part about dropping prefixes that
have already been seen.  I suppose that makes some sense, since that would use
the closest place the prefix is declared.  It's not something that would be easy
to implement while reusing existing Gecko code, though...

Comment 15

[Hixie (not reading bugmail)]

> The part about only taking one element doesn't capture the behavior of 
> innerHTML with tables...

Baring in mind that this is only for XML, why not?


> Also, I'm not sure about the wording of the part about dropping prefixes that
> have already been seen.  I suppose that makes some sense, since that would use
> the closest place the prefix is declared.  It's not something that would be 
> easy to implement while reusing existing Gecko code, though...

Well, the alternative is to start at the root and replace prefixes as you find 
them. The end result is the same. Either way, you end up with a list of prefixes 
that are in scope at that element. There's surely got to be a way to get the 
list of in-scope prefixes, no? If not, then this won't be trivial to implement, 
but I still think it make the most sense of the proposals so far.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Why are we speccing it differently for HTML and XML, really?  Given that we are,
I agree the single-element thing makes sense.

> There's surely got to be a way to get the list of in-scope prefixes, no?

There isn't actually.  It's never needed, in practice, except for this case...

Comment 17

[Hixie (not reading bugmail)]

I don't mind if we use the same definition, except that it will always be a
different definition at some point, since the two parsers are (radically)
different. Doesn't the HTML parser depend on earlier sibling context as well,
for instance? Such as knowing if there's an open <p> somewhere or something? I
don't really even want to try to specify how innerHTML is supposed to work in HTML.

But I suppose I'll have to anyway, so we might as well define it.

If you want to just clone all the elements up the tree and have that as the
context, with the last minor fixup for the context element namespace issue, that
works for me too, I guess.

I just need a definition, I don't mind really what it is.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> Doesn't the HTML parser depend on earlier sibling context as well,

In Mozilla, no.

Once we sort out what we're doing with this bug I'll be able to give a precise
definition of the context.

Comment 19

[Johnny Stenback (:jst)]

Comment on attachment 172964 [details] [diff] [review]
Perhaps like so

sr=jst

Comment 20

[steve.swanson]

Comment on attachment 172964 [details] [diff] [review]
Perhaps like so

I think this is the right solution.  Using the DOM Serializer is tempting, but
for now it seems better to keep thing separate.

Comment 21

[steve.swanson]

r=steve.swanson

Sorry it took so long.  Our office has been (and still is) off the net, 
keeping me from testing.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Patch checked in.

Leaving bug open to decide what behavior we really want, though.  sicking,
peterv, thoughts?