Closed Bug 1606043 Opened 2 years ago Closed 2 years ago

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

Categories

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

71 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla74
Tracking Status
firefox-esr68 --- wontfix
firefox71 --- wontfix
firefox72 --- wontfix
firefox73 --- wontfix
firefox74 --- fixed

People

(Reporter: mozbugzilla2021, Assigned: masayuki)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file)

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
Component: Untriaged → Form Autofill
Product: Firefox → Toolkit

This issue also occurs in Firefox 72 on Windows 10.

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

Status: UNCONFIRMED → NEW
Component: Form Autofill → DOM: Forms
Ever confirmed: true
Flags: needinfo?(masayuki)
Product: Toolkit → Core

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

Flags: needinfo?(masayuki) → needinfo?(bugs)

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.

Flags: needinfo?(bugs)

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"

Has Regression Range: --- → yes
Keywords: regression
Regressed by: 1504911
See Also: → 1584963
Priority: -- → P2

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.

Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Flags: needinfo?(masayuki)

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.

Pushed by masayuki@d-toybox.com:
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

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

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla74
You need to log in before you can comment on or make changes to this bug.