Closed Bug 346337 Opened 15 years ago Closed 13 years ago

Form input field data not retained on session restore

Categories

(Firefox :: Session Restore, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 3.1b1

People

(Reporter: tracy, Assigned: zeniko)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 4 obsolete files)

seen with Bon Echo build 2006072803 on Windows XP


   1. Start Firefox
   2. Load test form : http://bclary.com/projects/form-utils/form.pl?method=post
   3. Note counter, time
   4. Modify inputs  (I put test in each field and add label 3 for the rest of the fields)
   5. Kill Firefox
   6. Start Firefox
   7. Restore Session

tested reuslts:  all of the select, checkbox and radio fields were resest to label 1.  The text fields were randomly restored.  Some not restored

expected results the text fields restored to "test" and the other inputs restored to label 3
was also seeing on ssl test pages.  
That's less of a bug and more of a (quite valid) RFE since complete form data saving's never been implemented. Currently only text data is restored for non-encrypted pages -- and even that only as long as the text fields have an ID or a unique name - which however is mostly the case anyway. Unfortunately I've never managed to come up with a reasonable plan for how to cleanly implement this with decent resource usage...
OS: Windows XP → All
Hardware: PC → All
Duplicate of this bug: 366605
Component: Tabbed Browser → Session Restore
QA Contact: tabbed.browser → session.restore
Duplicate of this bug: 380501
Duplicate of this bug: 415210
Duplicate of this bug: 420475
Attached file Form element test case
I've found that the only form data that SessionStore will save is "text" type input fields and "textarea" elements.  Nothing else is saved.  This includes input element types of checkbox and radio as well as selection elements.

In addition if the text field either has no name or a name with spaces (not valid per W3 specifications) then those elements are not restored either.

The attached test case contains all basic types of form elements.
Blocks: 450465
Attached patch patch (obsolete) — Splinter Review
This patch pretty much replaces all current form data handling code: We now save form data per frame and only access it while we're updating our data collection, anyway. OTOH we store data for all form elements, whether they've been touched or not (thus fixing bug 391814). Our internal data collection will later on be updated whenever a form element is modified (onChange and onInput) or whenever the user navigates around.

Form elements with IDs continue to be accessed through getElementById - all others are now accessed through XPath (which also fixes bug 367387). Since bug 319768 never made it into our main tree (or at least not into mozilla-central), I've ported the required code over to nsSessionStore and made it slightly more flexible (we walk to the form elements from the first parent node with an ID).

Finally, this also fixes the issue with input element names containing spaces, etc., so that the test form in attachment #328502 [details] is now completely restored.
Attachment #334057 - Flags: review?(dietrich)
Attached patch don't save hidden fields (obsolete) — Splinter Review
Mostly the same patch, except that we no longer save the values of hidden fields (which aren't user editable, anyway) and fix an issue with <input>s without having a type attribute set at all (those default to type="text").
Assignee: nobody → zeniko
Attachment #334057 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #334094 - Flags: review?(dietrich)
Attachment #334057 - Flags: review?(dietrich)
Attached patch don't dispatch change events (obsolete) — Splinter Review
When restoring a form element, sometimes the dispatched change events can have unexpected side-effects. I've already several times almost changed bugs to fixed because of the change event to the resolution selection. So, better safe than sorry, I prefer not having those events for the time being.

A second change in this patch is that the generated XPath queries now take <input> names instead of types into account. That should make them slightly more reliable and easier to debug.

Note to self: We're still missing <input type="file">.
Attachment #334094 - Attachment is obsolete: true
Attachment #334156 - Flags: review?(dietrich)
Attachment #334094 - Flags: review?(dietrich)
Comment on attachment 334156 [details] [diff] [review]
don't dispatch change events

>     function restoreTextDataAndScrolling(aContent, aData, aPrefix) {
>       restoreTextData(aContent, aPrefix);
>+      if (aData.formdata)
>+        restoreFormData(aContent.document, aData.formdata);
      if (aData.children && aData.children[i]) {

if aData.formdata exists, is there a reason to try to restore legacy data? wouldn't legacy data no longer be present in the file after the first save w/ this patch?

>+var XPathHelper = {
>+  // these two hashes should be kept in sync
>+  namespaceURIs:     { "xhtml": "http://www.w3.org/1999/xhtml" },
>+  namespacePrefixes: { "http://www.w3.org/1999/xhtml": "xhtml" },
>+

this should probably be a js module, so others may use it.
(In reply to comment #11)
> if aData.formdata exists, is there a reason to try to restore legacy data?

I was considering extension authors used to the old serialization. But then again, they really should migrate anyway... I'll change that.

> >+var XPathHelper = {
> 
> this should probably be a js module, so others may use it.

This code is currently quite specific to this use case (as in: we don't care if a form has moved in the DOM as long as the IDs and names still match; as opposed to: I want this node only if it hasn't moved at all). Generalizing it into a JSM won't thus be as straight-forward as in the JSON case.

So I'd really prefer this to go in for now (also considering that a JSM might not make Firefox 3.1 while this patch should) and then discuss a possible generalization later - although AFAICT we've currently got no other code in /browser or /toolkit generating XPath queries for given nodes at all, so it wouldn't be clear what use cases to generalize for...
Version: 2.0 Branch → Trunk
Blocks: 451627
Blocks: 345135
Attached patch including tests (obsolete) — Splinter Review
... and finally also restoring <input type="file">.
Attachment #334156 - Attachment is obsolete: true
Attachment #335175 - Flags: review?(dietrich)
Attachment #334156 - Flags: review?(dietrich)
Attachment #335175 - Flags: review?(dietrich) → review+
Comment on attachment 335175 [details] [diff] [review]
including tests

r=me, thanks
Attached patch for check-inSplinter Review
Attachment #335175 - Attachment is obsolete: true
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/5f754cb1370f
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.1b1
Duplicate of this bug: 367387
Blocks: 426045
Blocks: 454908
Blocks: 456342
No longer blocks: 456342
Depends on: 456342
Depends on: 477564
Right now we have a Litmus test which checks the form data restoration after a crash. This is https://litmus.mozilla.org/show_test.cgi?id=6224

Simon, what does the automated test cover? Do we still need the Litmus test?
Flags: in-litmus+
The automated test covers saving and restoring form data when closing/duplicating tabs. These are however the same code paths as used for the crash case, so we should be fine without the Litmus test.
Thanks Simon. For now I have disabled the test. I'll mark it as in-litmus? so we can find a final decision when reworking this subgroup.

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090421 Shiretoko/3.5b4pre ID:20090421030848
Status: RESOLVED → VERIFIED
Flags: in-litmus+ → in-litmus?
in-litmus+
See tests 6224 to 6229.  They all check Form restore.
Flags: in-litmus? → in-litmus+
Is there an implemented throw for the case of a target page having a changed layout since last load? Especially pages with incomplete or missing IDs on form elements. If a page without IDs is 'fixed' so that every field has a solid ID, or a single field added, removed, or altered (in name or type), how does XPath cope with this?

Disregarding speed, is there any fail on accuracy when XPath re-walks a form without explicit ID? Is it possible for field data to be displaced into a different field on restore?
I am able to reproduce this issue using Mozilla/5.0 (Windows NT 6.1; rv:2.0b9pre) Gecko/20110109 Firefox/4.0b9pre.
(In reply to comment #23)
> I am able to reproduce this issue using Mozilla/5.0 (Windows NT 6.1;
> rv:2.0b9pre) Gecko/20110109 Firefox/4.0b9pre.

If that's the case, please file a new bug with as much information as possible. This bug was fixed long ago.
You need to log in before you can comment on or make changes to this bug.