30916 - Cloned html element wont remember their previous state

Status: VERIFIED FIXED

(Core :: Layout, defect, P3)

Description

[Jean-Francois Ducarroz]

When I clone an HTML element (html:select or html:input), the new element will 
not remember it's previous state when either I hide then show the element or 
when the element is in a tree and the tree scroll. Something wrong with the 
preState. I don't have any problem with the original element.

I will attach a test case.

Comment 1

[Jean-Francois Ducarroz]

this is needed by the new improved addressing widget. I am not able to check it 
in because of this bug!

Comment 2

[Jean-Francois Ducarroz]

Attachment: test case

Comment 3

[Jean-Francois Ducarroz]

how to use the test case:

1) load the xul file into the browser
2) with either the select or the input element section, select an option or type 
some text.
3) pres the corresponding hide button then the show button
4) ==> the element is back with is previous value/selection shown
5) Now, press the clone button. The select/input element is now a clone of the 
previous one
6) repeat step 1) to 3)
7) ==> the select/input element lost its previous value and display instead the 
default value.

Comment 4

[Eric Pollmann]

ducarroz - Is this blocking a PDT+ bug?  If so, should it get beta1/PDT+ status?

Comment 5

[Jean-Francois Ducarroz]

No, I will not check in my addressing widget code for B1 (bug 28677). However, I 
would like to check it in as soon as possible after the tree open for M15.

Comment 6

[Eric Pollmann]

In the DOM spec it says CloneNode copies "all attributes and their values". 
This is a little vague, but I believe the intention here was to copy the content
node and not all of the state that is accessible though the DOM.

In this example, the text input might have an HTML "value" attribute accessible
though the DOM "defaultValue" attribute.  This will be copied by the cloneNode
method.  The text input also has access to the state of the text widget though
the DOM "value" attribute.  This is not copied by cloneNode.

This is to say that if you want the value copied from child to clonedChild, the
right thing to do is to store the value yourself, then set the value on the
clonedChild.

I'll attach a patch that does this for your example...

Comment 7

[Jean-Francois Ducarroz]

I wasn't maybe enough clear in my description but the problem is not to copy the 
current value between the original element and the clone. The problem is that 
when you use a clone element, the prestate mechanism that let the element 
remember is current value when it's frame is destroyed doesn't work.

Comment 8

[rods (gone)]

What I was seeing, was when the new frame was shown RestoreState was never 
called. I guess reasonable question is: Should RestoreState be called? According 
to Eric description of spec, I would say the frame state manager is not 
obligated to call RestoreState on the new newly cloned node with the state from 
the "replaced" node.

Comment 9

[Eric Pollmann]

Yes, I was trying to say what Rod just said.  :)  This is of course my
interpretation of the spec and could be off.  I've asked around and it seems to
be right.  I actually came up with a patch for this, but it didn't work right -
the state was not restored when the newly created node was hidden then shown
again.

Is there a reason why you are using cloneNode and replaceChild?  Why not just
get/set the value of the input directly, and re-use the same input?  This would
probably be more efficient (faster), and would get rid of the flashing to boot.

Comment 10

[Jean-Francois Ducarroz]

I need this for the message compose addressing widget when when create 
dynamically node depending of the number of recipients.

Comment 11

[Eric Pollmann]

After talking with Jean-Francois, I realized that I had mis-understood this
problem.

The problem is that the presstate mechanism doesn't work *at all* for the new
node.  Even after it is created, and a value is manually set, if you hide then
show it, that value is lost.

I'll CC Nisheeth on this (should be back from vacation soon) but take a look at
it in the meantime.  This is definitely a bug and should be fixed.  Thanks for
explaining the situation to me Jean Francois.

Comment 12

[Eric Pollmann]

Debugging this revealed that this is because the content ID for the cloned node
is 0 (no content ID given).  We need to give a content ID to the node when it is
created.  Still looking...

Comment 13

[Eric Pollmann]

Quick hack fix for this bug that shows cloned nodes having a content ID would
fix the problem:

Index: nsGenericHTMLElement.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/html/content/src/nsGenericHTMLElement.cpp,v
retrieving revision 1.147
diff -r1.147 nsGenericHTMLElement.cpp
301a302,304
>   PRUint32 id = 0;
>   this->GetContentID(&id);
>   aDst->SetContentID(id);

I think I'm going to have to pass this one on to Nisheeth as he's the guru in
this area and I'm not too familiar with the mechanism of generating and
assigning content ID's.  The problem seems to be that we generate content ID's
in the content sink, and the content sink goes away after the document has been
parsed.

Vidur suggested a possible solution to this problem, and that is to store the
global 'content id counter' on the document rather than on the content sink, and
that way it would be accessible after the initial document load for dynamically
generated items like these.

Note that in this case the document is nsXULDocument, but we should also
consider the case of the HTML document.

Bumping priority to Blocker as this is blocking the new and vastly faster
message compose address widget from landing.

Comment 14

[Nisheeth Ranjan]

Accepting bug and setting target milestone to M15...

I've coded up a fix and started off a build.  Will test it tomorrow and should 
have it checked in by tomorrow afternoon.

Comment 15

[Jean-Francois Ducarroz]

Thanks for taking care of this so quickly.

Comment 16

[Jean-Francois Ducarroz]

what's the current status? did you check in the fix?

Comment 17

[Nisheeth Ranjan]

The fix just got checked in.  Sorry for the delay.

Comment 18

[Chris Petersen]

Marking verified per last comments.