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

Pointerevent extension #346

Closed
3 of 5 tasks
NavidZ opened this issue Feb 19, 2019 · 26 comments
Closed
3 of 5 tasks

Pointerevent extension #346

NavidZ opened this issue Feb 19, 2019 · 26 comments

Comments

@NavidZ
Copy link

NavidZ commented Feb 19, 2019

こんにちはTAG!

I'm requesting a TAG review of:

Further details (optional):

You should also know that...

[please tell us anything you think is relevant to this review]

We'd prefer the TAG provide feedback as (please select one):

  • open issues in our Github repo for each point of feedback
  • open a single issue in our Github repo for the entire review
  • leave review feedback as a comment in this issue and @-notify [@NavidZ @EiraGe]
@NavidZ NavidZ changed the title Pointerevent getPredictedPoints Pointerevent extension Feb 19, 2019
@alice alice self-assigned this Feb 26, 2019
@slightlyoff
Copy link
Member

Any updates?

@NavidZ
Copy link
Author

NavidZ commented Mar 14, 2019

@slightlyoff any updates on our side or the TAG folks?
I assume we provided all the information required. But let us know if there is anything missing.

@EiraGe
Copy link

EiraGe commented Mar 21, 2019

Hi @alice, can we get an expedited review on this? We are waiting for the review to ship the predicted events api in chrome.

@annevk
Copy link
Member

annevk commented Mar 22, 2019

How does this feature interact with shadow trees?

What is the exact modification of the event dispatch algorithm required and where is the monkey patch specified?

The description of the two methods lacks the detail to lead to interoperable implementations around these questions, as far as I can tell.

@NavidZ
Copy link
Author

NavidZ commented Mar 22, 2019

@annevk can you explain a little more.
You mean how does the predicted points interact with the shadow trees? What does this have to do with that at all? GetPredictedPoints API doesn't change anything at all regarding the dispatch algorithm.

@annevk
Copy link
Member

annevk commented Mar 22, 2019

@NavidZ say the target node is inside a shadow tree. How is the target attribute of the events returned by these two new methods updated to ensure the shadow tree does not leak?

@NavidZ
Copy link
Author

NavidZ commented Mar 25, 2019

@annevk this spec does not specify when to set those targets. It just guarantees that they are the same as the event's target. It is up to UAs when to set them. For example in Chrome we have this lazy approach that we only set this when it is accessed. But regardless of that, the guarantee that the target of the events returned by these methods are the same as the target of the dispatched event should prevent that leak. WDYT?

@annevk
Copy link
Member

annevk commented Mar 26, 2019

You need to define the actual architecture that makes that work. E.g., define that these events hold a pointer to some parent and redefine the target attribute's getter so it can sometimes return the some parent's target instead. That will make it possible to verify that it's safe and will ensure that other extensions don't clash with it. The actual implementation can still be different from that, as long as it's black-box indistinguishable.

@NavidZ
Copy link
Author

NavidZ commented Mar 26, 2019

@annevk I haven't seen something like that around.
Would that be enough to just explain this logic in plain text in this spec? if not do you happen to have a similar example to this somewhere else so I can see what they did and use the same format?

@annevk
Copy link
Member

annevk commented Mar 26, 2019

I'm not sure what you mean. I'd expect this to require changes to the DOM Standard as well (e.g., you're proposing changes to Event's target attribute's getter).

It also seems this would potentially affect composedPath() in weird ways (or perhaps that's forwarded too?).

A similar thing is how Event objects have a relatedTarget internal slot that gets adjusted in various ways and other specifications defining subclasses of Event use to define some of their features.

@NavidZ
Copy link
Author

NavidZ commented Mar 26, 2019

FYI @smaug---- @patrickhlauke

The coalesced events or predicted events aren't really dispatched events. We wanted to avoid defining a new structure to avoid extra complexity. So we used the same event structure for simplicity and defined a bunch of attributes in this spec to be no-op (such as stopPropagation) or return default value.

But at the same time I don't want to just add all sort of hooks which makes the specification all more complex.
Do you think changing the type of these events to some other newly defined type is easier then than just adding all sort of hooks in the DOM spec? Obviously the new type will not have target or path() or composedPath or anything. Again the goal is to not over complicate this and add hooks in other specs that are only used by this single spec.

@annevk
Copy link
Member

annevk commented Mar 26, 2019

If these are only meant to carry some data using a dictionary would be a lot better, and likely easier, yes.

@NavidZ
Copy link
Author

NavidZ commented Mar 26, 2019

Yeah. Everything is just a value except offsetX/Y attributes which are calculated with respect to the target. So technically when the target of the dispatched event changes (like in shadow DOM cases) the offset of these coalesced/predicted points also needs to be recalculated with respect to the new target.

@annevk
Copy link
Member

annevk commented Mar 26, 2019

In that case it sounds like you still need some kind of more complicated setup. (But not that complicated. Objects have a pointer to the event and some of their getters do some calculations based on the Event's internal target slot.)

@smaug----
Copy link

I thought the idea was that one could handle coalesced events just like normal events, if needed, even if prototype was modified or so.
But then, yeah, path isn't useful there anyhow.

@alice
Copy link

alice commented May 9, 2019

Apologies for taking so long to get to this! We are in the process of trying to refine our processes to get quicker at working through our backlog, plus I missed the request to expedite.

Unfortunately I found this somewhat difficult to review. An explainer would help greatly - the spec has only minimal explanation of the problem these new APIs are intending to solve, which means I'm having to somewhat guess at the context, which also makes the review more time consuming for me.

In particular, I couldn't figure out from the spec what the purpose of getPredictedEvents() is; the introduction only mentions getCoalescedEvents(). When and why would I call this method? Are the predicted events computed on demand or are they pre-computed before the pointer event is fired? If the latter, is it possible to opt out to avoid unnecessary work being done? If the former, why is the browser better able to perform this computation than a javascript library, which has access to the same information via getCoalescedEvents()?

@NavidZ
Copy link
Author

NavidZ commented May 13, 2019

@alice I have updated the introduction to explain the later addition of predicted events and pointerrawupdate to the introduction as well. I also cleared up the getCoalescedEvents explanation as well. Sorry that the introduction was missed as that section is probably going to be removed/rewritten once merged to the pointerevents specification.

@slightlyoff
Copy link
Member

the demo link is a 404; perhaps this is the right location?

https://eirage.github.io/Prediction/PredictionDemo.html

@EiraGe
Copy link

EiraGe commented May 17, 2019

Yeah, sorry that I update the demo link recently....

@lknik lknik self-assigned this Jun 12, 2019
@lknik
Copy link
Member

lknik commented Jun 12, 2019

Hi. Following a 12/06 telecon I filled a thing in your repo.

Furthermore, if you feel like there are no particular specific issues resulting fro this extension, perhaps at least link to considerations in the extended spec?

@patrickhlauke
Copy link

Thank you @lknik - we're hoping to merge the extension into what will become PE3 shortly, and will make sure to consider the implications for privacy/security of new features.

@lknik
Copy link
Member

lknik commented Jun 14, 2019

Sure. Any information about the plan for PE3 would be welcome, too. I still need to look at it all.
In the meantime, if this is meant to be a self-contained review, perhaps adding the consideretions in would be a good idea anyway?

@NavidZ
Copy link
Author

NavidZ commented Jun 24, 2019

We believe we addressed major concerns from TAG in the latest version. There is also a brief explainer for the features in the repository. Feel free to file more issues against the PointerEvent repo if you want clarification on more stuff.

@cynthia
Copy link
Member

cynthia commented Jun 26, 2019

Thanks a lot! The explainer does make the intent and constraints a lot more clear. We'll hopefully discuss this in today's call.

@alice
Copy link

alice commented Jun 26, 2019

We discussed this in our telecon today and we're happy with the use case and the broad design here.

Happy to close this, but just some parting thoughts:

  • We have some mild concern that the naming around getCoalescedEvents uses a word which is uncommon and difficult to spell, and is likely to be difficult for folks whose English vocabulary is not advanced for whatever reason. Something like getBundledEvents might be easier for more people to understand.

  • The spec text seems a bit rough, with a lot of long, hard to read paragraphs and some ReSpec warnings, and the editors would presumably want to clean it up at some point. We mention this because it made the review more difficult, and presumably implementers would also find it difficult to digest.

Thank you for your patience through this review process, and for your hard work on this API.

@alice alice closed this as completed Jun 26, 2019
@NavidZ
Copy link
Author

NavidZ commented Jun 26, 2019

Thanks @alice.

Regarding the ReSpec issues and everything editorial we are going to fix them all. As a matter of fact this extension document is going to go away and we are going to merge the content one at a time to the main spec and keep everything fixed with no warnings/errors.

Regarding the naming of it, just to give a little background Pointer Events working group actually had all different names including the one you suggested if I recall correctly. We intentionally said we need choose a hard word so people don't just randomly use it as it is a very niche API and hopefully that makes them go and look it up to see what it does and what "coalescing" means really. We knew most of the web apps should not care about or use these points for their use cases and using them would just hurt their performance and we were hoping this hard and more accurate word would serve that purpose as well.

@cynthia cynthia added the Resolution: satisfied The TAG is satisfied with this design label Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests