Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should input/change events be fired when the UA restores a form? #6853

Closed
mfreed7 opened this issue Jul 12, 2021 · 13 comments · Fixed by #7283
Closed

Should input/change events be fired when the UA restores a form? #6853

mfreed7 opened this issue Jul 12, 2021 · 13 comments · Fixed by #7283

Comments

@mfreed7
Copy link
Contributor

mfreed7 commented Jul 12, 2021

The existing spec seems to say that input and change events should be fired whenever the UA performs form filling/restoration activities:

When the user agent is to change an input element's value on behalf of the user (e.g. as part of a form prefilling feature), the user agent must queue an element task on the user interaction task source given the input element to first update the value accordingly, then fire an event named input at the input element, with the bubbles and composed attributes initialized to true, then fire an event named change at the input element, with the bubbles attribute initialized to true.

Chromium recently fixed a bug here and implemented this behavior, which landed in M90. We have received only one bug report about the new behavior, so I think it is relatively safe to assume this change was web compatible.

Gecko and WebKit do not currently appear to implement the above event firing behavior.

It would seem to me that we should honor this part of the spec - from a developer's point of view, the value of the input is being changed (on the user's behalf by the UA), so events should be fired to allow the developer to know about that. I've seen many site bugs caused by UA-filled inputs not being "noticed" by the site. E.g. by password fillers. But thoughts appreciated.

@mfreed7 mfreed7 added the agenda+ To be discussed at a triage meeting label Jul 12, 2021
@annevk
Copy link
Member

annevk commented Jul 21, 2021

This seems reasonable to me. It would be great if we had some autofill infrastructure in web-platform-tests.

cc @whatwg/forms

@tkent-google
Copy link
Contributor

I have seen real sites in which 'change' event handlers trigger navigation. We might need to disallow navigation while UA is restoring values.

e.g.

<input type=file onchange="this.form.submit()">

<select onchange="location.href = this.value"><option value=foo.html>foo</option>...

It seems the Chrome change changed the behavior for some input types, excluding <input type=file> and <select>.

@domenic
Copy link
Member

domenic commented Jul 21, 2021

Not restoring file inputs seems reasonable to me. But select seems like it should be restored, if we can get away with it...

@domenic
Copy link
Member

domenic commented Jul 21, 2021

However upon a moment's further consideration I think there's just no way restoring select would be web-compatible without some mitigation of the type @tkent-google discusses. Those kind of dropdown-to-navigate patterns are too prevalent. Hmm.

@mfreed7
Copy link
Contributor Author

mfreed7 commented Aug 31, 2021

So this seems like people generally agree with the existing spec language. I'm thinking I should just file browser bugs. Does that make sense to people?

@domenic
Copy link
Member

domenic commented Aug 31, 2021

I think we're scared that the existing spec language might not be web-compatible for select and file inputs.

@kohler
Copy link

kohler commented Aug 31, 2021

As a web developer, I'm currently having difficulty adapting to Chrome's new change event behavior, and would like to advocate disallowing Chrome's new behavior. That is, input/change events should not be fired when the UA restores a form.

The current spec language speaks of change and input events as being generated by the user. It explicitly says they are not generated by scripts. Restoring a form after navigation is arguably more like running a script than like the user typing stuff into inputs.

Example problem: Web forms commonly trigger back-end actions on change events. Clicking a box or entering text in an input might automatically submit an XHR to a remote server. After Chrome's update, navigating back to a form can cause a cascade of XHRs as the user’s changes are all re-applied. These redundant XHRs may take a while to crop up as problems or to be fully diagnosed. But they are problematic—for instance, a redundant XHR might overwrite a change that was made by another user.

@mfreed7
Copy link
Contributor Author

mfreed7 commented Aug 31, 2021

Thanks for adding the comment here. Also thanks for the comment you just made on the Chromium bug. I actually wanted to copy part of that response here, because it has a nuance that is interesting to me.

So the spec currently says this:

