998941 - [Input Events] Implement InputEvent.data and InputEvent.dataTransfer

Status: RESOLVED FIXED

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

Description

[Arkadiusz Michalski (Spirit)]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20140314220517

Steps to reproduce:

I open this bug to continue implement InputEvent interface (https://bugzilla.mozilla.org/show_bug.cgi?id=993253). Now we have support isComposing property but not data property (https://dvcs.w3.org/hg/dom3events/raw-file/tip/html/DOM3-Events.html#widl-InputEvent-data).

Comment 2

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

Unfortunately, I've not had enough knowledge of editor for implementing this feature yet, though. I would research about this.

Anyway, before implementing beforeinput event (bug 970802), we should implement this since .data attribute value is the most important information of beforeinput event.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

(w3.org/TR is for trash, always look at the latest version of a spec.)

Comment 4

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

Are you sure ? http://www.w3.org/TR/uievents/ even seems more recent at the moment...

Comment 5

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

So it looks like this is where we fire input events in the editor:

http://searchfox.org/mozilla-central/source/editor/libeditor/EditorBase.cpp#1977

The question is how to hook it up to the actual text we're inserting.  I suppose the most obvious way would be to add another member variable, but that seems cumbersome.  We'd also have to worry about reentrancy.  But I don't see another way to do it without a lot of refactoring.

Comment 6

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

Although, another member is anyway necessary, we might be able to get it with transaction manager. It's available only when undo/redo is enabled now. However, anyway we need transaction instances to do something modifying the editor. So, we can add them to transaction manager and clear them when we dispatch an input event.

Comment 7

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Bug 998941 - Implement InputEvent.data;

This is a new event from the spec, already supported by Chrome.  There's
no clear spec or tests, so the details of the implementation are
somewhat random.  This should be viewed as a work in progress, but is
probably better than nothing for now.

Review commit: https://reviewboard.mozilla.org/r/168366/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/168366/

Comment 8

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

Comment on attachment 8897081 [details]
Bug 998941 - Implement InputEvent.data;

https://reviewboard.mozilla.org/r/168366/#review173766

::: dom/webidl/InputEvent.webidl:10
(Diff revision 1)
>   */
>  
>  [Constructor(DOMString type, optional InputEventInit eventInitDict)]
>  interface InputEvent : UIEvent
>  {
> +  readonly attribute DOMString?    data;

I guess that we should make this behind a pref at least one cycle and we need to report some web app developers if this would break their feature detection and expected result for the users.

What do you think?

::: editor/libeditor/EditorBase.h:1251
(Diff revision 1)
> +  // What string we are inserting now, if any.
> +  nsString mStringToInsert;

Please insert this member around mRangeUpdater since this causes others would append other int/bool members and waste the footprint.

And cannot we use transactions in transaction manager to retrieve inserted string? If it's available, for example, if text is inserted two separated points in an edit action, using transactions approach can fire input events multiple times and reduce this additional footprint. I have no idea about the former case actually, however, such design is better for fixing bugs when we get bug reports.

Additionaly, only storing pointer to the first transaction of an edit action is enough in the approach.

How do you think? (Run time cost of your approach must be better, though.)

::: editor/libeditor/EditorBase.cpp:1996
(Diff revision 1)
> -    new EditorInputEventDispatcher(this, target, !!GetComposition()));
> +    new EditorInputEventDispatcher(this, target, !!GetComposition(),
> +      mStringToInsert));

nit: I like move mStringToInsert under "this".

::: editor/libeditor/EditorBase.cpp:2497
(Diff revision 1)
> +  // XXX Won't work if InsertTextImpl is called again before FireInputEvent
> +  mStringToInsert = aStringToInsert;

If we take this approach, how about to replace mStringToInsert when the transaction is CompositionTransaction, otherwise, just append the new string?

Additionally, I think that this is wrong point to modify mStringToInsert because the edit action might fail later. I think that it should be modified after succeeding to modify DOM tree actually.

