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

Original Event as attribute #9

Closed
nicjansma opened this issue Aug 15, 2018 · 11 comments
Closed

Original Event as attribute #9

nicjansma opened this issue Aug 15, 2018 · 11 comments

Comments

@nicjansma
Copy link

I could see users wanting to correlate the EventTimings within the context of what is happening in the page.

For example, if we get a click EventTiming, where was the user clicking?

Would it be possible to get the original Event as an attribute of the PerformanceEventTiming?

@npm1 npm1 added this to the Level 2 milestone Dec 12, 2018
@npm1 npm1 removed this from the Level 2 milestone Apr 24, 2019
@npm1
Copy link
Collaborator

npm1 commented Apr 24, 2019

Revisiting this based on feedback from Gilles from Wikimedia, it might make sense to expose this right away to make this API more actionable. Is there any preference on exposing the Event vs the EventTarget?

@tdresser
Copy link
Collaborator

Shadow dom makes this a bit complicated. We'd still need a list of targets, or something along those lines, as the target can change during event dispatch.

This is true of a number of fields on Event - in fact, I think the majority of the fields/methods don't make sense outside of event dispatch.
That isn't necessarily a hard blocker, but it does make me unsure this is the right approach.

@gi11es
Copy link

gi11es commented Apr 26, 2019

In that case, should we make a separate task to expose just the list of targets? That's the most critical field to me, to figure out what node users interacted with as part of the event. Which seems to be what Nic was also interested in.

All the other data inside the event object that isn't already exposed by the API is a nice-to-have and could be revisited in a second version of the API if there is demand for it.

@npm1
Copy link
Collaborator

npm1 commented Apr 26, 2019

Yea we could expose the list of all the targets... Alternatively, for simplicity we could report the event.target as computed at the time we finish dispatching the Event. For shadow DOM, this means the shadow host is reported as the target. I think this is a good balance between simple API shape, easy implementation, and good value. Thoughts on this?

@gi11es
Copy link

gi11es commented Apr 26, 2019

That sounds fair to me, it would be the same target you would capture if you were catching events at the document level.

@tdresser
Copy link
Collaborator

I think this would significantly reduce the usefulness of the API in the context of web components, which would be quite unfortunate.

I'd prefer to initially ship without any info on target than rush to ship with a single target.

@gi11es
Copy link

gi11es commented Jun 18, 2019

To give you an update on this, I ended up implementing a workaround by tracking all clicks on the page separately (since click was by far the biggest offender in our event timing entries with long processing times). The event timestamps match, which makes it convenient to match. With this information I was able to track down 3 performance issues in our code:

https://phabricator.wikimedia.org/T225946
https://phabricator.wikimedia.org/T226025
https://phabricator.wikimedia.org/T226023

Now, my workaround isn't ideal, because it introduces a lot of overhead, recording and reporting every click in case one of them is a slow one that will show up in Event Timing. I would rather avoid that and have the information there and then in the Event Timing entry with the event being exposed.

@tdresser
Copy link
Collaborator

Thanks for the examples! Fixing this is definitely on our backlog, but we're still working out how this (and other APIs) should relate to shadow DOM.

@npm1
Copy link
Collaborator

npm1 commented Jun 18, 2019

Great to hear that it was possible to use it, even if with a lot of difficulty! Personally I would be fine exposing the final event.target for now. This will not break any shadow DOM encapsulation. It won't be useful for websites that wrap everything in a giant web component but many web components folks have not yet even acknowledged that this is a thing and thus aren't yet receptive to exposing internals of shadow DOMs to enable performance measurements in this scenario.

@tdresser
Copy link
Collaborator

You're right that it probably is the pragmatic approach. If we get feedback that this limitation is negatively impacting usage of the API, we can revisit.

@npm1
Copy link
Collaborator

npm1 commented Aug 10, 2020

We currently expose the final target and it does not look like we can expose shadow DOM is a Window-level API so we can't expose more targets than that. Hence I'm considering this closed, let me know if you think there's more to be done here.

@npm1 npm1 closed this as completed Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants