search

WICG / webcomponents

Web Components specifications
Other
4.38k stars 375 forks source link

Events whose composed flag should be true #513

Closed hayatoito closed 8 years ago

hayatoito commented 8 years ago

We have decided to use Event.composed flag (false by default), instead of Event.scoped, negating its meaning. See the context: https://github.com/whatwg/html/issues/1160.

That means we should have a list of exceptional events whose composed flag should be true, instead of the current list.

Eventually, instead of having a list in one place, each event's definition should mention its composed flag should be true in each place, but I would like to have a rough consensus here.

I have made a list of events whose composed flag should be true by default, I think. The list is basically made from UIEvent spec: https://w3c.github.io/uievents/.

I am going to update Blink's implementation, however, what do you think about Composition Events? I am not 100% sure what should I do for Composiition Events. Should we exclude Composition Events from here?

hayatoito commented 8 years ago

@sorvell, I would like to hear opinions from Polymer folks too. Does the decision affect Polymer's implementation?

hayatoito commented 8 years ago

TouchEvents and PointerEvents are also candidates.

Given that, instead of enumerating every candidates, can we just say: "Every UIEvents, except for load, unload, abort, error and select"?

Is that okay?

annevk commented 8 years ago

We really have to update the relevant specifications that dispatch UI events. And if you really want to create a monkey patch first, I think it's better to be exhaustive than to hope implementations will do the right thing.

rniwa commented 8 years ago

We should explicitly enumerate every event.

hayatoito commented 8 years ago

I agree that we should explicitly enumerate every event. Let me try to enumerate every event here before updating the relevant specifications.

hayatoito commented 8 years ago

Here is the exhaustive list. This should be a good starting point, hopefully.

I think it is important that we can know the rationale how we can make the list. So here is how to reproduce the list.

1. Go to https://w3c.github.io/uievents/.
   Get every event types from Chapter 5, excluding events in Chapter 5.1
   -> We got:
      Focus Events:
        blur
        focus
        focusin
        focusout
      Mouse Events:
        click
        dblclick
        mousedown
        mouseenter
        mouseleave
        mousemove
        mouseout
        mouseover
        mouseup
      Wheel Events:
        wheel
      Input Events:
        beforeinput
        input
      Keyboard Events:
        keydown
        keyup
      Composition Events:
        compositionstart
        compositionupdate
        compositionend
2. Go to https://w3c.github.io/touch-events/
   -> We got:
      Touch Event:
        touchstart
        touchend
        touchmove
        touchcancel
3. Go to https://w3c.github.io/pointerevents/
   -> We got:
      Pointer Events:
        pointerover
        pointerenter
        pointerdown
        pointermove
        pointerup
        pointercancel
        pointerout
        pointerleave
        gotpointercapture
        lostpointercapture
hayatoito commented 8 years ago

Yeah, I think per interface basis might work well here because I could not find any exceptional case.

However, I do not agree the rationale you mentioned :) For synthetic events, we should always honor the EventInit dictionary. I meant:

let e = new FocusEvent('focus');
console.assert(e.composed == false);

let e = new FocusEvent('focus', { composed: false });
console.assert(e.composed == false);

let e = new FocusEvent('focus', { composed: true });
console.assert(e.composed == true);  <== true only if it is specified in EventInit dictionaly
domenic commented 8 years ago

It's worth checking to see if https://html.spec.whatwg.org/multipage/indices.html#events-2 has any events you'll want to include. My guess is that most of them that could be considered "UI events" are already in those other specs but some might be missing. E.g. I think change or copy or cut or paste might be worth considering.

annevk commented 8 years ago

Drag-and-drop too probably. Drag-and-drop and cut/copy could be tricky with the user leaking nodes from shadow trees. Maybe that's okay...

rniwa commented 8 years ago

However, I do not agree the rationale you mentioned :) For synthetic events, we should always honor the EventInit dictionary.

I agree in the case author specified a value but we should just change the default value for EventInit for those special constructors when the value is missing.

hayatoito commented 8 years ago

I agree in the case author specified a value but we should just change the default value for EventInit for those special constructors when the value is missing.

Can we do that? I am afraid that it is confusing if EventInit's default value depends on Event's interface.

dictionary EventInit {
  boolean bubbles = false;
  boolean cancelable = false;
  boolean composed = false;
};

I prefer not changing the default value, per Event interface. Rather, I would like to think as if UA set it to true explicitly, like the following:

// This is *imaginary* code, which is happening in browser's engine for UA events.
let e = new FocusEvent('xxx', { composed: true });

Thus, I would like users to specify it explicitly as UA does.

hayatoito commented 8 years ago

It looks that DragEvent inherits MouseEvent. Thus, this can be automatic if we use per-interface basis.

hayatoito commented 8 years ago

cut/copy/paste are defined in https://w3c.github.io/clipboard-apis/

Its interface is

interface ClipboardEvent : Event

I have taken a look at the spec, however, I do not have any preference whether to add ClipBoardEvent or not. I guess there are different opinions on ClipboardEvent.

It might be good to exclude it as a starting point. If web developers find that it is inconvenient, we can add it without a significant risk, I think.

hayatoito commented 8 years ago

In summary, so far:

Inheritance hierarchy:

If we use per-interface basss, FocusEvent, MouseEvent, InputEvent, KeyboardEvent, CompositionEvent, and TouchEvent are the candidates.

rniwa commented 8 years ago

I prefer not changing the default value, per Event interface. Rather, I would like to think as if UA set it to true explicitly, like the following:

// This is *imaginary* code, which is happening in browser's engine for UA events.
let e = new FocusEvent('xxx', { composed: true });

Thus, I would like users to specify it explicitly as UA does.

The thing is, FocusEvent doesn't take EventInit. It takes FocusEventInit, which inherits from UIEventInit, which in turn inherits from EventInit. In the case of MouseEvent and its derived classes, MouseEventInit doesn't even inherit from EventInit (this is presumably a spec bug though). In general, there should be a minimal difference between UA created events and author created events.

If we're not going to the route of initializing based on the interface, then we absolutely need to enumerate every single event for which composed flag must be set to true by UA instead of vaguely relying on the interface being used.

hayatoito commented 8 years ago

If possible, I would like to adopt per-interface basis for synthetic events too, but my conern is its feasibility.

Suppose the following cases:

 let e1 = new FocusEvent('xxx');
 let e2 = new FocusEvent('xxx', { bubbles: true });
 let e3 = new FocusEvent('xxx', { bubbles: true , composed: true });
 let e4 = new FocusEvent('xxx', { bubbles: true , composed: false});

If we adopt "default changes per-interface" for synthetic events too, I think user's expectations are:

 assert(e1.composed === true);
 assert(e2.composed === true);
 assert(e3.composed === true);
 assert(e4.composed === false);

However, since FocusEventInit inherits EventInit, we can not distinguish case e2 and e4 nicely in our binding layer. In both cases, FocusEventInit.composed was already set to false. We can not tell the difference.

rniwa commented 8 years ago

I don't understand how that works. You're invoking a different constructor. Surely you ought be able to distinguish that FocusEvent rather than Event constructor is invoked. Then, in each constructor, you can specify the appropriate default value for composed when it's missing in the dictionary specified by the author.

hayatoito commented 8 years ago

I have found that Blink's Web IDL binding generates the following for EventInit:

https://cs.chromium.org/chromium/src/out/Debug/gen/blink/core/events/EventInit.h

Nullable<bool> m_composed;

It is not bool. It is Nullable<bool>! I did not realize it. It looks that we can distinguish e2 (missing (m_composed == null , hopefully) and e4 (m_composed == false).

However, does WebIDL guarantee this third state? Do every user agents can tell whether the value is missing, false or true, strictly?

domenic commented 8 years ago

I don't have a very strong opinion here but I wanted to say that the direction this discussion is going seems rather strange to me. My mental model is that the composedness of an event is similar to whether it bubbles or is cancelable. That is, it is a property of an individual firing of the event. Saying that there should be a per-interface default for composedness seems very strange to me given that there is no similar concept for bubbles or cancelable.

As far as I can tell every input event fired by the platform bubbles. But we don't say that InputEvent should have a default of bubbles: true. We don't say that new InputEvent("input", {}) should be equivalent to new InputEvent("input", { bubbles: true }). Instead we explicitly specify the value of the bubbles flag at all places in the platform that fire input events.

I can see some value in providing non-normative guidance that in general events which use the InputEvent interface when fired by the platform will bubble, not be cancelable, and be composed. Maybe there could be a table listing this matrix for all event types and/or interfaces. But I don't think that should be part of the definition of InputEvent or InputEventInit, and should not affect the developer-facing defaults, which IMO should be the same for all events.

Yes, it will be a lot of work to update all the places that fire events to correctly set composed. But it is doable, and would be consistent with how the platform already works for other flags on events. I don't think we should try to take a shortcut here.

hayatoito commented 8 years ago

Yeah, regarding UA events, I think "per-interface basis" is just a syntax sugar which is only valid in this discussion. In updating the relevant specs, we should update each place where an event is fired.

Enumerating interfaces here should be just a guide which tells us where we should update.

rniwa commented 8 years ago

@domenic : That's a fair point. Let's go with explicitly enumerating every single that UA fires as composed=true then.

hayatoito commented 8 years ago

Thank you, folks. I think the conclusion is:

  1. For each relevant spec:
    1. For each place where an event is fired:
      1. If the event's interface is (or inherits) one of the enumerated interfaces here:
        1. Set composed=true

In terms of implementation, I think this conclusion is implementor-friendly because we do not have to update many places in our engine.

hayatoito commented 8 years ago

I have filed an issue for UIEvents specification: https://github.com/w3c/uievents/issues/89

hayatoito commented 8 years ago

I think we have to reflect our conclusions to the following specifications. I have filed issues:

RByers commented 8 years ago

I've looked at this change for the TouchEvents PR. Since TouchEvents and PointerEvents are just sub-classes of UIEvents I think we'll be happy to follow whatever pattern is decided on there. Probably not worth debating the issue for TouchEvents and PointerEvents specifically until a PR is approved for UIEvents.

I don't know much about ShadowDOM, so please excuse my ignorance. At first I thought it was odd that we'd need to explicitly specify that most types of UIEvent have composed=true, it would seem nicer if we could just specify the behavior by interface (and then eg. PointerEvent would inherit the behavior of MouseEvent). It looks like this is what @rniwa was advocating, right? But I understand @domenic's concern that that would be unusual.

Are there compat concerns to consider with the two different options here? It's common to synthesize a MouseEvent from JavaScript, and in general we can expect that developers won't think to set composed=true on such events (and certainly won't have for existing code). Is that a problem at all? Or perhaps it actually makes sense that JS-synthesized input events typically would not cross shadow DOM boundaries (since the code generating the event is operating at one particular component boundary, unless it explicitly knows to poke down into ShadowDOM)? In that case we can avoid encouraging the use of composed=true for synthetic events, except in scenarios where the developer is really trying to reason across component boundaries.

Note a variant of this problem exists today with bubbles. I often see developers (especially those use to the legacy TouchEvent.initTouchEvent API) forget to set bubbles=true when creating synthetic input events (of course touch events should bubble!). But that's obviously unlikely to change...

hayatoito commented 8 years ago

It's common to synthesize a MouseEvent from JavaScript, and in general we can expect that developers won't think to set composed=true on such events (and certainly won't have for existing code).

Yeah, I think it could happen that developers forget to add 'composed=true' when they should specify it, but that should not be a significant problem for developers, I think. They can learn, and fix it easily.

"The value of EventInit dictionary is always used for synthetic events" is an easy rule for developers to remember. That means they do not have to learn: "What is the default value of YYY for synthetic XXX events?"

RByers commented 8 years ago

Yeah, I think it could happen that developers forget to add 'composed=true' when they should specify it, but that should not be a significant problem for developers, I think. They can learn, and fix it easily.

But would they typically need to? I guess my position would be that if we're really going to ask developers to ALWAYS specify composed=true for their synthetic input events, then we should further explore @rniwa's idea of just making that happen for them by default. But if it really doesn't matter in most cases (false would typically produce unsurprising behavior), then I'm fine with the plan as-is.

Two main classes of use cases I can think of for synthetic input events:

1) Dispatching a synthetic input event for testing / UI automation purposes. In these cases I think the developer typically knows the exact target / abstraction layer and doesn't generally on bubbling. So I think that getting composed=false would be fine in these cases. Right?

2) Dispatching a synthetic input event of one type based on another event. Eg. some libraries generate synthetic MouseEvent instances for TouchEvents they receive (or TouchEvent for PointerEvent, etc.). In those cases, all the event properties (including bubbles, composed, etc.) should be copied into the new event, so this makes sense for new code. But that won't be happening today.

In particular, imagine a complex site that added TouchEvent support by installing a general purpose library that fired synthetic MouseEvent instances for TouchEvents seen. Somewhere else on the site they start using custom elements. Now touch events that hit that custom element will not work correctly (since the synthetic mouse events won't propagate across the shadow boundary) without also updating the synthetic event generation library. Maybe that's OK, but it's certainly an easy developer trap, which @rniwa's proposal would avoid. Incidentally, this would have worked fine with chromium's V0 ShadowDOM.

hayatoito commented 8 years ago

Thank you for the feedback. Yeah, I am aware that the current conclusion would cause a burden on developers in the short term. However,

If we make the behavior of composed be inconsistent with bubbles (or other properties) to avoid a trap in the short term, we will regret that in the long term, I think.

In addition to that, I think we still have an issue on technical feasibility on "per-interface" basis, as I explained in https://github.com/w3c/webcomponents/issues/513#issuecomment-224477918

RByers commented 8 years ago

Fair enough, I just wanted to make sure the implications were thought through. Sounds like a reasonable plan to me. @rniwa are you OK with this plan?

I'll keep an eye on the UIEvents PR.

rniwa commented 8 years ago

It appears to me that we also need to make drag events composed: drag, dragstart, dragend, dragenter, dragleave, dragover, drop

Otherwise, you may end up observing a fraction of events like dragend without dragstart.

rniwa commented 8 years ago

It looks like you also missed keypress.

rniwa commented 8 years ago

Oh, I didn't realize there was an earlier discussion about drag & drop. I suppose leaking nodes from a shadow tree is problematic. I'm okay with leaving it un-composed for now but we should make copy, paste, cut and their before* variants composed since they don't have any node leakage problem, and yet are very important user gestures.

rniwa commented 8 years ago

We should probably make DOMActivate composed as well although it's a deprecated event.

hayatoito commented 8 years ago

Thanks. I forgot to update Drag Events, which are defined in https://html.spec.whatwg.org/#dnd. I did not have an intention to make it un-composed.

If a Drag event has a property for a node, we should retarget it to prevent a leak. Let me check it.

hayatoito commented 8 years ago

DragEvent has a DataTransfer property:

https://html.spec.whatwg.org/#dragevent

interface DragEvent : MouseEvent {
  readonly attribute DataTransfer? dataTransfer;
};

It looks DataTransfer object does not have any node information which must be protected. https://html.spec.whatwg.org/#datatransfer

Given that, we can make Drag Events composed events without any retargeting for its internal property, I think. If I miss something, please correct me.

I am not 100% sure how drag and drop will make sense across node trees in real world. I am assuming that we need to support it.

hayatoito commented 8 years ago

PR for DragEvents is https://github.com/whatwg/html/pull/1534.

hayatoito commented 8 years ago

Regarding keypress and DOMActivate, I remember that I excluded these because these are legacy events. I excluded all events defined as legacy events in https://w3c.github.io/uievents/#legacy-event-types. I am not sure what is the best for legacy event types. Should we update these also?

but we should make copy, paste, cut and their before* variants composed since they don't have any node leakage problem, and yet are very important user gestures.

Do you know where before* variants are defined? I could not find a definition in https://w3c.github.io/clipboard-apis/.

domenic commented 8 years ago

I think we should update "legacy" event types. In my experience the "legacy" event types are used quite often across the web and it's mostly wishful thinking by spec authors that calls them legacy.

I don't feel strongly about this though.

hayatoito commented 8 years ago

Thanks. I got it. Since MutationEvent is not supported in a shadow tree, we have to update DOMActivate, DOMFocusIn, DOMFocusOut and keypress events there. Let me send a PR for these events in UIEvents.

hayatoito commented 8 years ago

PR for legacy UIEvents is : https://github.com/w3c/uievents/pull/93

dfreedm commented 8 years ago

Have TransitionEvent and AnimationEvent been addressed yet? Seems like those should be composed: true by the UA as well

annevk commented 8 years ago

Why?

rniwa commented 8 years ago

Yeah, I don't think transition events or animation events are initiated by users so I don't see why they need to composed.

ebidel commented 8 years ago

I can think of a few examples where TransitionEvent and AnimationEvent have been helpful in v0.

A ripple animation as the result of a button press: http://jsbin.com/yobuloralo/edit?html,output. A component consumer may want to wait for the animation to finish before doing more work (and thus preventing jank).

A mobile app drawer sliding in/out (as the result of a user interaction). Users may want to know when the drawer has fully opened.

A dialog overlay that transitions in (as the result of a user interaction). Users may want to know when the dialog's transition is done.

The component could fire fire a custom event, but that requires more boilerplate for its author. It's natural to listen for the animation events the component is already firing.

hayatoito commented 8 years ago

Thank you for the feedback. In such a user interaction, a composed event is always fired, isn't it? e.g. "mouseover" or "click" event. Is it difficult for component consumers to do the right thing at the right timing only by listening these composed events?

ebidel commented 8 years ago

Sure, you could listen for something like "click", but I'm not sure how you'd accurately know when the animation/transition is done. Plus, the last thing you'd want is user code (potentially) running during your animation.

A couple of more useful examples from the Google I/O 2016 PWA (we waited for several components to fire a transitionend event for internal animations, to delay work and make sure animations were smooth).

domenic commented 8 years ago

It seems really bad to be reusing animation events for those kind of domain specific "done sliding" things. I would expect you would fire control-specific events like "toggle" or "open" or "close".

Otherwise you are tying your consumers to the implementation detail of your component, which is that it currently animates. What happens when in v2 you implement a "low power mode" that stops doing animations when the battery level is low? Or in v13 when the industry has moved away from animations as a method of signifying transitions (like Apple has largely done from OS X 10.0 to the current macOS versions)?

Transition/animation events definitely feel like the kind of internal-to-a-control, implementation-choice-specific, presentation-related events that should not go outside the shadow tree by default.

hayatoito commented 8 years ago

@ebidel Thanks.

Could components authors catch these animation events in their shadow tree, and fire a component specific custom event for component consumers?

As the example does it, "on-countdown-complete", in https://github.com/GoogleChrome/ioweb2016/blob/41bdeb6ad3a41fe675f2b49dfa343121f720c135/app/elements/io-live.html#L270

Does it make an interface between component authors and component users more stable one?

ebidel commented 8 years ago

This was my earlier point:

The component could fire a custom event, but that requires more boilerplate for its author (has to add listeners, catch it, forward a different name, and do cleanup work in detached). It's more natural to use the animation events already being fired.

IMO, that's all more complicated than it needs to be.

What happens when in v2...

Events are part of a component's public API. I'd expect well authored components to clearly document changes. Real example: https://elements.polymer-project.org/elements/paper-button#events

domenic commented 8 years ago

Right. I think the concern is exactly about making it easy to accidentally expose implementation details as part of a component's public API. Doing so should be intentional.