Closed Bug 660549 Opened 13 years ago Closed 9 years ago

Moved textarea node loses history connection and content when going forth and back to a page

Categories

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

defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: techtonik, Unassigned)

References

()

Details

Attachments

(1 file)

User-Agent:       Mozilla/5.0 (Windows NT 6.0; rv:5.0) Gecko/20100101 Firefox/5.0
Build Identifier: Mozilla/5.0 (Windows NT 6.0; rv:5.0) Gecko/20100101 Firefox/5.0

Contents of unsubmitted <textarea> elements are meant to be saved if user follows some link on a page and then returns using the Back button. This works ok in FF5 for static forms, but fails if the <textarea> element was moved using JS.

Reproducible: Always

Steps to Reproduce:
See example at http://60.retreview.appspot.com/2/ ::
0. login
1. click Reply link at the bottom of comments thread
2. add some text
3. click any link on a page to go out from the page
4. click back
5. click Reply link again


Actual Results:  
Text entered in 2. is missing.

Expected Results:  
Text needs to persist. In Chrome it works.

Patch that adds this form persistence to dynamically generated form -   http://codereview.appspot.com/4529096
Hmm.  So what does that "Reply" link actually do?

In Gecko, state is saved for form controls based on a key that includes information like the form the control is in.  Then it's restored based on a key generated in the same way.  The saving is done when leaving the page, but the restoration obviously happens when _loading_ the page.

Which means that if "Reply" moves a control from one form to another, say, it won't work in Gecko.  Is that what's going on here?
Exactly.

Reply link handler clones form template, inserts it into DOM, and moves <textarea> that was already present before the form was cloned into the cloned form. On discard, <textarea> is moved back from the form and the form is destroyed.
Yeah, that's just not going to work.  How are we supposed to know that during load this is "the same" textarea that had the text during unload if it's in a totally different place in the DOM?

I can look at the heuristics WebKit is using here, but I bet there are cases where theirs fails and ours works; that's why it's a heuristic...

