Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should EventInit contain 'timestamp'? #76

Open
RByers opened this issue Sep 18, 2015 · 17 comments
Open

Should EventInit contain 'timestamp'? #76

RByers opened this issue Sep 18, 2015 · 17 comments
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest topic: events

Comments

@RByers
Copy link
Contributor

RByers commented Sep 18, 2015

https://dom.spec.whatwg.org/#dictdef-eventinit

If we change (as planned) the semantics of timestamp to be the platform time (as a DOMHighResTimestamp), then should script be able to supply arbitrary values for it?

Relevant to blink change here: https://codereview.chromium.org/1352523002/#msg9

@annevk
Copy link
Member

annevk commented Sep 21, 2015

Why?

@RByers
Copy link
Contributor Author

RByers commented Sep 21, 2015

Sorry, let's back up a second, I may be jumping to conclusions based on partial discussions elsewhere. Filed #80 to track the larger issue.

If we decide to do that, then timestamp is no longer an inherent property of the event, but data supplied by the thing that created the event, and as you've argued elsewhere (TouchEvent.pageX I think?) all such properties should be settable via the constructor.

Eg. imagine a PointerEvents polyfill that generates pointermove events from mousemove and touchmove. If timestamp represents the time the input was first received by the OS, then the polyfill should be able to propagate the value from a mousemove into a generated pointermove. There are other similar non-polyfill scenarios involving any synthetic input event.

@annevk
Copy link
Member

annevk commented Sep 22, 2015

Interesting. I'm no longer sure whether MouseEvent.prototype.offsetX needs to be able to be set or should just be computed on creation time, but your timestamp scenario does somewhat argue for making it settable, or at least for creating an event based on an existing one.

@RByers
Copy link
Contributor Author

RByers commented Sep 22, 2015

Thanks. In addition to copying the timestamp from other events, there are at least some small scenarios where you really want to synthesize a timestamp. Eg. Android and now iOS 9 align input to vsync through position/time interpolation. It's reasonable for a library to want to do something similar taking, say, a 100hz input event source and converting it to a 60hz one phase-locked to requestAnimationFrame. Doing that requires generating events with timestamp values that are interpolated (or perhaps even extrapolated) from the timestamps of other events.

But I think we can wait until we've done the spec updates for #80 to decide exactly what we want to do here.

@annevk
Copy link
Member

annevk commented Aug 16, 2016

Do we still want this?

@RByers
Copy link
Contributor Author

RByers commented Aug 25, 2016

Seems like this is blocked on #23, right? If the ultimate decision in #23 is that timeStamp should ALWAYS be the time the event object itself was created (i.e. pretty much useless), then there's probably no value in being able to set it by the constructor.

@annevk
Copy link
Member

annevk commented Aug 25, 2016

I guess we can wait on that.

@annevk annevk added the needs implementer interest Moving the issue forward requires implementers to express interest label Mar 16, 2017
@RByers
Copy link
Contributor Author

RByers commented Apr 17, 2017

Seems like the path for #23 is pretty clear now (just waiting on tests). Perhaps it's time to revisit this discussion?

Some related context where we have a blink bug because we were internally using the Event constructor code path which didn't have a way of specifying the timestamp.

@annevk
Copy link
Member

annevk commented Apr 17, 2017

#420 is still ongoing as far as I can tell.

Also, if we add this to the constructor, due to the way event dispatch is defined that would mean that each call site ends up defaulting this to the default value, which is not what we want, so adding this might be more involved than you think.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 24, 2017
To match the pointerevent and its corresponding
mouse and touch events time stamps we need to
have the time stamp in the constructor parameter.

Note that having the time stamp in the EventInit
dictionary is being debated as part of this issue:
whatwg/dom#76

BUG=710442

Review-Url: https://codereview.chromium.org/2834183002
Cr-Commit-Position: refs/heads/master@{#466690}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 24, 2017
To match the pointerevent and its corresponding
mouse and touch events time stamps we need to
have the time stamp in the constructor parameter.

Note that having the time stamp in the EventInit
dictionary is being debated as part of this issue:
whatwg/dom#76

BUG=710442

Review-Url: https://codereview.chromium.org/2834183002
Cr-Commit-Position: refs/heads/master@{#466690}
MXEBot pushed a commit to mirror/chromium that referenced this issue Apr 25, 2017
To match the pointerevent and its corresponding
mouse and touch events time stamps we need to
have the time stamp in the constructor parameter.

Note that having the time stamp in the EventInit
dictionary is being debated as part of this issue:
whatwg/dom#76

BUG=710442

Review-Url: https://codereview.chromium.org/2834183002
Cr-Commit-Position: refs/heads/master@{#466690}
@annevk
Copy link
Member

annevk commented Apr 25, 2017

So https://codereview.chromium.org/2834183002 landed without intent to ship? Is it not web-exposed or some such?

@majido
Copy link
Member

majido commented Jun 21, 2017

So https://codereview.chromium.org/2834183002 landed without intent to ship? Is it not web-exposed or some such?

I believe it is not web-exposed yet as it is not in the IDL file: https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/core/events/EventInit.idl

if we add this to the constructor, due to the way event dispatch is defined that would mean that each call site ends up defaulting this to the default value, which is not what we want, so adding this might be more involved than you think.

I am not sure if I understand the above correctly. The way I was thinking to do this was as follow:
Add timestamp to EventInitDic and have it default to equivalent of "performance.now()"
This means that we will use this default value when 1) create an event is used and 2) when Event constructor is invoked

AFAICT those are the two places where EventInit is used so not sure how the definition of event dispatch algorithm changes this behavior. So this is what I expect to happen:

let e = new MouseEvent(); // N = performance.now
$target1.dispatchEvent(e); // e.timestamp == N
$target2.dispatchEvent(e); // e.timestamp == N

If this makes sense I can start a patch or event better fold this change in #420 (I think using EventInit default value makes #420 wording simpler)

@RByers
Copy link
Contributor Author

RByers commented Jun 21, 2017

So https://codereview.chromium.org/2834183002 landed without intent to ship? Is it not web-exposed or some such?

I believe it is not web-exposed yet as it is not in the IDL file: https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/core/events/EventInit.idl

Right, the "constructor" mentioned there was C++ code. This was just an internal refactoring to fix a bug we had where timestamps weren't initialized propertly for UA-generated events. It might ultimately share C++ code with a change for this issue, but is otherwise entirely unrelated.

@annevk
Copy link
Member

annevk commented Jun 28, 2017

@majido shouldn't timestamp reflect when the event happened, not when it's created/dispatched?

@majido
Copy link
Member

majido commented Jun 28, 2017

That is correct. I think the right behavior should be as follow:

  1. For synthetic events (those constructed from JS), we should allow the timestamp to be provided to the constructor via EventInitDic as suggested by this issue. But the creation time is a sensible default value.
  2. For events created by user agent, the actual occurrence time should be used. And when that is not available, then once again I think falling back to using the creation time is a sensible choice.

I don't think we need to use dispatch time in any case. I might have not be precise before.

@annevk
Copy link
Member

annevk commented Jun 28, 2017

Okay, so the problem is that we don't make a distinction between synthetic and non-synthetic at creation. So unless we explicitly pass a time in all places that define that the user agent is to dispatch an event, we end up introducing a regression of sorts.

@annevk
Copy link
Member

annevk commented Jun 30, 2017

@domenic pointed out that we do. https://dom.spec.whatwg.org/#concept-event-fire and https://dom.spec.whatwg.org/#concept-event-create don't go through the constructor and those are the entry points for other standards. (I don't know if that's a good thing long term solution, but it's not a blocker here.)

@annevk annevk added the addition/proposal New features or enhancements label Apr 11, 2019
@jakearchibald
Copy link
Collaborator

I just ran into this. At Shopify, we want to take an event that happened in the document, and represent it as-real-as-possible in a worker. Being able to set the timeStamp would be useful here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest topic: events
Development

No branches or pull requests

4 participants