Bug 31539 - Form Reset : Press Back Button and Reset , TextareaElemnt is not set to Default Value
: Form Reset : Press Back Button and Reset , TextareaElemnt is not set to Defau...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Forms
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P1 Normal
Assigned To: Ben Murdoch
http://spe.mobilephone.net/wit/witroo...
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-11-16 02:45 PST by Amol
Modified: 2010-01-12 03:48 PST (History)
8 users (show)

See Also:


Attachments
Test part 1 (2.57 KB, text/html)
2009-12-01 08:40 PST, Ben Murdoch
no flags Details
Test part 2 - place in resources directory under part 1 (409 bytes, text/html)
2009-12-01 08:41 PST, Ben Murdoch
no flags Details
Proposed patch (1.35 KB, patch)
2009-12-07 11:22 PST, Ben Murdoch
no flags Details | Formatted Diff | Diff
Fix with manual test (3.82 KB, patch)
2010-01-11 09:58 PST, Ben Murdoch
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Amol 2009-11-16 02:45:29 PST
I visited  the formreset
link(http://spe.mobilephone.net/wit/xhtmlv2/formreset.xhtml ).

All Radio button , checkbox , TextArea has some defalut value .
After modifying it and pressing Submit , the data gets submitted , If we press back and Press a Reset Button . The textarea feild is not getting reset to default valu e,

I observer the same behavior in other Browser(Mozilla) , its resets the
same .


Regards ,
 Amol
Comment 1 Alexey Proskuryakov 2009-11-16 17:38:27 PST
I cannot reproduce this with Safari 4.0.3 on Mac OS X. What browser are you seeing this with?
Comment 2 Amol 2009-11-16 21:22:33 PST
Hi Alexey,
Thanks for the reply .
I can see this issue in Google Chrome(3.0.195.33) which i have installed .

I have ported the Webkit Browser code and the same issue i can see in Google android phone .


Thanks and Regards ,
 Amol
Comment 3 Ben Murdoch 2009-11-27 06:45:16 PST
I had a look at this on Android, and the problem seems to be when TextAreaElement::restoreFormControlElementState is called when we navigate back to the page. This resets the *default* value of the element to the state of the form before the submit (using TextAreaElement::setDefaultValue), so when we click reset, the default value to reset to matches the current state rather than the default value stipulated by the HTML.

Setting breakpoints in Safari, it seems restoreFormControlElementState isn't getting called because it's being pulled out of the cache and short circuiting some code, whereas on Android we are triggering a new load. If after submitting the form you clear Safari's cache and then go back, the bug surfaces and you cannot reset the text area to it's default value of 123.

I think the fix therefore is to call setValue rather than setDefaultValue in restoreFormControlElementState. I can send a patch for this.

Thanks, Ben
Comment 4 Ben Murdoch 2009-11-30 03:04:57 PST
I have a patch ready to fix this bug, but before I upload it I've been trying to write a layout test for it. It's tricky because to expose the bug on Safari, you need to clear the cache between submitting the form and hitting back. Is there a way to turn off or clear the cache through the layout test controller?

Thanks, Ben
Comment 5 Alexey Proskuryakov 2009-11-30 08:35:28 PST
Maybe setCacheModel(0) will work?
Comment 6 Ben Murdoch 2009-12-01 08:29:17 PST
I have written  a reduced test case that exposes the bug on Safari which is perfect for the layout test. However, I'm having trouble getting the test to execute automatically. If you open the test page and manually click the 'submit' button on the form in the iframe, then the test will expose the bug. When I try to automate this by programattically clicking the button, the test does not run correctly. I think I've tracked the problem down to the fact that it seems that for events from script that trigger navigation, no history items are created and so the call to history.back() in the form handler fails.

I will attach the test as I have it right now; does any one have any advice as to how I could fully automate it? I've tried using the eventSender object in DRT, calling the submit button's click() method, instantiating an Event object and dispatching it to the submit button, altering location.hash to try and create history entries, calling the submit() function on the form itself, but nothing seems to work...

The fix for the bug is a very simple one line change in HTMLTextAreaElement.cpp.

Thanks, Ben
Comment 7 Ben Murdoch 2009-12-01 08:40:39 PST
Created attachment 44083 [details]
Test part 1

First part of the test
Comment 8 Ben Murdoch 2009-12-01 08:41:56 PST
Created attachment 44084 [details]
Test part 2 - place in resources directory under part 1
Comment 9 Ben Murdoch 2009-12-07 11:22:12 PST
I haven't had any luck getting the test to automate - I wonder if we could consider adding the fix without a test? I have attached a patch...
Comment 10 Ben Murdoch 2009-12-07 11:22:57 PST
Created attachment 44422 [details]
Proposed patch
Comment 11 Darin Adler 2009-12-08 11:46:45 PST
Comment on attachment 44422 [details]
Proposed patch

>  void HTMLTextAreaElement::restoreFormControlState(const String& state)
>  {
> -    setDefaultValue(state);
> +    setValue(state);
>  }

Sure, we want to set the value. But don't we want to set the default value too?
Comment 12 WebKit Review Bot 2009-12-08 11:47:35 PST
style-queue ran check-webkit-style on attachment 44422 [details] without any errors.
Comment 13 Ben Murdoch 2009-12-08 11:54:14 PST
(In reply to comment #11)
> 
> Sure, we want to set the value. But don't we want to set the default value too?

I don't think so -- not to the value that was stored as the state of the text area, anyway?
Comment 14 Maciej Stachowiak 2009-12-29 05:45:50 PST
(In reply to comment #11)
> (From update of attachment 44422 [details])
> >  void HTMLTextAreaElement::restoreFormControlState(const String& state)
> >  {
> > -    setDefaultValue(state);
> > +    setValue(state);
> >  }
> 
> Sure, we want to set the value. But don't we want to set the default value too?

I don't think that's right - when saving form control state, what we saved originally was the value, not the default value. We could save the default value as well (in case it was changed e.g. by altering the <textarea>'s DOM children) but in general our form control state saving code doesn't try to do that type of thing.
Comment 15 Adele Peterson 2010-01-08 23:48:04 PST
How about adding the test as a manual test?
Comment 16 Ben Murdoch 2010-01-11 04:50:57 PST
(In reply to comment #15)
> How about adding the test as a manual test?

That would do it I should think, thanks for the tip! I've never written or used manual tests in WebKit -- is it as one would expect, a layout test that is designed to be run by hand? A quick look at current tests inside WebCore/manual-tests would suggest so and that the body of the test page should include instructions for running the test and the expected results. Are there any other guidelines or tips for writing a manual test? Are these tests run with any regularity?

Thanks, Ben
Comment 17 Ben Murdoch 2010-01-11 09:58:22 PST
Created attachment 46286 [details]
Fix with manual test

Here's the same patch with a manual test.
Comment 18 Ben Murdoch 2010-01-12 02:29:15 PST
Comment on attachment 46286 [details]
Fix with manual test

cq+ for landing.
Comment 19 WebKit Commit Bot 2010-01-12 03:48:12 PST
Comment on attachment 46286 [details]
Fix with manual test

Clearing flags on attachment: 46286

Committed r53132: <http://trac.webkit.org/changeset/53132>
Comment 20 WebKit Commit Bot 2010-01-12 03:48:20 PST
All reviewed patches have been landed.  Closing bug.