163714 - Form data is lost on "Back" from error page

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P1)

Description

[Will Budreau]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.1b) Gecko/20020820
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.1b) Gecko/20020820

Completing this form without selecting a component takes user to an error page
where the user is told to select "back."  However, upon selecting "back", the
user returns to this page where DETAILS FIELD HAS BEEN CLEARED.

This may be a browser bug, but the site should be more forgiving.


Reproducible: Sometimes

Steps to Reproduce:
1. Enter the details of a bug on the GUIDED bug form
2. Leave "Component" unselected
3. click "Submit"
4. On error page ("Press back to select component") press Back button
Actual Results:  
After 3, arrive at error page which says:
Error
You must choose a component that corresponds to this bug. If necessary, just guess.
Please press Back and try again. 

However, all fields from form have been emptied!

Expected Results:  
Mozilla should have maintained the fields, but SINCE it doesn't..

The website should be smart enough to know that this kind of thing happens and
not depend on the browser's "back" button to maintain data.  Either use
javascript to error check without leaving the form page, or re-include the form
on the Error page so the user does not lose it.

This was also reported bug 163305 for Windows 2000

Comment 1

[Bradley Baetz (:bbaetz)]

WFM, but browser, not m.o

Comment 2

[Mats Palmgren (inactive)]

*** Bug 163734 has been marked as a duplicate of this bug. ***

Comment 3

[Ray Birks]

This was also reported in PC/WinME in Bug 163734.

Comment 4

[Radha on family leave (not reading bugmail)]

Claudius can you give it a try?

I can't reproduce this. When I clicked back at the error page, I returned to the
guided "Enter  a bug" and my form values and selection list selections were
preserved. Do you have cache enabled?

Comment 5

[Mats Palmgren (inactive)]

Radha, I'm really surprised that can't reproduce this. I see this on a daily
basis when using Bugzilla and it's been going on for a while now. I usually
see it at http://bugzilla.mozilla.org/query.cgi after submitting the query
I go back to narrow the search even more, but the text in some of the text-fields
are now gone. It is *really* annoying beacuse it makes me submit wrong data.
I see this on both Linux and Windows 2000 with recent nightly trunk builds.
It has happened to me on several other sites too, so it's not just Bugzilla.

Comment 6

[Mats Palmgren (inactive)]

*** Bug 166228 has been marked as a duplicate of this bug. ***

Comment 7

[Mats Palmgren (inactive)]

I can easily reproduce bug 166228 as well, 2002-08-30-05 trunk Linux.

Comment 8

[John Keiser (jkeiser)]

This appears to be form controls, specifically textarea and input type=text.  No
other control has a problem.  If you enter text in textfields in the browser and
just keep going back and forward eventually it loses.

Comment 9

[Bradley Baetz (:bbaetz)]

*** Bug 166276 has been marked as a duplicate of this bug. ***

Comment 10

[Bradley Baetz (:bbaetz)]

Its also really easy to reproduce with bugzilla - using quicksearch then going
back does it 99% of the time.

Comment 11

[Andrew Schultz]

Attachment: testcase

the first form (almost) always loses the text, but the second does not.
regression between linux trunk builds 2002081104 and 2002081304

Comment 12

[John Keiser (jkeiser)]

OK, so the cause of this problem appears to be that the text input does not have
a ContentID, so it assumes that it's anonymous content, which can't be
saved/restored.  Why it doesn't get a ContentID when the focus is set on the
element is the next question.

Comment 13

[Steve Chapel]

Isn't this a dup of bug 147322?

Comment 14

[John Keiser (jkeiser)]

Actually, I suspect bug 147322 is fixed.  But no, this is not the same problem.

Comment 15

[John Keiser (jkeiser)]

Attachment: Patch

So DOMSlots clears the ContentID when it gets created.	Always.  This sucks.  I
fixed that, made GetFlags() and SetFlags() not allow you to set content ID bits
(for safety), and removed the unused MaybeClearDOMSlots().

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 98084 [details] [diff] [review]
Patch

- In SetFlags():

Don't change the assert, it's perfectly fine to use this helper to set the
content id bits. And:

     if (slots) {
       slots->mFlags |= aFlagsToSet;
-
       return;
     }

No removing my empty line before return here!

- In UnsetFlags()

Same thing, don't change the assert.

     if (slots) {
       slots->mFlags &= ~aFlagsToUnset;
-
       return;
     }

Same thing here.

- In nsDOMSlots::nsDOMSlots():

-  : mFlags(aFlags), mChildNodes(nsnull), mStyle(nsnull),
mAttributeMap(nsnull),
-    mBindingParent(nsnull), mContentID(0)
+  : mFlags(aFlags & ~GENERIC_ELEMENT_CONTENT_ID_MASK),
+    mChildNodes(nsnull),
+    mStyle(nsnull),
+    mAttributeMap(nsnull),
+    mBindingParent(nsnull),
+    mContentID(aFlags >> GENERIC_ELEMENT_CONTENT_ID_BITS_OFFSET)

Please put these back on as few lines as they fit on w/o overflowing 80
columns.

- In nsGenericElement::GetContentID()

-    PtrBits flags = GetFlags();
-
-    *aID = flags >> GENERIC_ELEMENT_CONTENT_ID_BITS_OFFSET;
+    *aID = mFlagsOrSlots >> GENERIC_ELEMENT_CONTENT_ID_BITS_OFFSET;

I like the old code better, sure, it's a tiny bit more code (but that will most
likely be optimized away by the compiler) but it leaves the dealing with
mFlagsOrSlots hidden in the helper that's written for that purpose, and for
that purpose only.

- In nsGenericElement::SetContentID()

   } else {
-    UnsetFlags(GENERIC_ELEMENT_CONTENT_ID_MASK);
-    SetFlags(aID << GENERIC_ELEMENT_CONTENT_ID_BITS_OFFSET);
+    mFlagsOrSlots &= ~GENERIC_ELEMENT_CONTENT_ID_MASK;
+    mFlagsOrSlots |= (aID << GENERIC_ELEMENT_CONTENT_ID_BITS_OFFSET);

This is not good, please leave the mucking with the mFlagsOrSlots hidden in the
helpers that know to do the right thing (looks like you're doing the right
thing, but nevertheless, use the helpers, that's what they're there for).

sr=jst if you make those changes, then we'll have no references to
mFlagsOrSlots in nsGenericElement.cpp, except for the initialzation in the
ctor. Perfect!

Comment 17

[Asa Dotzler [:asa]]

Who can review this. Time is short.

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 98084 [details] [diff] [review]
Patch

r=[e-mail] with jst's changes

Comment 19

[Asa Dotzler [:asa]]

Comment on attachment 98084 [details] [diff] [review]
Patch

a=asa (on behalf of drivers) for checkin to 1.2a.

What about bug 147322? Is that related? "Text entered into a Form Text area is
lost."

Comment 20

[John Keiser (jkeiser)]

The thing is, ContentID is *not* flags.  I don't think it should be manipulated
in flag functions.  I'll change it back though, since it's your code and we're
going to remove ContentID anyway, so the code will no longer offend :)

Regarding the column thing, I had it that way before, but it looked
ugly--specifically since the first and last initializers were so big that they
were on lines by themselves.  Not sure what you want here.

Comment 21

[John Keiser (jkeiser)]

Attachment: Patch v1.1

Since it's JST's code (not mine) and we're removing ContentID at some point
anyway, I've removed those style changes.  This is the resulting patch for
posterity.  It tests out fine.

Comment 22

[John Keiser (jkeiser)]

Checked in.