Closed Bug 998941 Opened 10 years ago Closed 5 years ago

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

Categories

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

Product:
Core
Component:
DOM: Events
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox67 --- fixed

People

(Reporter: crimsteam, Assigned: masayuki)

References

(
URL
)

Details

(Keywords: dev-doc-complete)

Attachments

(12 files)

Bug 998941 - Implement InputEvent.data;
59 bytes, text/x-review-board-request
Details
Bug 998941 - part 1-1: Implement InputEvent.data of UI Events
47 bytes, text/x-phabricator-request
Details | Review
Bug 998941 - part 1-2: Make editor set InputEvent.data to inserting text when it sets InputEvent.inputType to "insertText" or "insertCompositionText"
47 bytes, text/x-phabricator-request
Details | Review
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"
47 bytes, text/x-phabricator-request
Details | Review
Bug 998941 - part 1-4: Make editor set InputEvent.data to "ltr" or "rtl" when InputEvent.inputType is "formatSetBlockTextDirection"
47 bytes, text/x-phabricator-request
Details | Review
Bug 998941 - part 1-5: Make HTMLEditor set InputEvent.data when InputEvent.inputType is "insertLink"
47 bytes, text/x-phabricator-request
Details | Review
Bug 998941 - part 1-6: Make HTMLEditor set InputEvent.data when InputEvent.inputType is "fontName"
47 bytes, text/x-phabricator-request
Details | Review
Bug 998941 - part 1-7: Make HTMLEditor set InputEvent.data to serialized color value when InputEvent.inputType is "formatBackColor" or "formatForeColor"
47 bytes, text/x-phabricator-request
Details | Review
Bug 998941 - part 2-1: Implement InputEvent.dataTransfer declared by Input Events
47 bytes, text/x-phabricator-request
Details | Review
Bug 998941 - part 2-2: Make HTMLEditor set InputEvent.dataTransfer when InputEvent.inputType is "insertFromPaste", "insertFromDrop" or "insertReplacementText"
47 bytes, text/x-phabricator-request
Details | Review
Bug 998941 - part 2-3: Create new constructors of DataTransfer to set only plain text or nsITransferable
47 bytes, text/x-phabricator-request
Details | Review
Bug 998941 - Update web platform tests for InputEvent.data and InputEvent.dataTransfer
47 bytes, text/x-phabricator-request
Details | Review 
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).
Depends on: 993253
Version: 31 Branch → Trunk
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.
Blocks: 970802
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All
(w3.org/TR is for trash, always look at the latest version of a spec.)
URL: http://www.w3.org/TR/uievents/#widl-C... → https://w3c.github.io/uievents/#inter...
Are you sure ? http://www.w3.org/TR/uievents/ even seems more recent at the moment...
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.
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 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.
I'll check the automated tests. However, for uplifting to upstream, shouldn't they be in separated patch?
Assignee: nobody → ayg
Status: NEW → ASSIGNED
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.
(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 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.
(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.
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 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.
Assignee: ayg → nobody
Status: ASSIGNED → NEW
Attachment #8897081 - Flags: review?(masayuki)
Summary: Implement D3E InputEvent.data → Implement InputEvent.data and InputEvent.dataTransfer
Summary: Implement InputEvent.data and InputEvent.dataTransfer → [Input Events] Implement InputEvent.data and InputEvent.dataTransfer
Priority: -- → P3
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
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.
Ok, thanks for feedback
Assignee: nobody → masayuki
Status: NEW → ASSIGNED

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.

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?

Flags: needinfo?(bugs)

(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.)

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?

Flags: needinfo?(bugs)

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

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.

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.

(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.

(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?

(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!

Flags: needinfo?(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.

Flags: needinfo?(emilio)

Thank you very much!!

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.

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

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).

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).

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.

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.

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.

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

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.

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.

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.

(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.

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

Flags: needinfo?(m_kato)
Pushed by masayuki@d-toybox.com:
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/15457 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Flags: needinfo?(m_kato)

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

Flags: needinfo?(masayuki)

(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.

Flags: needinfo?(masayuki)

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.

No longer depends on: 1533989
Regressions: 1533989

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.

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!

Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.