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

Does relatedTargetScoped need changes #486

Closed
annevk opened this issue Apr 19, 2016 · 13 comments
Closed

Does relatedTargetScoped need changes #486

annevk opened this issue Apr 19, 2016 · 13 comments

Comments

@annevk
Copy link
Collaborator

annevk commented Apr 19, 2016

It seems for all user-agent-dispatched events this will be true. However, its value is false by default. If true is indeed the desired default, should we not name it differently? unScopedRelatedTarget or some such.

It's also only relevant for UI events, not DOM events in general. It seems like a pretty bad layering violation, though it's not entirely clear to me if we can solve that. (We have a couple of those already with shadow trees, so it might not be too bad.)

@annevk annevk changed the title Does relatedTargetScoped make sense Does relatedTargetScoped need changes Apr 19, 2016
@annevk annevk added the v1 label Apr 19, 2016
@hayatoito
Copy link
Contributor

hayatoito commented Apr 20, 2016

For synthetic events, relatedTargetScoped should be false by default. The context is:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=20017

Yeah, "true by default in all cases" is desired, I think. However, I could not do it for synthetic events because it would break the compatibility.

Though this compatibility risk looks very low, IMO.

I did not realize that relatedTargetScoped is violating layering. Can we have a hook in event path calculation in DOM events so that UI events spec can use it?

@annevk
Copy link
Collaborator Author

annevk commented Apr 20, 2016

Ah okay, if true by default is not possible I guess the flag is fine. And yeah, I'm wondering whether a hook would be better since Event does not have relatedTarget and the situation for touch events is even more different. @RByers, @travisleithead, thoughts?

@rniwa
Copy link
Collaborator

rniwa commented Apr 21, 2016

What backwards compatibility issue are we talking about? This flag only affects events that cross a shadow boundary, which didn't exist in the past so I don't understand what kind of compatibility concern we have.

@hayatoito
Copy link
Contributor

@annevk
Copy link
Collaborator Author

annevk commented Apr 21, 2016

@rniwa I think the issue would be synthetic events. @hayatoito isn't there some way that we could only do this of the original target was inside a shadow tree? That should avoid that concern I think.

@hayatoito
Copy link
Contributor

hayatoito commented Apr 29, 2016

Isn't there some way that we could only do this of the original target was inside a shadow tree? That should avoid that concern I think.

That could be possible if it is okay to have a special rule. Thus, by default:

  • In a shadow tree, we do not dispatch a synthetic event if a target and relateTarget are same.
  • In other cases, we still dispatch a synthetic event event even if a target and relateTarget are same (for the compatibility)

Actually, Blink did a similar thing so that we do not break a compatibility in the past, AFAIR.

Then, we can rename the flag to unScopedRelatedTarget ( I do not have any good name), whose value is false by default in all cases.

@annevk
Copy link
Collaborator Author

annevk commented May 2, 2016

I think if we flipped the default we don't need a flag at all for v1. And we just special case all events that have relatedTarget in the event dispatch algorithm. It's still a little weird, and special casing touch events will be too, but I guess there's no great way around that.

Also paging @smaug---- and @inscriber.

@RByers
Copy link
Member

RByers commented May 2, 2016

It's still a little weird, and special casing touch events will be too, but I guess there's no great way around that.

Yeah there may not be any clean way around special-casing the dispatch of touch events here. That's what blink's implementation has to do already (EventPath::adjustForTouchEvent), so I guess it's not too surprising to require some parallel complexity in the spec.

It's an interesting question to ask whether an event with multiple logical related elements is an inherently bad DOM event design or not. There are some reasons why this approach to handling multi touch is better for developers and for performance than (eg.) dispatching a separate event per touch point (like Pointer Event do), but it's quite debatable.

/cc @tkent-google @dtapuska

@hayatoito
Copy link
Contributor

hayatoito commented May 25, 2016

I would like to solve this issue. I'm fine with Anne's proposal in #486 (comment)

  • Remove this flag.
  • We always do "relatedTargetScoped" in a shadow tree.
  • One exception: We dispatch a synthetic event in a non-shadow tree even if a target and relateTarget are same.

Is there any objection to this idea?

@hayatoito
Copy link
Contributor

hayatoito commented May 26, 2016

One more question: Should we do "relatedTargetScoped" for all kinds of events whenever they have relatedTarget property?

e.g.

  1. UI Events. e.g. let e = new MouseEvent('foo', {relatedTarget: y}); x.dispatchEvent(e);
  2. Other Events. e.g. let e = new Event('my-event')); e.relatedTarget = y; x.dispatchEvent(e);
  • Only for 1
  • or for both 1 and 2 ?

I am fine with "only for 1". WDYT?

@smaug----
Copy link

Event interface doesn't have relatedTarget property.
When you do let e = new Event('my-event')); e.relatedTarget = y; you just add an expando object to the event instance.
So, 1.

@hayatoito
Copy link
Contributor

Thanks. That matches my understanding!

@annevk
Copy link
Collaborator Author

annevk commented May 26, 2016

Yeah, only when there's an IDL-defined relatedTarget attribute. Ideally at some point that manifests itself as an internal slot [[relatedTarget]] which we can talk about in specifications and content cannot recreate, but until we reach that precision I guess we should just talk about a relatedTarget attribute...

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

5 participants