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

"create an event" doesn't work for certain UI events #414

Closed
domenic opened this issue Feb 20, 2017 · 17 comments
Closed

"create an event" doesn't work for certain UI events #414

domenic opened this issue Feb 20, 2017 · 17 comments

Comments

@domenic
Copy link
Member

domenic commented Feb 20, 2017

https://dom.spec.whatwg.org/commit-snapshots/26cfadd185649915bb73d147c8a7a41335cb7ab0/#constructing-events

Consider KeyboardEvent. The problem is that there are many elements in EventModifierInit (which KeyboardEventInit derives from) which are not attributes. So that's not great.

One way to try to fix this would be something like trying to invoke the constructor with the default value, instead of just setting the attributes? And then we assume that the override defined in https://w3c.github.io/uievents/#event-constructors is good enough?

@annevk
Copy link
Member

annevk commented Feb 21, 2017

Can't they simply define their own constructor?

@domenic
Copy link
Member Author

domenic commented Feb 21, 2017

You mean, define their own version of "create an event"? I haven't checked, but it seems probable that e.g. HTML uses "create an event" to create mouse events... we'd have to go update HTML and any other specs to use the new UI events "create an event" instead of the DOM "create an event", if that's what you're suggesting.

@annevk
Copy link
Member

annevk commented Feb 21, 2017

Maybe we should just inline https://w3c.github.io/uievents/#event-constructors as a legacy exception?

@domenic
Copy link
Member Author

domenic commented Feb 21, 2017

I like that idea. @garykac, thoughts?

@garykac
Copy link

garykac commented Feb 21, 2017

Do you mean moving this algorithm snippet:

if Mouse or Keyboard event
  foreach EventModifierInit argument
    ...

into the DOM spec (and removing section 3.6 from UIEvents), or having UIEvents expose an function that the DOM spec would call?

The advantage of the latter is that it will be easier for me to modify the parts related to the "key modifier names", which is something that I need to do.

@annevk
Copy link
Member

annevk commented Feb 21, 2017

We could do the latter as well, provided it's somehow limited to these event classes. I mainly want to dissuade others from trying to copy that pattern, although I guess we should consult with some implementers to make sure this doesn't happen in more places before we commit.

@domenic
Copy link
Member Author

domenic commented Feb 21, 2017

I guess the main idea is to make https://dom.spec.whatwg.org/commit-snapshots/26cfadd185649915bb73d147c8a7a41335cb7ab0/#constructing-events work for UI events too. We should be able to do so in a way that calls out to UI events so you can maintain the steps there, mostly.

So concretely I think the best thing would be to add to UI events an algorithm "initialize a mouse or keyboard event from a initialization dictionary" (better name welcome) which takes an event and a dictionary.

Then DOM's constructor algorithm can branch and perform the current step 4 only for non-mouse/keyboard events, calling out to UI event's "initialize a mouse or keyboard event from a initialization dictionary" for those cases. The same for DOM's "create an event" algorithm step 4.

How does that sound?

@annevk
Copy link
Member

annevk commented Feb 21, 2017

Would be great if @smaug----, @dtapuska, and @cdumez could review that suggestion.

@domenic
Copy link
Member Author

domenic commented Feb 21, 2017

I mean it's mostly spec factoring so I'm not sure why they'd need to do so? But sure, the more eyes the merrier.

@annevk
Copy link
Member

annevk commented Feb 21, 2017

I want to know if these are the only special classes.

@annevk
Copy link
Member

annevk commented Mar 13, 2018

So given what we need for "relatedTargets" (see #585) I'm thinking the event constructor calls an internal algorithm that by default is a no-op. (We might want to define it works for inheritance somehow too.)

That way we construct the object, then for the dictionary members we set those dictionary members that have a corresponding internal slot on the object. Then we call that internal algorithm which can handle the remainder, e.g., setting relatedTargets based on the provided relatedTarget, or the much more complicated equivalent operation for Touch Events.

It seems this should work for keyboard events too. We'd recommend against relying on this algorithm without filing an issue against this repository for advice, or some such.

@domenic
Copy link
Member Author

domenic commented Mar 13, 2018

Although the general direction seems right, isn't part of the problem that the init dicts for certain UI events, there is no corresponding internal slot/property? I guess maybe if you checked for the slot first it'd be ok.

@annevk
Copy link
Member

annevk commented Mar 13, 2018

Yeah, we'd check if there's a slot, and if there's not just leave it be and hope the internal algorithm handles it. That's what we need for relatedTarget for instance, which would be backed by relatedTargets.

@annevk
Copy link
Member

annevk commented Mar 15, 2018

I've done an initial step here: #603.

How exactly do we deal with subclasses here? I think each event class needs to have some associated steps that by default don't do anything. Then when constructing an event we call these steps for each class in the chain?

@annevk
Copy link
Member

annevk commented Mar 16, 2018

@hayatoito @TakayoshiKochi here's my rough idea:

So when you create a TouchEvent object, these construction steps can take care of initializing the "touch target list". When you create a MouseEvent, they can take of initializing "relatedTarget" (and maybe some other fields). Same for KeyboardEvent's special requirements, etc.

Does that make sense?

(I'm not entirely sure if the inheritance is needed or if we can do with a single set of steps per event class. Maybe we should just have a single set of steps for now and if inheritance becomes a need somewhere they can implement their own.)

@hayatoito
Copy link
Member

That makes sense to me.

if the inheritance is needed or if we can do with a single set of steps per event class.

I don't have any preference. It sounds either should work.
"with a single set of steps per event class" might be a simpler way, for now.

@annevk
Copy link
Member

annevk commented Mar 23, 2018

Okay, so based on reading OP again I think the best way forward here is a single callback. UI events can then define the implementation of this callback for WheelEvent, MouseEvent, KeyboardEvent. That implementation would call shared algorithms that can handle the bits identical across these interfaces.

annevk added a commit that referenced this issue Mar 23, 2018
This hook can be used by complex Event subclasses, such as KeyboardEvent and TouchEvent, to initialize their internal state upon creation.

Fixes #414.
annevk added a commit that referenced this issue Apr 6, 2018
This hook can be used by complex Event subclasses, such as KeyboardEvent and TouchEvent, to initialize their internal state upon creation.

Follow-up: w3c/uievents#194 and w3c/touch-events#94.

Fixes #414.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants