[UI Events][Input Events] implement beforeinput event
Categories
(Core :: DOM: Editor, defect, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox74 | --- | fixed |
People
(Reporter: devongovett, Assigned: masayuki)
References
(Depends on 2 open bugs, )
Details
(Keywords: dev-doc-complete, parity-chrome, parity-safari, Whiteboard: [tw-dom])
Attachments
(5 files)
Comment 1•12 years ago
|
||
| Reporter | ||
Comment 2•12 years ago
|
||
Comment 3•12 years ago
|
||
Comment 4•12 years ago
|
||
Comment 5•12 years ago
|
||
| Assignee | ||
Comment 6•12 years ago
|
||
Comment 7•11 years ago
|
||
Updated•11 years ago
|
Updated•10 years ago
|
Updated•10 years ago
|
Updated•9 years ago
|
Updated•9 years ago
|
Comment 9•9 years ago
|
||
Comment 10•9 years ago
|
||
| Assignee | ||
Comment 11•9 years ago
|
||
| Assignee | ||
Comment 12•9 years ago
|
||
Comment 13•9 years ago
|
||
| Assignee | ||
Comment 14•9 years ago
|
||
Updated•8 years ago
|
Comment 15•8 years ago
|
||
Comment 16•8 years ago
|
||
Comment 17•8 years ago
|
||
Updated•8 years ago
|
Updated•8 years ago
|
Updated•8 years ago
|
Updated•8 years ago
|
| Assignee | ||
Comment 18•8 years ago
|
||
| Assignee | ||
Comment 19•8 years ago
|
||
Comment 20•8 years ago
|
||
Comment 21•8 years ago
|
||
| Assignee | ||
Updated•8 years ago
|
Comment 22•7 years ago
|
||
Comment 23•7 years ago
|
||
Comment 24•7 years ago
|
||
| Assignee | ||
Comment 25•7 years ago
|
||
Comment 26•7 years ago
|
||
| Assignee | ||
Comment 27•7 years ago
|
||
Comment 28•7 years ago
|
||
Comment 29•7 years ago
|
||
Comment 30•7 years ago
|
||
Comment 31•7 years ago
|
||
| Assignee | ||
Comment 32•7 years ago
|
||
| Assignee | ||
Comment 33•7 years ago
•
|
||
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.)
Updated•7 years ago
|
Comment 34•6 years ago
|
||
See bug 1547409. Moving webcompat whiteboard tags to project flags.
Updated•6 years ago
|
Updated•6 years ago
|
Comment 35•6 years ago
|
||
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.
| Assignee | ||
Comment 36•6 years ago
|
||
(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.
| Assignee | ||
Comment 37•6 years ago
|
||
FYI: Also Safari conforms to Level 2 not completely. Safrai's event order around composition events is completely broken.
Comment 38•6 years ago
|
||
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.
Comment 39•6 years ago
|
||
For testing reference, there is web platform tests coverage for beforeinput in these tests:
- editing/other/exec-command-with-text-editor.tentative.html
- input-events/input-events-exec-command.html
- input-events/input-events-cut-paste-manual.html
- input-events/input-events-get-target-ranges-manual.html
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).
| Assignee | ||
Comment 40•6 years ago
|
||
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.
Comment 41•6 years ago
|
||
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).
Comment 42•6 years ago
|
||
We're working on changes to TinyMCE that will make use of beforeinput, so this would be good for us as well.
| Assignee | ||
Comment 43•6 years ago
|
||
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.
| Assignee | ||
Comment 44•6 years ago
|
||
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.
| Assignee | ||
Comment 45•6 years ago
|
||
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
| Assignee | ||
Comment 46•6 years ago
|
||
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
| Assignee | ||
Comment 47•6 years ago
|
||
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
| Assignee | ||
Comment 48•6 years ago
|
||
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
| Assignee | ||
Comment 49•6 years ago
|
||
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.
Comment 50•6 years ago
|
||
Comment 51•6 years ago
|
||
Backed out for xpcshell perma fails.
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=284785908&repo=autoland&lineNumber=4379
Backout: https://hg.mozilla.org/integration/autoland/rev/d1bd6a967ff28b0137a78b4d6bc2041068d8da3a
| Assignee | ||
Comment 52•6 years ago
|
||
Ah, I forgot this fact:
https://searchfox.org/mozilla-central/rev/9e45d74b956be046e5021a746b0c8912f1c27318/dom/base/nsContentUtils.cpp#4214-4215
Comment 53•6 years ago
|
||
Comment 54•6 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/9b29ee6fd891
https://hg.mozilla.org/mozilla-central/rev/648ad637c58e
https://hg.mozilla.org/mozilla-central/rev/2ce2d30b65e7
https://hg.mozilla.org/mozilla-central/rev/2dbb6572374c
https://hg.mozilla.org/mozilla-central/rev/f6a56b3d0955
| Assignee | ||
Updated•6 years ago
|
Comment 55•6 years ago
|
||
@masayuki, you are truly amazing. Thank you for all of your hard work! Really looking forward to this.
Comment 56•6 years ago
|
||
Since this is disabled by default, I've done the following for documentation purposes:
- Made a few improvements to the article about the beforeinput event
- Added a row to the DOM section of the article Experimental features in Firefox describing this change
- Added the
dev-doc-neededkeyword to bug 1609291 (covering enabling the event by default)
Comment 57•5 years ago
|
||
@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?
| Assignee | ||
Comment 58•5 years ago
•
|
||
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.
Comment 59•5 years ago
|
||
Great to hear! Fingers crossed it's the next cycle. Thanks for the speedy response.
Comment 60•5 years ago
|
||
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?
Description
•