When the user agent is to change an input element's value on behalf of the user (e.g. as part of a form prefilling feature), the user agent must queue an element task on the user interaction task source given the input element to first update the value accordingly, then fire an event named input at the input element, with the bubbles and composed attributes initialized to true, then fire an event named change at the input element, with the bubbles attribute initialized to true.

And you said:

That's one read. You could also emphasize “changes”. In form pre-filling, from the user's point of view, the control changes on their behalf, and a change event seems appropriate. In navigation, the control preserves its prior state: the relevant change event has already fired. The spec is clear that change events are fired when the control changes value. In navigation, the control is not changing value from the user's POV (it is just changing because of how navigation is implemented in this browser). And when interpreting the spec, shouldn't one put weight on currently implemented behavior, which was the same for all three browsers for many years before April?

The point you make is that from the user's point of view, nothing is changing, they're just "going back" to the past page, with form values (and everything else!) intact. I can certainly see that. And in fact that made me revisit the original bug that triggered my change to Chromium. In that case, there are some other elements on the page that "respond" to changes in the inputs. And the bug was that those other elements don't get properly "restored" when a tab is duplicated or a back/forward navigation happens. But the interesting thing there is that that behavior (not restoring the other elements) is a Chromium-only behavior. Gecko and WebKit both restore the entire page, so there's no need to re-fire the input/change events to re-synchronize.

I adapted this example page to show this. The text values next to the range sliders respond to input/change events and change accordingly, but with a 2 second delay so you can see them. In Chromium, before the delay, you can see the bug - the sliders are restored, but the text does not match. After the events fire, things look good again. But this behavior is not shared by Gecko and WebKit - they both restore the full page, including the modified values, immediately. They have no need, therefore, for the extra events being fired.

I'm beginning to think I should restore Chromium's behavior.

Regardless of that decision, clearly it would be good to at least clarify this part of the spec. We should define (or link to?) what it means to "change" an input element's value.

@mfreed7
Copy link
Contributor Author

mfreed7 commented Sep 1, 2021

One of my action items from the last spec triage meeting was to experiment with the behavior of element restoration across engines. I put together this doc with the results:

https://docs.google.com/document/d/1DsqyuK5NeV_EYQ-2Sa3-OofjDDQL47VIiV6ev9fWvu4/edit?usp=sharing&resourcekey=0-sCyHpHpHlfG3V1XbM8veKA

Apologies for sharing as a Google Doc, but it was a lot easier to build the table. Please address comments back here to this thread, and not in the doc.

TL;DR: there are definitely some differences between the engines for non-form elements (e.g. <span>), file inputs (<input type=file>), and generally around what happens for Back/Forward, Duplicate Tab, and Refresh. My comment above still stands, I believe: I'm thinking Chromium should likely go back to not firing input/change for these navigations.

@past past removed the agenda+ To be discussed at a triage meeting label Sep 3, 2021
blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this issue Sep 4, 2021
After some significant discussion [1][2], we've reached the conclusion
that these changes should be rolled back. The issue is that the spec
says "user agent is to change an input element's value", and in the
case of form restoration on back/forward nav or tab navigation, that
isn't seen by a user as a "change". It is a restoration of pre-existing
state, which shouldn't even be visible to the user.

I left the existing tests, but inverted their logic. I'm not sure
that's hugely valuable, but that's what I did.

[1] whatwg/html#6853
[2] https://crbug.com/1244603
[3] whatwg/html#6853 (comment)

Fixed: 1244603
Bug: 1131234
Change-Id: Ifdd92a6dc473e7d33ee6c28609d730e1903e3fda
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3140573
Commit-Queue: Mason Freed <[email protected]>
Commit-Queue: Joey Arhar <[email protected]>
Auto-Submit: Mason Freed <[email protected]>
Reviewed-by: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#918293}
@mfreed7
Copy link
Contributor Author

mfreed7 commented Oct 11, 2021

Just to quickly update this issue. The conclusion we reached was that not firing events when the UA restores form controls (e.g. during tab duplicate or back/forward nav) is the right thing to do. So on it's face, this issue should be closed.

However, there is still an open action item to update the spec to make this situation more clear. I'll leave this issue open pending that change.

@annevk
Copy link
Member

annevk commented Oct 25, 2021

@mfreed7 @smaug---- hey, @gijsk brought https://bugzilla.mozilla.org/show_bug.cgi?id=654072 to my attention. That indicates Firefox restores the disabled state of controls. It also indicates developers would like to know this restoration has happened in some way. That does suggest to me it might be worth spending more effort here on the full model and perhaps adding an event developers can act upon.

@garygreen
Copy link

garygreen commented Oct 25, 2021

@mfreed7 your document and observations are interesting - have you also considered how browsers handle redirects? I believe both page refresh and submit/back would count as navigate requests? So redirects would be an entirely different mode where restoration occurs?

It seems the general consensus among browsers is that restoration shouldn't occur on page refresh - this makes logical sense because the act of REFRESHING means you want a clean slate. Restoring on page back makes sense, because your restoring an original page state, not refreshing state.

If firefox didn't restore on page refresh I think that would help with most devs grumbles about how it differs in operation to all other browsers

domenic added a commit that referenced this issue Nov 1, 2021
(But, it does fire formStateRestoreCallback, since that is explicitly used for synchronizing the custom element's internal state.)

Closes #6853.
domenic added a commit that referenced this issue Nov 1, 2021
(But, it does fire formStateRestoreCallback, since that is explicitly used for synchronizing the custom element's internal state.)

Closes #6853.
@domenic
Copy link
Member

domenic commented Nov 1, 2021

I've posted #7283 which IMO should close out this issue.

Regarding @annevk's comment in #6853 (comment): I think there's a potential separate issue worth filing around driving better interop on form state restoration during navigation, if implementers desire. Currently the spec is intentionally vague: https://html.spec.whatwg.org/#persisted-user-state-restoration says

Optionally, update entry's persisted user state to reflect any state that the user agent wishes to persist, such as the values of form fields.

and

Optionally, update other aspects of entry's document and its rendering, for instance values of form fields, that the user agent had previously recorded in entry's persisted user state.

which we could try to make more normative if they want. E.g. prohibiting that clause from being used for "reload" navigations.

As for an event, I think pageshow is the event people are looking for?

domenic added a commit that referenced this issue Nov 3, 2021
(But, it does fire formStateRestoreCallback, since that is explicitly used for synchronizing the custom element's internal state.)

Closes #6853.
dandclark pushed a commit to dandclark/html that referenced this issue Dec 4, 2021
(But, it does fire formStateRestoreCallback, since that is explicitly used for synchronizing the custom element's internal state.)

Closes whatwg#6853.
mfreed7 pushed a commit to mfreed7/html that referenced this issue Jun 3, 2022
(But, it does fire formStateRestoreCallback, since that is explicitly used for synchronizing the custom element's internal state.)

Closes whatwg#6853.
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
After some significant discussion [1][2], we've reached the conclusion
that these changes should be rolled back. The issue is that the spec
says "user agent is to change an input element's value", and in the
case of form restoration on back/forward nav or tab navigation, that
isn't seen by a user as a "change". It is a restoration of pre-existing
state, which shouldn't even be visible to the user.

I left the existing tests, but inverted their logic. I'm not sure
that's hugely valuable, but that's what I did.

[1] whatwg/html#6853
[2] https://crbug.com/1244603
[3] whatwg/html#6853 (comment)

Fixed: 1244603
Bug: 1131234
Change-Id: Ifdd92a6dc473e7d33ee6c28609d730e1903e3fda
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3140573
Commit-Queue: Mason Freed <[email protected]>
Commit-Queue: Joey Arhar <[email protected]>
Auto-Submit: Mason Freed <[email protected]>
Reviewed-by: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#918293}
NOKEYCHECK=True
GitOrigin-RevId: 6306a97192a65c7ce96555b7cf74f78a64907bd9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

7 participants