Bug 77391 – Hidden form elements do not save their state prior to form submission

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Jon Lee]

Hidden form elements has a value attribute in mode default, in contrast to most other form elements that work in value mode. Default mode clobbers the strings returned by value() and defaultValue(), which means that the input's state is not saved. When you go back to the page that had the hidden form element, its value is not restored to what it was immediately prior to submission.

<rdar://problem/10563108>

[Jon Lee]

Created attachment 124793 [details]
Patch

[WebKit Review Bot]

Attachment 124793 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/qt/Skipped
CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Source/WebCore/WebCore.exp.in
Auto-merging Source/WebKit/mac/ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit/mac/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

Comment on attachment 124793 [details]
Patch

lol, really that easy huh?

[Jon Lee]

Committed r106388: <http://trac.webkit.org/changeset/106388>

[Kent Tamura]

We stopped saving the value of the hidden type intentionally.  See Bug 23346.

[Brady Eidson]

(In reply to comment #5)
> We stopped saving the value of the hidden type intentionally.  See Bug 23346.

In what bug and/or revision did we actually *stop* saving/restoring form values for hidden elements on back navigation?  I don't remember being part of the conscious decision to do so and probably would've objected.

Bug 23346 has a single r-'ed patch in it.  It's been so long since activity there I don't remember details about the hubbub.  But on the surface it seems to be about fixing the incorrect restoration, and not disabling restoration.

Regardless, this bug was generated by a few very real world sites that are broken since we don't do this for hidden elements, so the fix needs to stand...

[Jon Lee]

(In reply to comment #6)
> Regardless, this bug was generated by a few very real world sites that are broken since we don't do this for hidden elements, so the fix needs to stand...

To that point, with this fix we will be matching Firefox's behavior.

[Kent Tamura]

Please look at Bug 26241 http://trac.webkit.org/changeset/57013 .
Bug 23346 also have some discussion about it.

Type=hidden is often used for session keys, which can be refreshed by page reload, and the current implementation of WebCore often restores form control values to incorrect form controls.  Updating non user-editable controls like type=hidden is dangerous.

I think we should save/restore type=hidden values if
 - The value was updated during user gesture events
 - WebCore restores values to the correct controls.

[Brady Eidson]

(In reply to comment #8)
> Please look at Bug 26241 http://trac.webkit.org/changeset/57013 .
> Bug 23346 also have some discussion about it.
> 
> Type=hidden is often used for session keys, which can be refreshed by page reload, and the current implementation of WebCore often restores form control values to incorrect form controls.  Updating non user-editable controls like type=hidden is dangerous.
> 
> I think we should save/restore type=hidden values if
>  - The value was updated during user gesture events
>  - WebCore restores values to the correct controls.

I think we should save/restore type=hidden values if it is important to big websites, or important types of websites.  And it is:  More than one large bank has been requesting this.

So combining my "should do this" with your "should do this", I think we simply need to fix the bug where we might restore values to the wrong form control.

I'm not nearly as concern that auto generated session keys in hidden form controls are of concern.  Do you know of a real website that would break if we didn't also do the user gesture check?

Jon Lee might know more about the specific cases we've been looking at and can comment on this.

[Kent Tamura]

Jon, what's the requirement for the bank site?
Does saving hidden values updated during UserGestureIndicator::processingUserGesture() solve the bank issue?
Does saving hidden values updated after HTMLInputElement::finishParsingChildren() solve the bank issue?
Does dispatching 'change' events on restoring other field values solve the bank issue?

[Jon Lee]

(In reply to comment #10)
> Jon, what's the requirement for the bank site?
> Does saving hidden values updated during UserGestureIndicator::processingUserGesture() solve the bank issue?
> Does saving hidden values updated after HTMLInputElement::finishParsingChildren() solve the bank issue?
> Does dispatching 'change' events on restoring other field values solve the bank issue?

Sorry for the delay in response.

I actually am not totally sure what the requirement is, because the code that I received exhibiting the bug was only a simplified example. It is also unclear if this is the only situation in which they need hidden form fields to be restored.

But in the provided example, there is a link in an iframe, and upon clicking, it calls a JS function that alters the hidden form field and submit that field's owning form. The expectation is that the field's value remains what it was immediately prior to the submit upon back navigation.

[Darth]

(In reply to comment #11)
> The expectation is that the field's value remains what it was immediately prior to the submit upon back navigation.

Dunno, but isn't that kinda risky? For example in our case (accounting, money related) it caused serious issues. Our form was dynamically generated, so when back was pressed, it re-generated that form with correct values from the back end, but then the browser over wrote those values with what was previously submitted - stale values.

Maybe when a network action happens based on a user gesture, like a form submit, the back and forth navigation should not auto-complete stuff that is not visible to the user? For stuff that is visible, there is at least the visual sanity check that a user can do prior to a re-submit.

Or perhaps one can check the page cache control directive, if its no-store / no-cache, then don't fill the prior values, at least for hidden fields?

[Kent Tamura]

(In reply to comment #11)
> (In reply to comment #10)
> > Jon, what's the requirement for the bank site?
> > Does saving hidden values updated during UserGestureIndicator::processingUserGesture() solve the bank issue?
> > Does saving hidden values updated after HTMLInputElement::finishParsingChildren() solve the bank issue?
> > Does dispatching 'change' events on restoring other field values solve the bank issue?
> 
> Sorry for the delay in response.
> 
> I actually am not totally sure what the requirement is, because the code that I received exhibiting the bug was only a simplified example. It is also unclear if this is the only situation in which they need hidden form fields to be restored.
> 
> But in the provided example, there is a link in an iframe, and upon clicking, it calls a JS function that alters the hidden form field and submit that field's owning form. The expectation is that the field's value remains what it was immediately prior to the submit upon back navigation.

Thank you for the reply.
I think the second one (saving hidden values updated after HTMLInputElement::finishParsingChildren()) is the safest for Bug 88685.

Darth,
FYI. Adding autocomplete=off to a <form> prevents value restoring.

[Darth]

(In reply to comment #13)
> FYI. Adding autocomplete=off to a <form> prevents value restoring.

Yeah, thats what I ended up doing, just that some users liked some stuff getting auto filled.

[Jon Lee]

You should be able to attach autocomplete on the elements you wish to have autocomplete on to override the form's autocomplete bit.