The way to make this work reliably is to not move around in the DOM things whose state you'd like to be restored across history navigations.
(In reply to comment #3)
> Yeah, that's just not going to work.  How are we supposed to know that
> during load this is "the same" textarea that had the text during unload if
> it's in a totally different place in the DOM?

The <textarea> element is the same. It is just moved to a different place - not cloned or copied. Erasing its id when you move it seems illogical to me.
There is no "erasing its id".  The question is how we tell that some textarea element is one document is "the same" as some other textarea in some _other_ document.  These are completely separate objects that we have to somehow connect to each other.  Again, the value is saved when _leaving_ the previous page (which has a textarea near the end) but needs to be restored when _loading_ the page on history navigation, in some textarea that's near the beginning.  What makes those two textareas related to each other?

Right now we treat a control as "the same" if it has the same type, is in the same form (by index in document.forms), has the same name, and the name of the form is the same.

Since you move the control from one form to another, we think they're different controls.

I just looked at what WebKit does, and they treat controls as "the same" if they have the same name, same type, and come in the same order in the document amongst controls with that name and type.  But the ordering is node construction order, not DOM order at unload.  That might make more sense in some situations like yours, I guess.  It'll break other situations.  Need to think about the tradeoffs.

Jonas, Mounir, thoughts?
Status: UNCONFIRMED → NEW
Ever confirmed: true
I'm not a big fan of using DOM construction order either. It means problems if nodes are created dynamically (such as in repeating templates) or lazily.

Really the page should be using session history or some such.

One possible way we can improve things here and make them more predictable is if we key on the id attribute of the <textarea> if one exists. That way we should be resilient if the node is moved around the document.
Construction order makes object creation behavior consistent with JS where you expect instances of nodes to be like any other objects with identifier as a "memory reference", which is not destroyed when an object is moved to a different programming structure.

For example, if you do:
  form.insertBefore(msgTextarea, form.firstChild);
your msgTextarea variable doesn't lose a reference to DOM node even though it is in a different form now. 

This id can be constructed using whatever combination of type/position/etc. you want when the object is created, but once it is created, the id may not change. This will guarantee that the textarea data is restored regardless of its final position when the page is left.
 
> I just looked at what WebKit does, and they treat controls as "the same" if
> they have the same name, same type, and come in the same order in the
> document amongst controls with that name and type.  But the ordering is node
> construction order, not DOM order at unload.  That might make more sense in
> some situations like yours, I guess.  It'll break other situations.

I'd like to know more about these other situations.
Component: Layout: Form Controls → DOM: Core & HTML
OS: Windows Vista → All
QA Contact: layout.form-controls → general
Hardware: x86 → All
Version: unspecified → Trunk
Jonas, using the id wouldn't really work in the repeated template case very well either, because those often clone everything, including the ID.
Well, a good templating system would fix that. And if people don't use unique ids then the fact that things don't work as well should be no surprise.

But I agree we should definitely handle that case (for example by storing an index into the list of elements with the same id). I just don't think we need to optimize as much for that case.
The other issue with ids is that most controls don't have them, so while they would help with this particular testcase if this textarea has an id they don't necessarily solve the problem in general.
But back to the main issue: I think the fundamental problem we have right now is that we generate the state key when _unloading_ the page to save but when _loading_ the page to restore.  That really does seem fundamentally broken; comment 7's point about the state key being immutable (which it's not in Webkit, btw, if you move nodes back and forth across document boundaries) is spot on.
Blocks: 673646
Blocks: 311507
(In reply to comment #5)
> Right now we treat a control as "the same" if it has the same type, is in
> the same form (by index in document.forms), has the same name, and the name
> of the form is the same.
It seems to need same index in form.elements too. If you have two controls <input name="a"> and <input name="b"> and you delete a, then b no longer gets restored after reload. (Worse, if you have <input id="a"> and <input id="b"> and you delete a, then b's value gets restored to a after reload, since the controls are only persisted by name attribute, even though it's deprecated.)
Blocks: 677430
(In reply to neil@parkwaycc.co.uk from comment #12)
> (Worse, if you have <input id="a"> and <input id="b">

This is bug 677430 now
Blocks: 680893
Depends on: 737851
That fixes the problem; I added a mStateKey to nsGenericHTMLFormElement and the key is now specified explicitely to the save/restore methods.
Attachment #608037 - Flags: review?(mounir)
Comment on attachment 608037 [details] [diff] [review]
Uses a State Key generated at load time when saving field contents

Review of attachment 608037 [details] [diff] [review]:
-----------------------------------------------------------------

As far as I understand it, your patch isn't changing when the state is saved. It's still being done by nsIFormControl::SaveState() which is called from nsGenericHTMLFormElement::UnbindFromTree() and undirectly from nsDocument::RemovedFromDocShell().

I think what you want to do is to call SaveSubtreeState() when the page has loaded. This will basically create the id. Then, if SaveState() is called again, the id will not be computed again but the value will be updated.

Anyhow, you should probably move that patch to bug 737851 and ask Boris for feedback/review. I would be glad to help but I don't really know that code well enough to really review it.

[1] https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#7078

::: content/html/content/src/nsGenericHTMLElement.h
@@ +457,2 @@
>     */
>    static nsresult GetPrimaryPresState(nsGenericHTMLElement* aContent,

If you create a new class for state key management, I believe this method could go inside it (and could even be non-static).

@@ +468,3 @@
>     */
>    static already_AddRefed<nsILayoutHistoryState>
> +  GetLayoutHistory(nsGenericHTMLElement* aContent,

Same here.

@@ +480,4 @@
>     * @return false if RestoreState() was not called, the return
>     *         value of RestoreState() otherwise.
>     */
>    static bool RestoreFormControlState(nsGenericHTMLElement* aContent,

And here.

@@ +994,5 @@
>    nsHTMLFieldSetElement* mFieldSet;
> +
> +  /* Used to store the key to that element in the session. See StateKey. Is
> +     void until StateKey has been used */
> +  nsCString mStateKey;

Could you prevent adding stuff to nsGenericHTMLFormElement? That wolud increase the size of all form controls while it's only needed for a few of them. It's probably not a big deal because the most used would use that |mStateKey| but adding a new class for elements that have a state wouldn't hurt, right?
Attachment #608037 - Flags: review?(mounir)
We don't want to change when the state is saved.  We want to change when the state key is generated.  The state should be saved when unloading the document, because that's when there might be user-edited state to save.
This got fixed by the patch in bug 737851, unsurprisingly.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
No longer blocks: 680893
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: