Closed Bug 970802 Opened 10 years ago Closed 4 years ago

[UI Events][Input Events] implement beforeinput event

Categories

(Core :: DOM: Editor, defect, P3)

Product:
Core
Component:
DOM: Editor
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla74
Project Flags:
Webcompat Priority P2
Tracking Flags:
Tracking Status
firefox74 --- fixed

People

(Reporter: devongovett, Assigned: masayuki)

References

(Depends on 2 open bugs, Blocks 1 open bug,
URL
)

Details

(Keywords: dev-doc-complete, parity-chrome, parity-safari, Whiteboard: [tw-dom])

Attachments

(5 files)

Bug 970802 - part 1: Add `beforeinput` event tests into existing mochitests r=smaug!
47 bytes, text/x-phabricator-request
Details | Review
Bug 970802 - part 2: HTML editor command classes shouldn't handle edit actions multiple times r=m_kato!
47 bytes, text/x-phabricator-request
Details | Review
Bug 970802 - part 3: Implement `beforeinput` event dispatcher and add `onbeforeinput` event handler attribute r=smaug!
47 bytes, text/x-phabricator-request
Details | Review
Bug 970802 - part 4: Make `TextControlState` dispatch "beforeinput" event if there is no `TextEditor` r=smaug!
47 bytes, text/x-phabricator-request
Details | Review
Bug 970802 - part 5: Make `AutoEditActionDataSetter` created method dispatch "beforeinput" event r=smaug!,m_kato!
47 bytes, text/x-phabricator-request
Details | Review 
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_1) AppleWebKit/537.73.11 (KHTML, like Gecko) Version/7.0.1 Safari/537.73.11

Steps to reproduce:

http://www.w3.org/TR/DOM-Level-3-Events/#event-type-beforeinput
That specification is obsolete (and doesn't define the event clearly enough to be implementable anyway).  The current specifications covering that area are http://www.whatwg.org/specs/web-apps/current-work/multipage/ and http://dom.spec.whatwg.org/ and neither one mentions this event at all.
Can you point me to something that says the spec is obsolete? That draft I linked to is only a few months old.

If it is obsolete, then where are the events that were being specified there going to be specified now? There is absolutely no mention of the keyboard events (e.g. keydown, keyup, input, beforeinput), and even the other events in that spec I linked to. Are these just not specified at all anymore? I find this hard to believe.
> That draft I linked to is only a few months old.

Hrm.  I didn't realize they were still working on that one.  Olli?
Flags: needinfo?(bugs)
"input" events are specified in the HTML specification, for what it's worth.
Masayuki, you might know the current status with this event.
Flags: needinfo?(bugs)
beforeinput event hasn't been stable yet. We don't discuss about this deeply.
Summary: implement beforeInput event → implement beforeinput event
The DOM3 Events spec is still trying to go to CR last I heard. In theory it's being replaced by the UI Events spec: https://dvcs.w3.org/hg/d4e/raw-file/tip/source_respec.htm. I'm not sure what the current progress on either is though.

As far as I know, the only controversial issue with beforeinput is https://www.w3.org/Bugs/Public/show_bug.cgi?id=23913. Masayuki and I have hit a bit of an impasse there.

I agree it'd be nice to have a spec that was actually implementable and to have it in one place instead of in a DOM spec and the HTML spec.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: btpp-followup-2016-03-10, [tw-dom]
Whiteboard: btpp-followup-2016-03-10, [tw-dom] → [tw-dom]
This won't be exactly a simple bug to fix and while implementing this need to review the spec and file spec bugs and also test other implementations (which are also guaranteed to have some unusual behavior given the complexity here).

Filed one spec bug https://github.com/w3c/uievents/issues/87
Johannes thinks this is the #1 priority bug for editing <https://github.com/w3c/editing/pull/146#issuecomment-242434100>.  Masayuki, do you think it makes sense for me to look at taking this, including spec review and testing?  Where should I look in our code to fire a beforeinput event on user actions?

Since Chrome is already implementing this, it would be a good idea to start work on it soon so that their implementation doesn't get set in stone before we can review it.
Flags: needinfo?(masayuki)
# Sorry for the delay to reply due to my business trip and being sick yesterday.

Yeah, if we could implement it, it'd be great to me too. However, I don't have much time as least this Q3. And perhaps, Q4 isn't enough long time to work on it to me. So, you'll work on it, I'm very happy ;-)

Where is a good point to dispatch beforeinput event depends on what causes the input.

If keypress event will cause input, I think that we should dispatch it from EventStateManager::PreHandleEvent() when it receives an eKeyPress event.
# beforeinput should be be fired before keypress event <https://www.w3.org/TR/uievents/#keypress-event-order>.

If composition string will be changed, I think that we should dispatch it from TextComposition::CloneAndDispatchAs() when it dispatches eCompositionUpdate because we need to dispatch beforeinput before dispatching compositionupdate <https://www.w3.org/TR/uievents/#events-composition-input-events>. (And I think that we need to rename it or create a new method, DispatchBeforeInputAndCompositionUpdate() which dispatches beforeinput and calls CloneAndDispatchAs() with eCompositionUpdate.)

I'm not sure where is the best place to dispatch beforeinput for the others. E.g., cut, paste, undo, redo. (We should NOT dispatch beforeinput for execCommand()? I don't check the conclusion of discussion about it, it's difficult to do it due to security issue.)
Flags: needinfo?(masayuki)
> If keypress event will cause input, I think that we should dispatch it from
> EventStateManager::PreHandleEvent() when it receives an eKeyPress event.
> # beforeinput should be be fired before keypress event
> <https://www.w3.org/TR/uievents/#keypress-event-order>.

And I think that we should do this only when the eKeyPress event will cause changing DOM in the editor's normal path. So, ESM needs to retrieve target editor and editor needs to implement a test event method which returns true when eKeyPress event causes changing the DOM.
If you think this is a really big project, then I'm afraid I won't have time to work on it, given my limited work hours.
According to ContentEditable spec, we need to dispatch beforeinput event from EventStateManager and it needs to check if it should be fired with "contenteditable" state or an editable element like <textarea>.
https://w3c.github.io/editing/contentEditable.html#editing-states

I think that, first, we should implement beforeinput event dispatched immediately before keypress and compositionupdate as our internal event and dispatch it from ESM.

Then, editor should switch to handle "beforeinput" instead of "keypress".

After that, we should implement "beforeinput" for paste or something if it's necessary (currently, UI Events doesn't say it should be fired, though).

Finally, expose "beforeinput" to the web.
See Also: → 1181501
Blocks: 1379584
Whiteboard: [tw-dom] → [tw-dom][parity-webkit][parity-blink]
pinging Mike to check Web Compatibility and priority on this on this...
Flags: needinfo?(miket)
Flags: needinfo?(miket) → webcompat?
Priority: -- → P3
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #14)
> Then, editor should switch to handle "beforeinput" instead of "keypress".

Oops, this must be wrong because keypress event should be fired on editor and the editor value shouldn't have been changed when web content received keypress event.
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #18)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #14)
> > Then, editor should switch to handle "beforeinput" instead of "keypress".
> 
> Oops, this must be wrong because keypress event should be fired on editor
> and the editor value shouldn't have been changed when web content received
> keypress event.

Or, ESM should make beforeinput event store WidgetKeyboardEvent of eKeyPress event. Then, when editor receives beforeinput and needs to consume it, editor can dispatch eKeyPress event before changing the DOM. Finally, ESM stops dispatching eKeyPress event only when the eKeyPress event has already been dispatched. I guess that this is better.
I can see that this issue has only x86 and Mac OS X specified as platforms, but it's my understanding that the event should be cross platform, right?
yeah, don't care about the OS field too much, the bug was probably just filed on a Mac.
OS: Mac OS X → All
Hardware: x86 → All
Summary: implement beforeinput event → [UI Events][Input Events] implement beforeinput event
Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)
Keywords: parity-chrome, parity-safari
Whiteboard: [tw-dom][parity-webkit][parity-blink] → [tw-dom]
Any news on this? It makes it much harder to build advanced editors, and means that lots of context menu actions are broken in strange ways.
It's in progress. First we need to implement inputType (bug 1447239), however. The work for inputType (e.g. bug 1465702) is progressing steadily.
Yes, and also there is a big issue to implement beforeinput. That is, beforeinput has a lot of attributes which indicate where will be modified.  On the other hand, our editor adjusts selection, changes DOM tree in each step. So, we need to change our editor design to compute anything *before* dispatching beforeinput event. That's really hard. Additionally, if beforeinput should be fired before keypress event since our editor handles the following keypress event. This requires a lot of changes in keypress event path probably. For example, we need to change autocomplete.
Changing all this in the legacy contentEditable="true" is hard and will add inevitable complex regressions because all actual rich editors implemented on top of contentEditable="true" has many awful hacks to get around each navigator issues.

Why don't you leave contentEditable="true" as is, and focus on a fully fresh implementation with the new contentEditable states? 
https://w3c.github.io/editing/contentEditable.html#events-state
(In reply to Sylvain Spinelli from comment #26)
> Changing all this in the legacy contentEditable="true" is hard and will add
> inevitable complex regressions because all actual rich editors implemented
> on top of contentEditable="true" has many awful hacks to get around each
> navigator issues.

contenteditable="true" is not legacy. There is not standardized alternative API for now.

> Why don't you leave contentEditable="true" as is,

What do you mean? Any behavior of operations in contenteditable hasn't been standardized.

> and focus on a fully fresh implementation with the new contentEditable states? 
> https://w3c.github.io/editing/contentEditable.html#events-state

It's really out of scope of this bug, though, as far as I know, the other values of contenteditable won't be standardized since contenteditable-disabled is proposed and being discussed.
https://github.com/w3c/editing/issues/176

Anyway, there are only some proposals, no standardized specs. (I think that "events" and "caret" are really bad idea since they must cause allowing IME unaware applications.)
(In reply to Sylvain Spinelli from comment #26)
> Why don't you leave contentEditable="true" as is, and focus on a fully fresh
> implementation with the new contentEditable states? 
> https://w3c.github.io/editing/contentEditable.html#events-state

I don't see the point of leaving the browser implementation incomplete and inconsistent with other browsers. As mentioned by others, beforeinput event will be very helpful for rich text editors that are based on contenteditable. Actually the fact that FF does not handle it, requires mentioned "awful hacks" - so adding this means one awful hack less (for CKEditor that's undo - one of key features).

There is always a risk of regressions, but that should not stop Mozilla team from enhance Firefox. Especially given that there's probably a good test coverage for this component.
Thanks for your replies.

FYI, I develop dedicated WYSIWYM (https://en.wikipedia.org/wiki/WYSIWYM) editors and my point of view seams different:
- contentEditable="true" implementations are really different and I think it will be very hard to unified. The goal seems unachievable.
- my needs are *highly standardized* solid natives foundations, especially caret and selection management.
- others native features like actions described in contenteditable-disabled proposal are useless in my case. I need to manage them myself with dedicated semantic and scheme constraints.
- beforeinput event is really important (IME...), but need to be cancelable (specified in "Input Events Level 2", not implemented in Chromium).

In conclusion, I think a well standardized cancelable beforeinput event in a contentEditable="typing state" context would be perfect in my case.
> Especially given that there's probably a good test coverage for this component.

There isn't.

But also the big regression worry is webcompat regressions, not functionality regressions.  That is, sites having Firefox-specific codepaths that will break if we change our behavior, even if we make the behavior identical to other browsers in the process....
Any progress on this? Being able to cancel default behaviour for beforeinput would solve a lot of problems. It does not conform to W3C documentation that reads the event should be cancellable.
See the dependency tree in this page. I'm still working on InputEvent.inputType.

Oh, neither Chrome nor Safari conforms to current event order of keypress event declared by UI Events. Additionally, their behavior is better than the UI Events declaration for Gecko. I filed spec issue here: https://github.com/w3c/uievents/issues/220

(This is the biggest blocker of this bug.)

Component: General → Editor

See bug 1547409. Moving webcompat whiteboard tags to project flags.

Webcompat Priority: --- → ?
Webcompat Priority: ? → P2
Flags: webcompat?

Hi, just wanted to confirm if InputEvents Level 2 specification is going to be used for the implementation, rather than Level 1? I was trying to implement a small wysiwyg editor as a test in blink/webkit, and I found the Level 1 implementation in blink was unusable; it doesn't let you cancel many of the beforeinput inputType's, and so you can't (for example) sanitize input before it is inserted.

(In reply to ozixtheorange from comment #35)

Hi, just wanted to confirm if InputEvents Level 2 specification is going to be used for the implementation, rather than Level 1? I was trying to implement a small wysiwyg editor as a test in blink/webkit, and I found the Level 1 implementation in blink was unusable; it doesn't let you cancel many of the beforeinput inputType's, and so you can't (for example) sanitize input before it is inserted.

Level 2 is too risky draft since it does not take care of backward compatibility. For example, see this issue. If you just want to make some inputType cases cancelable even in Level 1, I think that just requesting such change to Level 1 is better for short-term goal.

Without solving such unstable points of Level 2 or Blink shipping level 2, we won't support Level 2 by default.

FYI: Also Safari conforms to Level 2 not completely. Safrai's event order around composition events is completely broken.

Okay, I can see there's some controversy there. Could you put in the Level 2 cancelable guarantees for this implementation then? I believe that would be sufficient for my case. The main thing is being able to cancel the delete/insert events with "undefined" cancelability in Level 1. That way, I can manually delete (or choose to not delete, depending on the elements), and also sanitize the new content.

I'm actually fine if deleteByComposition happens after compositionstart event. My only request is that you can cancel the deleteByComposition/insertFromComposition so that I can remove the initially selected nodes myself, or insert the finalized IME content myself.

For testing reference, there is web platform tests coverage for beforeinput in these tests:

However, I see a note that the interaction of KeyboardEvent between beforeinput and input isn't yet tested, so coverage may be questionable (WebKit's LayoutTests and Blink's web tests both seem to have quite a few tests which may help improve coverage in that case).

First, I'll implement beforeinput event roughly and behind pref even in Nightly. Then, I'll fix smaller bugs to conform to the spec. Then, enable it on Nightly channel.

Assignee: nobody → masayuki
Status: NEW → ASSIGNED

Note that coda.io also relies on beforeinput, and it might actually be the only major feature missing which is blocking them (if I work around it with a hack, typing works fine on the site).

See Also: → https://webcompat.com/issues/15276

We're working on changes to TinyMCE that will make use of beforeinput, so this would be good for us as well.

Currently, spec declares that beforeinput event should be fired before keypress and compositionupdate. However, the former behavior is different from any other browsers so that we don't need to conform to the spec, I mean the spec should be fixed for existing behavior. And the latter should also be fixed by the spec for consistency with keypress case and the spec was just documenting IE's behavior, but according to MS, it was a bug of IE, surprisingly. Safari dispatches beforeinput event after compositionupdate, so, we will follow the Safari's behavior because we don't need additional big redesign of editor. Anyhow, both compositionupdate and beforeinput are fired before modifying the DOM, therefore, the order must not cause problem in the real use cases.

This patch adds a lot of beforeinput event tests into existing mochitests
which test input events. But this does not add tests of canceling
beforeinput event because it requires really complicated path until
implementing beforeinput actually.

Note that beforeinput event is not fired with Document.execCommand().
Therefore, this patch does not add WPT for testing beforeinput event.
And unfortunately, WPT cannot test most cases of the new tests.

Some HTML editor command classes call *AsAction() methods multiple times.
That causes that user needs multiple undo/redo for a command and one command
causes multiple "input" events. For both compatibility with the other
browsers and making "beforeinput" cancelable, they should call *AsAction()
once. Instead, HTMLEditor should handle related element deletion.

This patch makes HTMLEditor::RemoveInlinePropertyInternal() remove multiple
elements if its caller allows, and HTMLEditor::SetInlinePropertyAsAction()
call RemoveInlinePropertyInternal() in some cases.

According to comm-central and BlueGriffon, we can make
HTMLEditor::RemoveInlineProperty() also removes the related methods
automatically because comm-central does same thing from script and BlueGriffon
does not use this. However, we cannot do that for
HTMLEditor::SetInlineProperty() because BlueGriffon may call it with any
HTML5 elements and does not expect removing elements in same block at that
time. If we needed to reduce the overhead of comm-central, we could change
only RemoveInlineProperty(), but it would make these APIs behavior
inconsistent.

Depends on D58123

This patch makes nsContentUtils::DispatchInputEvent() dispatch beforeinput
event too. And also adds onbeforeinput event handler which is really
important for feature detection (although Chrome has not implemented this
attribute yet: https://bugs.chromium.org/p/chromium/issues/detail?id=947408).

However, we don't implement InputEvent.getTargetRanges() in this bug and
implementing beforeinput event may hit bugs of some web apps. Therefore,
this patch disables beforeinput event by default even in Nightly channel.

Depends on D58124

If TextControlState does not have TextEditor and its SetValue() is called
from SetUserInput(), TextControlState itself needs to dispatch beforeinput
event.

If the value is modified by beforeinput event listener, it's intended that
preventDefault() is called by the web apps. However, the behavior in this
case is not mentioned by UI Events nor Input Events spec. We should just file
a spec issue instead of emulating Chrome's behavior for now because it requires
more changes, but this case must be an edge case.
The spec issue is: https://github.com/w3c/input-events/issues/106

Depends on D58125

AutoEditActionDataSetter is created in the stack when editor's public method
is called and that guarantees lifetime of global objects in editor such as
editor itself, selection controller, etc.

The dispatcher of beforeinput event returns NS_ERROR_EDITOR_ACTION_CANCELED
if an event is actually dispatched but canceled. The reason why it's an error
is, editor code must stop handling anything when any methods return error.
So, returning an error code is reasonable in editor module. But when it's
filtered by EditorBase::ToGenericNSResult() at return statement of public
methods, it's converted to NS_SUCCESS_DOM_NO_OPERATION. This avoids throwing
new exception, but editor class users in C++ can distinguish whether each edit
action is canceled or handled. The reason why we should not throw new
exception from XPCOM API is, without taking care of each caller may break some
our UI (especially for avoiding to break comm-central). Therefore, this patch
does not make XPCOM methods return error code when beforeinput event is
canceled.

In most cases, immediately after creating AutoEditActionDataSetter is good
timing to dispatch beforeinput event since editor has not touched the DOM
yet. If beforeinput requires data or dataTransfer, methods need to
dispatch beforeinput event after that. Alhtough this is not a good thing
from point of view of consistency of the code. However, I have no better
idea.

Note 1: Our implementation does NOT conform to the spec about event order
between keypress and beforeinput (dispatching beforeinput event after
keypress event). However, we follow all other browsers' behavior so that it
must be safe and the spec should be updated for backward compatibility.
Spec issue: https://github.com/w3c/uievents/issues/220

Note 2: Our implementation does NOT conform to the spec about event order
between compositionupdate and beforeinput. Our behavior is same as
Safari, but different from Chrome. This might cause web-compat issues.
However, our behavior does make sense from point of view of consistency of
event spec. Additionally, at both compositionupdate and beforeinput,
composition string in editor has not been modified yet. Therefore, this
may not cause web-compat issues (and I hope so).
Spec issue: https://github.com/w3c/input-events/issues/49

Note that this patch makes editor detect bugs that beforeinput event hasn't
been handled yet when it dispatches input event or modifying data and
dataTransfer value are modified after dispatching beforeinput event with
MOZ_ASSERTs.

Depends on D58126

All patches are really big, but most part is changing or adding tests, and C++ changes are simple. Don't mind even if you take long time to review them.

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/aa9bd45c09b1
part 1: Add `beforeinput` event tests into existing mochitests r=smaug
https://hg.mozilla.org/integration/autoland/rev/ce6853e64ed6
part 2: HTML editor command classes shouldn't handle edit actions multiple times r=m_kato
https://hg.mozilla.org/integration/autoland/rev/6b185296c742
part 3: Implement `beforeinput` event dispatcher and add `onbeforeinput` event handler attribute r=smaug
https://hg.mozilla.org/integration/autoland/rev/1fb9cf2264b6
part 4: Make `TextControlState` dispatch "beforeinput" event if there is no `TextEditor` r=smaug
https://hg.mozilla.org/integration/autoland/rev/5511edd700f7
part 5: Make `AutoEditActionDataSetter` created method dispatch "beforeinput" event r=smaug,m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/9b29ee6fd891
part 1: Add `beforeinput` event tests into existing mochitests r=smaug
https://hg.mozilla.org/integration/autoland/rev/648ad637c58e
part 2: HTML editor command classes shouldn't handle edit actions multiple times r=m_kato
https://hg.mozilla.org/integration/autoland/rev/2ce2d30b65e7
part 3: Implement `beforeinput` event dispatcher and add `onbeforeinput` event handler attribute r=smaug
https://hg.mozilla.org/integration/autoland/rev/2dbb6572374c
part 4: Make `TextControlState` dispatch "beforeinput" event if there is no `TextEditor` r=smaug
https://hg.mozilla.org/integration/autoland/rev/f6a56b3d0955
part 5: Make `AutoEditActionDataSetter` created method dispatch "beforeinput" event r=smaug,m_kato
Depends on: 1609629
No longer regressions: 1609629

@masayuki, you are truly amazing. Thank you for all of your hard work! Really looking forward to this.

Since this is disabled by default, I've done the following for documentation purposes:

Keywords: dev-doc-needed → dev-doc-complete
Regressions: 1659276

@masayuki @sheppy - Thanks for all the work on this. Just wondering if there is any status update on the beforeInput event being enabled by default in Firefox?

Well, if everything would be fine, it'd be enabled by default in Nightly channel in the next cycle. But I worried about the performance issue of that. If it'd be true, we could change the plan and it might cause put it off to the cycle after the next cycle.

Great to hear! Fingers crossed it's the next cycle. Thanks for the speedy response.

As of 84.0.2, this seems to be working well, but it's still hidden behind the dom.input_events.beforeinput.enabled flag. Is there still work remaining to have this enabled by default Firefox?

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: