[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, Blocks 1 open bug, )
Details
(Keywords: dev-doc-complete, parity-chrome, parity-safari, Whiteboard: [tw-dom])
Attachments
(5 files)
Comment 1•11 years ago
|
||
Reporter | ||
Comment 2•11 years ago
|
||
Comment 3•11 years ago
|
||
Comment 4•11 years ago
|
||
Comment 5•11 years ago
|
||
Assignee | ||
Comment 6•11 years ago
|
||
Comment 7•11 years ago
|
||
Updated•10 years ago
|
Updated•9 years ago
|
Updated•9 years ago
|
Updated•9 years ago
|
Updated•9 years ago
|
Comment 9•9 years ago
|
||
Comment 10•8 years ago
|
||
Assignee | ||
Comment 11•8 years ago
|
||
Assignee | ||
Comment 12•8 years ago
|
||
Comment 13•8 years ago
|
||
Assignee | ||
Comment 14•8 years ago
|
||
Updated•7 years ago
|
Comment 15•7 years ago
|
||
Comment 16•7 years ago
|
||
Comment 17•7 years ago
|
||
Updated•7 years ago
|
Updated•7 years ago
|
Updated•7 years ago
|
Updated•7 years ago
|
Assignee | ||
Comment 18•7 years ago
|
||
Assignee | ||
Comment 19•7 years ago
|
||
Comment 20•7 years ago
|
||
Comment 21•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Comment 22•7 years ago
|
||
Comment 23•6 years ago
|
||
Comment 24•6 years ago
|
||
Assignee | ||
Comment 25•6 years ago
|
||
Comment 26•6 years ago
|
||
Assignee | ||
Comment 27•6 years ago
|
||
Comment 28•6 years ago
|
||
Comment 29•6 years ago
|
||
Comment 30•6 years ago
|
||
Comment 31•6 years ago
|
||
Assignee | ||
Comment 32•6 years ago
|
||
Assignee | ||
Comment 33•6 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•6 years ago
|
Comment 34•6 years ago
|
||
See bug 1547409. Moving webcompat whiteboard tags to project flags.
Updated•5 years ago
|
Updated•5 years ago
|
Comment 35•5 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•5 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•5 years ago
|
||
FYI: Also Safari conforms to Level 2 not completely. Safrai's event order around composition events is completely broken.
Comment 38•5 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•5 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•5 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•5 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•5 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•5 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•5 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•5 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•5 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•5 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•5 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_ASSERT
s.
Depends on D58126
Assignee | ||
Comment 49•5 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•5 years ago
|
||
Comment 51•5 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•5 years ago
|
||
Ah, I forgot this fact:
https://searchfox.org/mozilla-central/rev/9e45d74b956be046e5021a746b0c8912f1c27318/dom/base/nsContentUtils.cpp#4214-4215
Comment 53•5 years ago
|
||
Comment 54•5 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•5 years ago
|
Comment 55•5 years ago
|
||
@masayuki, you are truly amazing. Thank you for all of your hard work! Really looking forward to this.
Comment 56•5 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-needed
keyword to bug 1609291 (covering enabling the event by default)
Comment 57•4 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•4 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•4 years ago
|
||
Great to hear! Fingers crossed it's the next cycle. Thanks for the speedy response.
Comment 60•4 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
•