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

Set coalesced events isTrusted flag to false #187

Closed
NavidZ opened this issue Mar 8, 2017 · 10 comments · Fixed by #189
Closed

Set coalesced events isTrusted flag to false #187

NavidZ opened this issue Mar 8, 2017 · 10 comments · Fixed by #189

Comments

@NavidZ
Copy link
Member

NavidZ commented Mar 8, 2017

@dtapuska @smaug---- @RByers
I was wondering if it is alright to leave the isTrusted flag to be false instead of setting it from the original event. I believe since these coalesced events are not the events to be fired or maybe handled by themselves having the isTrusted bit doesn't convey much of a meaning.

@NavidZ NavidZ changed the title Coalesced events isTrusted flag Set coalesced events isTrusted flag to false Mar 8, 2017
@smaug----
Copy link
Contributor

smaug---- commented Mar 9, 2017

If UA has created the event, it should have isTrusted=true. Events created explicitly from JS have isTrusted=false. (element.click() is a special case for these rules)

@dtapuska
Copy link

dtapuska commented Mar 9, 2017

It creates special rules around dispatch. For example if you redispatch an trusted event then you need to walk all the coalesced events and update them to match isTrusted of the parent to be false no? I wondered if we could avoid this by just saying all the fields on the base Event aren't really useful.

@smaug----
Copy link
Contributor

of the parent? Why would you need to update anything? The coalesced events are there without any dispatching. One could also just put some expando js property to an event with some trusted events and those wouldn't get updated.

@dtapuska
Copy link

dtapuska commented Mar 9, 2017

Because then you'd be in a situation whereby
parent.isTrusted == false
parent.getCoalescedEvents(0).isTrusted == true

And you can't really determine the case whereby a person held onto coalesced events and re-dispatched them at you. isTrusted communicates whether the UA really dispatched this event or not. And if we are populating the event it should really convey the correct meaning.

I'd honestly wonder if we didn't reuse the Events object and just had a dictionary of values for the coalesced events then we don't get into all this mess of saying what fields are applicable and aren't. For example why would someone think that offsetX/offsetY wouldn't work; well you'd have to read the spec that there is no target set for the event and those properties are dependent on having a target.

@smaug----
Copy link
Contributor

smaug---- commented Mar 9, 2017

yeah, dictionaries might make this simpler.

Trying to recall why events are used...

@smaug----
Copy link
Contributor

Iif web app wants to handle those coalesced events after all, it might be simpler to reuse PointerEvents. But then, if values in the events are bogus...

@dtapuska
Copy link

dtapuska commented Mar 9, 2017

On the converse of my point... I could do something like:

var a = event.getCoalescedEvent(0);
foo.dispatchEvent(event);
console.log("isTrusted " + a.isTrusted);

It is odd that the isTrusted changes even though you didn't do something to the child object itself. That is why I had a discussion with Navid about whether we just say isTrusted is bogus like the rest of the fields we said were bogus, target, relatedTarget, bubbles, stopPropogation, etc...

But to me this feels like a wart we are adding to the web platform possibly.
@RByers any opinion?

I know we did think that if someone wanted to change their code from using the "unioned" events to using the coalescedEvents then they'd just add a for loop and that was the thought around using Events but I don't know if that makes a whole lot of sense anymore.

@RByers
Copy link
Contributor

RByers commented Mar 9, 2017

I don't have a strong opinion on the bigger picture question of whether it's better to use Event instances or dictionaries with a subset of the properties - they each have their tradeoffs. We discussed it at TPAC and I believe @smaug---- said he preferred Events which is why we chose that. And I know this shouldn't really matter relative to the other concerns, but I don't think any of us really wanted to have to maintain duplicate definitions in the spec including for properties defined in other specs (clientX etc).

IMHO it's not really worth re-litigating that decision now. The getCoalescedEvents API is already a relatively niche use case, I think it's unlikely that these details we're worried about are going to impact any developer in practice.

IMHO we don't get much real benefit in practice by clearing the isTrusted bit on dispatch (the only real protection against abuse is in extension scenarios, since there's no protecting against malicious code running in the same context). I'd personally just go with @smaug----'s suggestion of propagating isTrusted from the parent event, and not worrying about trying to clear it on re-dispatch. Developers already need to be thoughtful about where an Event object is coming from when making important decisions based on isTrusted, this is just one more case (which IMHO is extremely unlikely to matter in practice).

@dtapuska
Copy link

dtapuska commented Mar 9, 2017

If you are trying to do this at object creation time you can't (inheriting from your parent). Because isTrusted is always initialized to true according to the spec event for; isTrusted is supposed to be true until they are dispatched (https://dom.spec.whatwg.org/#interface-eventtarget).

Whereas I was just saying lets default it to false always and forget about it. So really we are debating whether the coalesced events should have isTrusted be true or false. I guess the DOM spec always have Events default to true so I'm fine matching that (but I've never really agreed with that).

@RByers
Copy link
Contributor

RByers commented Mar 9, 2017

I see. Yeah either of those options is fine with me.

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

Successfully merging a pull request may close this issue.

4 participants