1609291 - [UI Events][Input Events] Ship "beforeinput" event and `InputEvent.getTargetRanges()` in Nightly (and early beta) channel

Status: RESOLVED FIXED

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

Description

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

Perhaps, we need QA with real web sites before shipping beforeinput event, and some web apps may require InputEvent.getTargetRanges() if they want to modify target ranges before browsers do default action.

Comment 1

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

FYI: You can turn it enabled with dom.input_events.beforeinput.enabled in about:config.

Comment 2

[Eric Shepherd [:sheppy]]

For documentation purposes: this was implemented in Firefox 74; awaiting on-by-default to list it as supported in BCD. Be sure to also remove it from the DOM section in "Experimental features in Firefox".

Comment 3

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

Finally, I must have filed necessary tasks for writing patches for this bug. In my current plan, we should make getTargetRanges() should include invisible white-spaces which should be removed to avoid making them visible accidentally. However, we should not include newly empty ancestor inline/block elements for compatibility with both Blink and WebKit. And for now, I won't collect actual deleting range in table. It requires more refactoring, but not so important for shipping in Nightly channel. And also, I won't care about deletion with multiple selection ranges because it requires a lot of traditional behavior changes (i.e., risky for web-compat) and not supported by the other browsers and users must not use multiple selection ranges in editor.

Comment 4

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

Enabling beforeinput event in Nightly seems that it does not cause perf regression at least for Raptor.
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=d3f42b67d723dbe523f0c5c12d74aad923ba5ca6&newProject=try&newRevision=e65f61f0cc0b9f7ab826bb79a259b91f078d91c0&framework=10

Comment 5

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

Attachment: Bug 1609291 - Fix a simple bug of `test_dom_keyboard_event.html` r=smaug!

Comment 6

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

Attachment: Bug 1609291 - Make `test_input_event.html` put off to initialize elements whose `onbeforeinput` will be used r=smaug!

The test enables beforeinput pref at getting focus. However, before that,
each element whose onbeforeinput is used to check the test result is created
and whose onbeforeinput is set. In this order, onbeforeinput isn't
available as an event listener attribute.

This patch makes the test create and initialize <input> and <textarea>
elements after enabling beforeinput.

Depends on D91860

Comment 7

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

Attachment: Bug 1609291 - Make run all tests in `test_clipboard_events.html` with a child window r=smaug!

Although I don't understand 100% of the reason why it fails to enable
onbeforeinput attribute with enabling the pref at first add_task(),
it seems that it's too late to enable it (I guess that JIT compiler refers
GlobalEventHandlers before enabling it). Anyway, I don't find a way
to test it with dynamically enables beforeinput events in it.

Therefore, this patch rewrites the test without add_task() and make it
more stable at least on my environment. Even with this change, I see orange
in my environment, but I guess that SimpleTest.promiseClipboardChange() has
a bug of something making it unstable.

Depends on D91861

Comment 8

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

Attachment: Bug 1609291 - Fix new failure of `test_bug430392.html` when `beforeinput` is enabled r=smaug!

If beforeinput event is enabled, the assertion count hitting in
WSRunScanner::GetEditableBlockParentOrTopmotEditableInlineContent() becomes 0.
The reason is, AutoEditActionDataSetter::MaybeDispatchBeforeInputEvent()
gets nullptr as the result of HTMLEditor.GetInputEventTargetElement().
Then, it returns error and every edit action handlers stop handling it.
However, for the main purpose of beforeinput, i.e., overriding browsers'
default action, we need to dispatch beforeinput event even in such case.
(I.e., web apps may want to do something around the non-editable node.)

Therefore, this patch makes HTMLEditor.GetInputEventTargetElement() scan
editing host by itself if HTMLEditor.GetActiveEditingHost() returns nullptr
due to non-editable focus node. And also make test_bug430392.html check
whether beforeinput events and input events are fired as expected.

I think that I should add new WPT to check nested editing host cases for
considering beforeinput and input event target, but I'd like to do it
in another bug for shipping beforeinput in Nightly channel as soon as
possible.

Depends on D91862

Comment 9

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

Attachment: Bug 1609291 - Make `AutoEditActionDataSetter::MaybeDispatchBeforeInputEvent()` stop returning error when there is no proper event target of `beforeinput` event r=smaug!

editor/libeditor/tests/test_bug974309.html and
editor/libeditor/tests/test_bug1268736.html start to throw exception if
beforeinput event is enabled. They test XPCOM editor API when a non-editable
element has focus and it's outside of any editing hosts. In this case,
we shouldn't throw an exception for backward compatibility. Although the
all callers should stop handling any edit action in this case, let's make
MaybeDispatchBeforeInputEvent() return a special success code instead of
an error code for making all callers keep working with traditional paths.

Depends on D91863

Comment 10

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

Attachment: Bug 1609291 - Enable `beforeinput` event and `InputEvent.getTargetRanges()` by default in Nightly channel and early Beta r=smaug!

Now, InputEvent.getTargetRanges() returns better ranges when deleting
selected ranges (both collapsed cases and non-collapsed cases, and the
result is similar to Blink). So, beforeinput events listeners can treat
deletion.

Therefore, we can ship beforeinput event for getting more feedback from
our testers.

Note that both of them are behind same pref because getTargetRanges()
is required only by beforeinput event listeners and it may be used
for feature detection of beforeinput event because Blink does not support
onbeforeinput event handler attribute.

Depends on D91864

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b8e7854d72ce
Fix a simple bug of `test_dom_keyboard_event.html` r=smaug
https://hg.mozilla.org/integration/autoland/rev/ead7db3e08f7
Make `test_input_event.html` put off to initialize elements whose `onbeforeinput` will be used r=smaug
https://hg.mozilla.org/integration/autoland/rev/2a6bac028754
Make run all tests in `test_clipboard_events.html` with a child window r=smaug
https://hg.mozilla.org/integration/autoland/rev/3ad77c14180a
Fix new failure of `test_bug430392.html` when `beforeinput` is enabled r=smaug
https://hg.mozilla.org/integration/autoland/rev/e9803b0d9c1d
Make `AutoEditActionDataSetter::MaybeDispatchBeforeInputEvent()` stop returning error when there is no proper event target of `beforeinput` event r=smaug
https://hg.mozilla.org/integration/autoland/rev/55d1325436c1
Enable `beforeinput` event and `InputEvent.getTargetRanges()` by default in Nightly channel and early Beta r=smaug

Comment 12

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/b8e7854d72ce
https://hg.mozilla.org/mozilla-central/rev/ead7db3e08f7
https://hg.mozilla.org/mozilla-central/rev/2a6bac028754
https://hg.mozilla.org/mozilla-central/rev/3ad77c14180a
https://hg.mozilla.org/mozilla-central/rev/e9803b0d9c1d
https://hg.mozilla.org/mozilla-central/rev/55d1325436c1

Comment 13

[Chris Mills [:cmills]]

These changes have been documented; see https://github.com/mdn/sprints/issues/3799#issuecomment-717309934 for the full details.

Let us know if you need anything else. Thanks!