::: editor/libeditor/HTMLEditor.cpp:1136
(Diff revision 1)
>  HTMLEditor::InsertBR(nsCOMPtr<nsIDOMNode>* outBRNode)
>  {
>    NS_ENSURE_TRUE(outBRNode, NS_ERROR_NULL_POINTER);
>    *outBRNode = nullptr;
>  
> +  mStringToInsert = NS_LITERAL_STRING("\n");

ditto.

This should be modified after CreateBR().

::: widget/TextEvents.h:1076
(Diff revision 1)
>  
>  class InternalEditorInputEvent : public InternalUIEvent
>  {
>  private:
>    InternalEditorInputEvent()
> -    : mIsComposing(false)
> +    : mData(EmptyString())

Why do we need to assign empty string here? I think that it doesn't need to be in initializing list.

::: widget/TextEvents.h:1090
(Diff revision 1)
>    }
>  
>    InternalEditorInputEvent(bool aIsTrusted, EventMessage aMessage,
>                             nsIWidget* aWidget = nullptr)
>      : InternalUIEvent(aIsTrusted, aMessage, aWidget, eEditorInputEventClass)
> +    , mData(EmptyString())

Same here.

::: widget/TextEvents.h:1107
(Diff revision 1)
>      result->AssignEditorInputEventData(*this, true);
>      result->mFlags = mFlags;
>      return result;
>    }
>  
> +  nsString mData;

I think that nsAutoString is better because Widget*Event and Internal*Event are allocated in stack in most cases.

Comment 9

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

I'll check the automated tests. However, for uplifting to upstream, shouldn't they be in separated patch?

Comment 10

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8897081 [details]
Bug 998941 - Implement InputEvent.data;

https://reviewboard.mozilla.org/r/168366/#review173982

