1606043 - Autofill of hidden (display: none) forms gives wrong Form#checkValidity until the next tick

Status: RESOLVED FIXED

(Core :: DOM: Forms, defect, P2)

Description

[Colin S]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:71.0) Gecko/20100101 Firefox/71.0

Steps to reproduce:

1. Visit test case at https://jsfiddle.net/snover/yruhfcxg/show
2. Click on “Details” to open the <details> element
3. Enter some data into the form fields
4. Submit the form
5. Choose to save form data in the browser’s autofill, when requested
6. Reload the test case so the form data is auto filled

Actual results:

Page output:

Input events fired? 2
Form considered valid? false
Valid on the next tick? true

Expected results:

Page output:

Input events fired? 2
Form considered valid? true
Valid on the next tick? true

Comment 1

[Patrick (he/him)]

This issue also occurs in Firefox 72 on Windows 10.

Comment 2

[Matthew N. [:MattN]]

I can reproduce this issue and confirm that if I remove the <details> wrapper then I get the expected result.

Masayuki, is this a known issue with input.setUserInput(…)?

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Matthew N. [:MattN] (PM me if request are blocking you) from comment #2)

I can reproduce this issue and confirm that if I remove the <details> wrapper then I get the expected result.

Masayuki, is this a known issue with input.setUserInput(…)?

I don't know, smaug, do you know?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

I don't know. setUserInput was added for autocomplete/autofill.

https://searchfox.org/mozilla-central/rev/4537228c0a18bc0ebba2eb7f5cbebb6ea9ab211c/dom/html/HTMLInputElement.cpp#2636-2640
looks suspicious, but perhaps this isn't about that.

Comment 5

[Matthew N. [:MattN]]

Last good revision: 8991d660f20e3eea652e060c30e17670b45a9257 (2018-11-20)
First bad revision: fbd97100c83cc07705244725a3245e6d14bbe9cf (2018-11-21)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8991d660f20e3eea652e060c30e17670b45a9257&tochange=fbd97100c83cc07705244725a3245e6d14bbe9cf

I suspect bug 1504911 which caused a similar regression which was already fixed
Bug 1584963 - ValidityState reports invalid patternMismatch for an auto-filled valid "pattern"

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, I see what's going on. HTMLInputElement::OnValueChanged() needs to be called before dispatching input event. It's called only from end of TextControlState::SetValue() if there is no TextEditor. That is not late before since HTMLInputElement::SetUserInput() dispatched input event later. However, currently, TextControlState::SetValueWithoutTextEditor() dispatches input event before that.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1606043 - Make `TextControlState::SetValueWithoutTextEditor()` notify the text control element of value changed before dispatching `eEditorInput` event r=smaug!

I made nsContentUtils::DispatchInputEvent() update validity of event target
before dispatching eEditorInput event in bug 1584963. However, this is not
enough because it updates validity only when the type of <input> element is
mail, but it's required for other types, for example, <input required>.

This patch reverts the hack added in bug 1584963, instead, makes
TextControlState::SetValueWithoutTextEditor() notifies TextControlElement
of value change before dispatching eEditorInput event because it may have made
damage to the performance if nsContentUtils::DispatchInputEvent() updated
all validity state of the event target.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2977fbf09b41
Make `TextControlState::SetValueWithoutTextEditor()` notify the text control element of value changed before dispatching `eEditorInput` event r=smaug

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Could be uplift to beta, but I don't recommend that for risk management.

Comment 10

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/2977fbf09b41