Bug 77391

Created attachment 124793 [details] Patch

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.

Comment on attachment 124793 [details] Patch lol, really that easy huh?

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

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

(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...

(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.

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.

(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.

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?

(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.

(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?

(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.

(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.

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