::: dom/webidl/InputEvent.webidl:10
(Diff revision 1)
>   */
>  
>  [Constructor(DOMString type, optional InputEventInit eventInitDict)]
>  interface InputEvent : UIEvent
>  {
> +  readonly attribute DOMString?    data;

We could, but do you have any specific reason to think this would break feature-detection?

::: editor/libeditor/EditorBase.h:1251
(Diff revision 1)
> +  // What string we are inserting now, if any.
> +  nsString mStringToInsert;

I moved the member.  Unfortunately, I don't understand the rest of your comment, perhaps because I'm not familiar enough with how the transaction manager works.  Where would you like me to put the string?  As a member of nsTransactionManager?  Do you think one input event should be fired per transaction, or per edit action?  What do we do if a transaction is an aggregate of several transactions that do nontrivial things like insert and delete text, or insert in two different places?  (I don't know if we create such transactions.)

::: editor/libeditor/EditorBase.cpp:2497
(Diff revision 1)
> +  // XXX Won't work if InsertTextImpl is called again before FireInputEvent
> +  mStringToInsert = aStringToInsert;

Does appending the string always make sense?  What if there are two inserts in different places, for instance?

It's helpful to make two issues for two separate points so I can mark each point separately as fixed/dropped.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Aryeh Gregor (:ayg) (working until August 22, then no longer with Mozilla) from comment #10)

> >  [Constructor(DOMString type, optional InputEventInit eventInitDict)]
> >  interface InputEvent : UIEvent
> >  {
> > +  readonly attribute DOMString?    data;
Do we return same values from .data as other browsers which have shipped the attribute? If not, then it might break sites. 
And existence of .data might lead to different code paths which are so far tested only on some other browsers.

Comment 12

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8897081 [details]
Bug 998941 - Implement InputEvent.data;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168366/diff/1-2/

Comment 13

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8897081 [details]
Bug 998941 - Implement InputEvent.data;

https://reviewboard.mozilla.org/r/168366/#review174004

::: commit-message-1f363:9
(Diff revision 2)
> +no clear spec or tests, so the details of the implementation are
> +somewhat random.  This should be viewed as a work in progress, but is
> +probably better than nothing for now.
> +
> +MozReview-Commit-ID: 5X7LSYoQjid
> +

I made an updated version of the same basic approach.  If you'd prefer a different approach entirely, I need more details on what you'd like.

Comment 14

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Olli Pettay [:smaug] from comment #11)
> Do we return same values from .data as other browsers which have shipped the
> attribute? If not, then it might break sites. 
> And existence of .data might lead to different code paths which are so far
> tested only on some other browsers.

In theory, yes.  I don't know if it's used widely yet.  We certainly don't return the same values as other browsers in all cases, because there's no standard on when to set the .data property at all.  If our approach is to put it behind a pref unless it's definitely safe, then it makes sense to have a pref.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Masayuki requested to have it under a pref for a cycle, so enable in Nightly only initially.
And pref has also the advantage that if the feature is breaking web sites, it is easy switch it off with a hot fix.

Comment 16

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

Comment on attachment 8897081 [details]
Bug 998941 - Implement InputEvent.data;

https://reviewboard.mozilla.org/r/168366/#review173982

> We could, but do you have any specific reason to think this would break feature-detection?

I have no any specific idea. However, I've experienced a lot changing something which shouldn't cause any problem caused breaking something with odd implementations.

And like smaug said, we should have a way to kill the feature if we'd need to disable it before release.

> I moved the member.  Unfortunately, I don't understand the rest of your comment, perhaps because I'm not familiar enough with how the transaction manager works.  Where would you like me to put the string?  As a member of nsTransactionManager?  Do you think one input event should be fired per transaction, or per edit action?  What do we do if a transaction is an aggregate of several transactions that do nontrivial things like insert and delete text, or insert in two different places?  (I don't know if we create such transactions.)

I *think* that input event should be merged per action if it's possible. However, if it's impossible, it should be allowed to be fired twice or more times per edit action.

nsTransactionManager has undo items with mUndoStack whose type is nsTransactionStack. nsTransactionStack is a subclass of nsDeque. At starting to handle edit action, editor can retrieve the size of mUndoStack in mEditorBase at the case of EditorBase::NotifyEditorObservers(). Then, FireInputEvent() can retrieve all transaction's inserted text if nsPIEditorTransaction interface has |nsString GetStringToInsert() const|.

Then, if even if there are 2 or more transactions which caused inserting string, FireInputEvent() can merge it easily. (e.g., inserting text, inserting a line break, inserting text...)

> Does appending the string always make sense?  What if there are two inserts in different places, for instance?
> 
> It's helpful to make two issues for two separate points so I can mark each point separately as fixed/dropped.

Yes, if there are some cases text is inserted to multiple places. However, at least for now, I have no idea. However, even if it's reported, using nsTransactionManager to retrieve the string to insert from FireInputEvent(), it can fire multiple input events easier.

Comment 17

[msuhr]

When will the support for data property for an InputEvent implemented?

Currently the event is triggered correctly for input elements or contenteditable host editors. But the data property is undefined

see https://developer.mozilla.org/en-US/docs/Web/API/InputEvent/data

Comment 18

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

We plan it before implementing beforeinput since beforeinput event listeners need both inputType and data or dataTransfer. Currently, I'm rewriting our editor code for implementing inputType. Then, I'll work on implementing data/dataTransfer, and finally, I'll work on implementing beforeinput. This needs really long time unfortunately, though.

Comment 19

[msuhr]

Ok, thanks for feedback

Comment 20

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d159b2bdf5565896a2f815919b637c0805dc98e9

Comment 21

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

Oh, neither Chrome nor Safari sets InputEvent.data when InputEvent.inputType is "formatBackColor", "formatFontColor" or "formatFontName". I think that we don't need to support them here because they are available only with execCommand and their data value is really complicated.

Comment 22

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=125d3af939a71384202bd482a68dcc26d4d3e3c1

Comment 23

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f549ccf3a696c3e7b45f5691b496c2ae4681be26

Comment 24

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

smaug:

I have a question. I want to add a member to RefPtr<DataTransfer> to InternalEditorInputEvent for InputEvent.dataTransfer. However, with the new member, (new AsyncEventDispatcher(aEventTargetElement, inputEvent))->RunDOMEventWhenSafe() in nsContentUtils::DispatchInputEvent() claims that Hit MOZ_CRASH(DataTransfer not thread-safe) at m:/src/xpcom/base/nsISupportsImpl.cpp:40. However, in my understanding, AsyncEventDispatcher runs in created thread, i.e., won't run in another thread. So, I guess that the MOZ_ASSERT is caused by using Runnable. Is this intentional? If so, do you have ideas to avoid this issue?

Comment 25

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

(Well, if we cannot resolve this issue simply, I think that it's enough to set InputEvent.dataTransfer only when nsContentUtils::IsSafeToRunScript() returns true and dispatch it synchronously without AsyncEventDispatcher. Perhaps, InputEvent.dataTransfer won't be used by any web apps when "input" event is caused by script, e.g., document.execCommand("paste") is called by an event listener.)

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

I think I don't have enough information. Sure AsyncEventDispatcher is single thread only, since it deals with events and eventtargets which are single thread objects.
The issue is somewhere else.

What is the stack trace when you get the assert and what kind of patch you have?

Comment 27

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

Okay, I'll post it tomorrow morning because I need to build debug build without any optimization because optimization hides actual crash point.

Comment 28

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

Ah, really sorry. This must be my patch's bug. Probably, I set uninitialized raw pointer to the new member. I'll check it tomorrow.

Comment 29

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a599392b1a8bc751ad0c14c1e5e65dc7ce8d9fcf

Comment 30

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8a95d6d0e34cc7248810ccfd37ee1f1ad75a12e

Comment 31

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9c6c8ff47cbaa2dd91f1783df961e155d683561

Comment 32

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8be96edbccc62a18fb839f88a7dcf183efb015a1

Comment 33

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

According to a Safari developer, Safari on macOS may set data value to color when inputType is formatBackColor or formatFontColor. However, they don't set it when execCommand() causes an input event. So, that we're still not sure that whether we should compute rgb() or rgba() style value with parsing given value or not. If we need to do it, looks like that the parser is written only by Rust. I'm not sure it's available from C++ code though. Anyway, I don't want to do complicated things... So, we should keep setting data to null in these cases.

On the other hand, in the cause of formatFontName, perhaps, it's okay to set given value as-is because web apps can set it directly to Element.style.fontFamily. So, just setting raw value must be enough.

Comment 34

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=33507e265a0c0a2d985ae720aad64e82c972ac51

Comment 35

[Brian Birtles (:birtles)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #33)

According to a Safari developer, Safari on macOS may set data value to color when inputType is formatBackColor or formatFontColor. However, they don't set it when execCommand() causes an input event. So, that we're still not sure that whether we should compute rgb() or rgba() style value with parsing given value or not. If we need to do it, looks like that the parser is written only by Rust. I'm not sure it's available from C++ code though. Anyway, I don't want to do complicated things... So, we should keep setting data to null in these cases.

I'm sure Emilio or someone else on layout team would be very happy to write a patch to call the rust code to parse a color if you decide you need to.

Comment 36

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd57654d9b0bac63fc72bfa32626742f54325008

Comment 37

[Emilio Cobos Álvarez (:emilio)]

(In reply to Brian Birtles (:birtles, away Feb 8, 11, 18) from comment #35)

I'm sure Emilio or someone else on layout team would be very happy to write a patch to call the rust code to parse a color if you decide you need to.

Is the existing ServoCSSParser::ComputeColor enough?

Comment 38

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

(In reply to Emilio Cobos Álvarez (:emilio) from comment #37)

(In reply to Brian Birtles (:birtles, away Feb 8, 11, 18) from comment #35)

I'm sure Emilio or someone else on layout team would be very happy to write a patch to call the rust code to parse a color if you decide you need to.

Is the existing ServoCSSParser::ComputeColor enough?

Oh, looks like it's enough (according to the comment in the header).

Is there a method to create serialized computed value from nscolor value? If so, I'm really happy!

Comment 39

[Emilio Cobos Álvarez (:emilio)]

Should be able to move the string-creation part of:

https://searchfox.org/mozilla-central/rev/e00ea598e52bbb35f8c45abf9c2eade17962bb5e/layout/style/nsComputedDOMStyle.cpp#1022

To nsStyleUtil or something, and use that.

Comment 40

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

Thank you very much!!

Comment 41

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=af7028d3ec9a6beb29ea08a2ac5ca94861212bcf

Comment 42

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=52e7c828b2443b5973260f7c5e4b531e01a9b1b9

Comment 43

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdb4b5a4190f800a407c3a078b25ecd99eea8f26

Comment 44

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

Attachment: Bug 998941 - part 1-1: Implement InputEvent.data of UI Events

InputEvent.data notifies web apps of inserting/inserted text with "beforeinput"
and "input" events. So, this is important especially for "beforeinput" event
listeners. That's the reason why we need to support this before implementing
"beforeinput" event.

This patch adds it into InputEvent and make it enabled by default.

Comment 45

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

Attachment: Bug 998941 - part 1-2: Make editor set InputEvent.data to inserting text when it sets InputEvent.inputType to "insertText" or "insertCompositionText"

This patch makes nsContentUtils::DispatchInputEvent() support to set
InputEvent.data. Whether the its value should be null or DOMString depends
on InputEvent.inputType value.

• https://rawgit.com/w3c/input-events/v1/index.html#overview
• https://rawgit.com/w3c/input-events/v1/index.html#dfn-data
• https://w3c.github.io/input-events/#overview
• https://w3c.github.io/input-events/#dfn-data

According to the draft specs, InputEvent.data should be always inserting text
when inputType is "insertText" or "insertCompositionText" (or
"insertFromCompoition" if Level 2 support is enabled).

Comment 46

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

Attachment: Bug 998941 - part 1-3: Make TextEditor (only when not HTMLEditor instance) set InputEvent.data to inserting string when InputEvent.inputType is "insertFromPaste", "insertFromDrop" or "insertReplacementText"

https://rawgit.com/w3c/input-events/v1/index.html#dfn-data
https://w3c.github.io/input-events/#dfn-data

Both Input Events Level 1 and Level 2 declare that InputEvent.data should be
set to inserting string only on TextEditor when InputEvent.inputType is
"insertFromPaste", "insertFromPasteAsQuotation", "insertFromDrop",
"insertTranspose", "insertReplacementText" or "insertFromYank".

Currently, we support only "insertFromPaste", "insertFromDrop",
"insertReplacementText". Therefore, this patch makes TextEditor set
EditorBase::mEditActionData::mData only for them (and the instance is not
HTMLEditor's).

Comment 47

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

Attachment: Bug 998941 - part 1-4: Make editor set InputEvent.data to "ltr" or "rtl" when InputEvent.inputType is "formatSetBlockTextDirection"

When InputEvent.inputType is "formatSetBlockTextDirection" or
"formatSetInlineTextDirection", InputEvent.data value should be one of
"ltr", "rtl", "auto" or "null".
https://rawgit.com/w3c/input-events/v1/index.html#dfn-data
https://w3c.github.io/input-events/#dfn-data

We only supports "ltr" and "rtl" when user switches the direction with
Accel + Shift + X. Therefore this patch makes EditorBase set the data
to "ltr" or "rtl".

Oddly, with synthesizing the shortcut keys, the command is not executed
properly in the automated test. Therefore, this patch dispatches the
command directly.

Comment 48

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

Attachment: Bug 998941 - part 1-5: Make HTMLEditor set InputEvent.data when InputEvent.inputType is "insertLink"

Although neither Chrome nor Safari does not set InputEvent.data value when
InputEvent.inputType is "insertLink", but it's easy to implement. Therefore,
this patch implements it as declaration of Input Events.

This patch sets the value to raw href attribute value because we create
<a> element without absolute URI when web apps call execCommand("createLink")
with relative URI.

Comment 49

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

Attachment: Bug 998941 - part 1-6: Make HTMLEditor set InputEvent.data when InputEvent.inputType is "fontName"

Although neither Chrome nor Safari does not set InputEvent.data value when
InputEvent.inputType is "fontName", but it's easy to implement. Therefore, this
patch implements it as declaration of Input Events.

This patch uses given value as-is. Perhaps, this shouldn't cause any problems
because such value can be set to Element.style.fontFamily without any changes.

Note that automated test will be added into WPT later.

Comment 50

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

Attachment: Bug 998941 - part 1-7: Make HTMLEditor set InputEvent.data to serialized color value when InputEvent.inputType is "formatBackColor" or "formatForeColor"

Although neither Chrome nor Safari does not set InputEvent.data when the event
is caused by document.execCommand() with backColor, foreColor nor
hiliteColor, Safari supports styling color with touchbar and in that case,
Safari sets it (*1).

Additionally, currently Safari uses rgb() to represents a color value and
using same rule to serializing color value for CSS OM matches Safari's behavior
and can represent any valid color values.

This patch makes given color value parsed and then serialized with same code
in style system. If the value is currentColor or invalid value, sets given
value as-is for forward compatibility.

Note that automated tests will be added into input-events-exec-command.html
by the last patch.

1. https://github.com/w3c/input-events/issues/94#issuecomment-461061517

Comment 51

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

Attachment: Bug 998941 - part 2-1: Implement InputEvent.dataTransfer declared by Input Events

InputEvent.dataTransfer is declared by Input Events Level 1 and Level 2 (i.e.,
not UI Events). It's necessary for "beforeinput" event on contenteditable
elements because of with some InputEvent.inputType values on contenteditable,
InputEvent.dataTransfer is used instead of InputEvent.data.

According to the Chrome's behavior, if InputEvent.dataTransfer is created by
web apps, the DataTransfer object is mutable. Otherwise, i.e., the event
represents user input, the DataTransfer object is read only. We should follow
this behavior.

This is enabled by default.

Comment 52

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

Attachment: Bug 998941 - part 2-2: Make HTMLEditor set InputEvent.dataTransfer when InputEvent.inputType is "insertFromPaste", "insertFromDrop" or "insertReplacementText"

InputEvent.dataTransfer should be set to non-null when InputEvent.inputType
is "insertFromPaste", "insertFromDrop" or "insertReplacementText" and
editor is an HTMLEditor instance:
https://rawgit.com/w3c/input-events/v1/index.html#dfn-data
https://w3c.github.io/input-events/#dfn-data

("insertTranspose" and "insertFromYank" are not currently supported on Gecko.)

This patch makes nsContentUtils::DispatchInputEvent() take dataTransfer value
and EditorBase set it via AutoEditActionDataSetter like data value.

However, we need to create other constructors of DataTransfer to create its
read-only instances initialized with nsITransferable or nsAString. This will
be implemented by the following patch.

Comment 53

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

Attachment: Bug 998941 - part 2-3: Create new constructors of DataTransfer to set only plain text or nsITransferable

DataTransfer should have more 2 constructors. One takes |const nsAString&| and
sets its data to the string. The other takes |nsITransferable*| and stores it
in its member for filling each data of DataTransferItem when it's retrieved.

Comment 54

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

Attachment: Bug 998941 - Update web platform tests for InputEvent.data and InputEvent.dataTransfer

Comment 55

[Olli Pettay [:smaug][[e-mail]]]

Ah, I found https://github.com/w3c/uievents/issues/139, that clarifies "" vs null.

Comment 56

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

(In reply to Olli Pettay [:smaug] (massive needinfo queue, ping on IRC on anything urgent) from comment #55)

Ah, I found https://github.com/w3c/uievents/issues/139, that clarifies "" vs null.

Yeah, sorry, I forgot to mention it with comment.

Comment 57

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

Makoto-san: ping for "part 2-2".

Comment 58

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7773539f3fdc
part 1-1: Implement InputEvent.data of UI Events r=smaug
https://hg.mozilla.org/integration/autoland/rev/7eae0724c0aa
part 1-2: Make editor set InputEvent.data to inserting text when it sets InputEvent.inputType to "insertText" or "insertCompositionText" r=smaug,m_kato
https://hg.mozilla.org/integration/autoland/rev/ebd32851bb24
part 1-3: Make TextEditor (only when not HTMLEditor instance) set InputEvent.data to inserting string when InputEvent.inputType is "insertFromPaste", "insertFromDrop" or "insertReplacementText" r=smaug,m_kato
https://hg.mozilla.org/integration/autoland/rev/cfd766ac9963
part 1-4: Make editor set InputEvent.data to "ltr" or "rtl" when InputEvent.inputType is "formatSetBlockTextDirection" r=smaug,m_kato
https://hg.mozilla.org/integration/autoland/rev/d392cc639292
part 1-5: Make HTMLEditor set InputEvent.data when InputEvent.inputType is "insertLink" r=smaug,m_kato
https://hg.mozilla.org/integration/autoland/rev/f4039b1e1fb6
part 1-6: Make HTMLEditor set InputEvent.data when InputEvent.inputType is "fontName" r=smaug,m_kato
https://hg.mozilla.org/integration/autoland/rev/ef76c4985c1a
part 1-7: Make HTMLEditor set InputEvent.data to serialized color value when InputEvent.inputType is "formatBackColor" or "formatForeColor" r=smaug,m_kato,emilio
https://hg.mozilla.org/integration/autoland/rev/5b4aa09a20df
part 2-1: Implement InputEvent.dataTransfer declared by Input Events r=smaug
https://hg.mozilla.org/integration/autoland/rev/b259d7a34058
part 2-2: Make HTMLEditor set InputEvent.dataTransfer when InputEvent.inputType is "insertFromPaste", "insertFromDrop" or "insertReplacementText" r=smaug,m_kato
https://hg.mozilla.org/integration/autoland/rev/14aa08270157
part 2-3: Create new constructors of DataTransfer to set only plain text or nsITransferable r=smaug
https://hg.mozilla.org/integration/autoland/rev/e726a088a06c
Update web platform tests for InputEvent.data and InputEvent.dataTransfer r=smaug

Comment 59

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/15457 for changes under testing/web-platform/tests

Comment 60

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 61

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/7773539f3fdc
https://hg.mozilla.org/mozilla-central/rev/7eae0724c0aa
https://hg.mozilla.org/mozilla-central/rev/ebd32851bb24
https://hg.mozilla.org/mozilla-central/rev/cfd766ac9963
https://hg.mozilla.org/mozilla-central/rev/d392cc639292
https://hg.mozilla.org/mozilla-central/rev/f4039b1e1fb6
https://hg.mozilla.org/mozilla-central/rev/ef76c4985c1a
https://hg.mozilla.org/mozilla-central/rev/5b4aa09a20df
https://hg.mozilla.org/mozilla-central/rev/b259d7a34058
https://hg.mozilla.org/mozilla-central/rev/14aa08270157
https://hg.mozilla.org/mozilla-central/rev/e726a088a06c

Comment 62

[Pascal Chevrel:pascalc]

I see in the commits that this is enabled by default:

+// Whether InputEvent.data is enabled.
+pref("dom.inputevent.data.enabled", true);

Yet comment #15 was saying that it should be behind a nightly flag and not ride the trains so as that we can evaluate the webcompat impact:
(In reply to Olli Pettay [:smaug] (PTO-ish Feb 16-23) from comment #15)

Masayuki requested to have it under a pref for a cycle, so enable in Nightly
only initially.
And pref has also the advantage that if the feature is breaking web sites,
it is easy switch it off with a hot fix.

Masayuki, can you clarify if this is meant to ship in 67? Thanks

Comment 63

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 64

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

(In reply to Pascal Chevrel:pascalc from comment #62)

I see in the commits that this is enabled by default:

+// Whether InputEvent.data is enabled.
+pref("dom.inputevent.data.enabled", true);

Yet comment #15 was saying that it should be behind a nightly flag and not ride the trains so as that we can evaluate the webcompat impact:
(In reply to Olli Pettay [:smaug] (PTO-ish Feb 16-23) from comment #15)

Masayuki requested to have it under a pref for a cycle, so enable in Nightly
only initially.
And pref has also the advantage that if the feature is breaking web sites,
it is easy switch it off with a hot fix.

Masayuki, can you clarify if this is meant to ship in 67? Thanks

Yes, I currently plan to ship it at 67 because we're following the other browsers right now. Those new attributes are really important when we ship beforeinput and we need to ship the new event as soon as possible. On the other hand, if some major web apps used these new attributes for feature detection, we'd stop shipping the release at least at 67.

Comment 65

[Eric Shepherd [:sheppy]]

For documentation purposes:

We need to remove the note from the first example on KeyboardEvent.key that explains the errors in Firefox because we don't have the data property, as well as update BCD.

Comment 66

[Chris Mills [:cmills]]

Further notes for MDN writers:

I've added a note about these additions to https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/67#APIs.

The BCD needs updating, and we also need to clean up the property pages a bit; add examples, etc.

Comment 67

[Chris Mills [:cmills]]

OK, documentation done.

I've removed the bit about Firefox not supporting InputEvent.data from the KeyboardEvent.key page as sheppy details above.

I've added compat data to BCD:
https://github.com/mdn/browser-compat-data/pull/4065

I've also added a demo to the data and dataTransfer pages:
https://developer.mozilla.org/en-US/docs/Web/API/InputEvent/dataTransfer
https://developer.mozilla.org/en-US/docs/Web/API/InputEvent/data

Let me know if you think this needs anything else. Thanks!