Closed Bug 1504911 Opened 6 years ago Closed 6 years ago

HTMLInputELement::SetUserInput() shouldn't dispatch "input" element by itself if has editor

Categories

(Core :: DOM: Events, enhancement, P2)

Product:
Core
Component:
DOM: Events
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla65
Tracking Flags:
Tracking Status
firefox65 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(6 files)

Bug 1504911 - part 0: Add "input" event tests into existing tests
47 bytes, text/x-phabricator-request
Details | Review
Bug 1504911 - part 1: Make all "input" event dispatcher in C++ use new utility method
47 bytes, text/x-phabricator-request
Details | Review
Bug 1504911 - part 2: Make nsTextEditorState::SetValue() dispatch "input" event if it's called for handling part of user input
47 bytes, text/x-phabricator-request
Details | Review
Bug 1504911 - part 3: Make TextEditRules::WillSetText() not handle anything when EditAction is eReplaceText
47 bytes, text/x-phabricator-request
Details | Review
Bug 1504911 - part 4: Make all script for web content dispatch "input" event with proper event interface
47 bytes, text/x-phabricator-request
Details | Review
Bug 1504911 - part 5: Make HTMLEditor::InsertTableCellsWithTransaction() create AutoPlaceholderBatch and AutoTopLevelEditSubActionNotifier
47 bytes, text/x-phabricator-request
Details | Review
HTMLInputELement::SetUserInput() dispatches "input" event by itself. However, the event class won't be InputEvent and its inputType won't be initialized by editor. So, when HTMLInputElement has editor, editor should dispatch "input" event and HTMLInputElement should do nothing.
Priority: -- → P2
It's difficult to create new test which checks "input" events caused by all edit operations especially when text is inserted from our UI. Therefore, this adds "input" event type checks into existing tests. Additionally, this adds new test for MozEditableElement.setUserInput() whose behavior needs to be fixed in this bug. Currently, InputEvent interface should be used only on text controls or contenteditable editor when dispatching "input" event. https://w3c.github.io/input-events/#events-inputevents You may feel odd to use different event interface for same "input" events. However, other browsers also use InputEvent interface only in the cases. So, we should follow them for now.
Currently, a lot of code dispatch "input" event and some of them dispatch "input" event with wrong interface and/or values. Therefore this patch creates nsContentUtils::DispatchInputEvent() to make all of them dispatch correct event. Unfortunately, due to bug 1506439, we cannot set pointer to refcountable classes of MOZ_CAN_RUN_SCRIPT method to nullptr. Therefore, this patch creates temporary RefPtr<TextEditor> a lot even though it makes damage to the performance if it's in a hot path.
When editor is modified as part of user action, aFlags of nsTextEditorState::SetValue() includes eSetValue_BySetUserInput. In this case, TextEditor (if there is) or the method itself (if there is no editor yet) should dispatch "input" event by themselves because we will need to initialize InputEvents more since we're going to implement Input Event specs. Note that even with this patch, password field stops dispatching "input" event with call of HTMLInputElement::SetUserInput(). This is caused by a hidden bug of TextEditRules. This will be fixed in a following patch.
When all editor text is replaced while handling a user operation, editor needs to dispatch "input" event. Therefore, in such case, i.e., EditAction is eReplaceText, TextEditor::SetTextAsSubAction() needs to handle it instead of TextEditRules::WillSetText().
Currently, some "input" event dispatchers in our script dispatch "input" event with UIEvent. This is completely wrong. For conforming to HTML spec, Event is proper event. Additionally, for conforming to Input Events, InputEvent is proper event only on <textarea> or <input> element which has a single line editor. For making us to maintain easier, this patch adds new API, "isInputEventTarget" to MozEditableElement which returns true when "input" event dispatcher should use InputEvent for the input element. Finally, this makes some dispatchers use setUserInput() instead of setting value and dispatching event by themselves. This also makes us to maintain them easier. Note that this does not touch "input" event dispatchers which dispatch events only for chrome (such as URL bar, some pages in about: scheme) for making this change safer as far as possible.
Currently, calling nsITableEditor.insertTableCell() does not cause dispatching "input" event since it does not create AutoPlaceholderBatch. Additionally, different from InsertTableRowsWithTransaction() and InsertTableColumnsWithTransaction(), it does not create AutoTopLevelEditSubActionNotifier. Because of those APIs should work similartly, we should make it creates both auto class instances.
Comment on attachment 9025998 [details] Bug 1504911 - part 4: Make all script for web content dispatch "input" event with proper event interface This patch touches mobile/android/modules/geckoview/GeckoViewAutoFill.jsm because of fixing its bug. However, according to the file path, this is a part of GeckoView. I don't know how to test this actually nor which automated test covers this code. So, I still don't test it yet. Additionally, it uses an internal API, MozEditableElement.setUserInput(), which is marked as IsChromeOrXBLOrUAWidget. https://searchfox.org/mozilla-central/rev/5117a4c4e29fcf80a627fecf899a62f117368abf/dom/webidl/HTMLInputElement.webidl#198-199 Could you let me know how to test it or an automated test which checks this behavior?
Flags: needinfo?(jimnchen+bmo)
Oh, sorry, I realized that I've already received your email! Thanks!
Flags: needinfo?(jimnchen+bmo)
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #32) > Comment on attachment 9025998 [details] > Bug 1504911 - part 4: Make all script for web content dispatch "input" event > with proper event interface > > This patch touches mobile/android/modules/geckoview/GeckoViewAutoFill.jsm > because of fixing its bug. However, according to the file path, this is a > part of GeckoView. I don't know how to test this actually nor which > automated test covers this code. So, I still don't test it yet. > Additionally, it uses an internal API, MozEditableElement.setUserInput(), > which is marked as IsChromeOrXBLOrUAWidget. > https://searchfox.org/mozilla-central/rev/ > 5117a4c4e29fcf80a627fecf899a62f117368abf/dom/webidl/HTMLInputElement. > webidl#198-199 Well, I added event interface check into AccessibilityTest.kt and ContentDelegateTest.kt, and build all builds for Android, and make all tests run: https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=212562803&revision=88b2c64c0da961b5701d489b406d7cc8c72286e1 Then, I don't see any errors. So, I assume that the change is correct.
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/d4a142e1648b part 0: Add "input" event tests into existing tests r=smaug https://hg.mozilla.org/integration/autoland/rev/48440593d675 part 1: Make all "input" event dispatcher in C++ use new utility method r=smaug https://hg.mozilla.org/integration/autoland/rev/110224ca9256 part 2: Make nsTextEditorState::SetValue() dispatch "input" event if it's called for handling part of user input r=smaug https://hg.mozilla.org/integration/autoland/rev/7f09364736a0 part 3: Make TextEditRules::WillSetText() not handle anything when EditAction is eReplaceText r=m_kato https://hg.mozilla.org/integration/autoland/rev/09fd7845a50b part 4: Make all script for web content dispatch "input" event with proper event interface r=smaug https://hg.mozilla.org/integration/autoland/rev/824bcd08c85e part 5: Make HTMLEditor::InsertTableCellsWithTransaction() create AutoPlaceholderBatch and AutoTopLevelEditSubActionNotifier r=m_kato
Depends on: 1524212
Regressions: 1584963
Regressions: 1606043
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: