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: