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

Merge events into "onnavigate" or "onfocusmove"? #32

Closed
hugoholgersson opened this issue Feb 16, 2018 · 11 comments
Closed

Merge events into "onnavigate" or "onfocusmove"? #32

hugoholgersson opened this issue Feb 16, 2018 · 11 comments
Labels
closed:rejected Suggestion rejected by the editors topic:spec type:bug Describes a problem with the spec/tests/polyfill in their present form

Comments

@hugoholgersson
Copy link

hugoholgersson commented Feb 16, 2018

I'm thinking fewer events give authors a chance to write tighter JavaScript: one handler for all of the navigation-related reactions.

When authors tweak their apps' navigation, I imagine debugging being easier if they only need to break and assert things in one event handler (not 3). We will enjoy the same convenience when writing web platform tests.

Proposal: Give NavigationEvent a reason or action attribute.

From an implementor's perspective I see two advantages:

  1. We pollute GlobalEventHandlers's namespace a bit less :)
  2. Tighter web platform tests.

By the way, the event's name does not need to include before. That's already implicit from being cancelable, right?

@frivoal
Copy link
Collaborator

frivoal commented Feb 19, 2018

I'm thinking fewer events give authors a chance to write tighter JavaScript: one handler for all of the navigation-related reactions.

You can already do that with 3 events, and the handler would look almost identical to the one you'd write for what you suggest.

function spatnavHandler(e) {
switch (e.type) {
  case 'navbeforefocus': 
  /* foo */
  case 'navbeforescroll': 
  /* bar */
  case 'navnotarget': 
  /* baz */
}

vs

function spatnavHandler(e) {
switch (e.reason) {
  case 'navbeforefocus': 
  /* foo */
  case 'navbeforescroll': 
  /* bar */
  case 'navnotarget': 
  /* baz */
}

The only difference is in how you register them:

['navbeforefocus', 'navbeforescroll', 'navnotarget'].map(function(e) {
  element.addEventListener(e, spatnavHandler);
});

vs

element.addEventListener("spatnav", spatnavHandler);

But I am not sure you'd generally want the same handle for all three.

  1. Say you're trying to implement something like https://github.com/WICG/spatial-navigation/blob/master/explainer.md#nav-rule-property-cssui4

    You probably don't want to change anything when navbeforescroll or navnotargetwould be fired. you only care about the case where candidates were found, so that you can change which of them you pick, and therefore you only react to the navbeforefocus event

  2. Say you're trying to implement something like https://github.com/WICG/spatial-navigation/blob/master/explainer.md#nav-loop-property-cssui4

    You probably don't want to change anything to navbeforescroll or navbeforefocus, but in the case where navnotarget is fired (because there's no more focusable things in the direction the user asked for), you do want to take over, and search for candidates in the opposite direction.

By the way, the event's name does not need to include before. That's already implicit from being cancelable, right?

No strong opinion here. I was trying to follow the pattern of UI-EVENTS which has a before-input and an input event, and since there already are focus and scroll events, and these would fire before, calling them something with "before" in the name made sense to me.

That said, I'm not super happy about the current names, so if we're not (strongly) violating existing conventions by not using "before" in the name, maybe the shorter the better.

@hugoholgersson
Copy link
Author

The only difference is in how you register them

That's a good argument for combining them. Web devs usually prefer compact code.

@frivoal
Copy link
Collaborator

frivoal commented Feb 21, 2018

It is an argument to combine them IF the typical case is to catch them all with the same handler. I am not convinced it is.

I am sure that I am biased in all sorts of ways, but most use cases I can think of need different handlers for the different events, in which case the current design makes more sense.

If you think that wanting to catch them all in the same handler is typical, could you share some use cases for that?

@hugoholgersson
Copy link
Author

Let me present the lazy JS developer Lars. Lars writes excellent code. He thinks. So if the error handling is not really needed he rather leaves it out, to save some time. To Lars, navnotarget and navbeforescroll sounds like error cases. Surely, he doesn't need that.

Now if one "tell it all"-event, let's call it navigate, also tells him that something "error-like" happened, he might have added a check for it. This pattern maybe even requires him to do that check. Enforced defensive programming.

Another common use case would be debugging. Lars normally doesn't care about the error-like events. But, while trying to figure out what his app is doing wrong, he puts a breakpoint / a log print in his onnavigate-handler. Less effort compared to adding new handlers.


From implementors' perspective, in C++ or Rust, I'd rather just implement and write tests for DispatchOnNavigate() instead of 3 methods.

When promoting this spec to vendors, it'd be easier. We can just say "You only need to implement onnavigate and focus-navigation and you're done!" (given that the JS API isn't mandatory).

@frivoal
Copy link
Collaborator

frivoal commented Feb 21, 2018

What I am still not understanding in your scenario: what is Lars trying to do that benefits form him catching all events together? Sure, if his code should be handling all three (especially if it should handled all three the same way), then making it easier to handle all three (the same way) is better. But why should it?

If you put it in abstract terms like this, the same argument could apply to having a single mouse event, instead of click, dblclick, auxclick, mouseout, etc.
Sure, you could make a single event for all that, with an action attribute, but instead we have a bunch of events, all using the MouseEvent interface, and differentiated by their type attribute, because you typically react to these various events differently, and usually not to all of them.

To me, the situation is the same here. If the typical use case calls for a single unified event handler for all 3 cases, then merging them can make sense. I do not believe it does. Happy to be proven wrong, but that calls for use cases (specific examples of things authors are trying to achieve by catching those events).

You can also turn the argument on its head: if Lars doesn't think of what sounds like "error cases" to him, then he won't handle them when he first writes the code. Then on his 27 inch iMac, everything fits the screen without scrolling or overflow, so in his environment, all he ever gets is the action: focus kind of navigation event. Then some user loads it in a different environment, where some elements are not visible, sometimes the action: notarget or action: scroll kind of navigation event fires, and Lars' code does totally the wrong thing, because the implicit assumption that action: focus is the only kind of navigation event is violated. So, for people like Lars, separating the event into distinct types that must be registered separately makes his code more robust.

@hugoholgersson hugoholgersson changed the title Merge events into "onnavigation"? Merge events into "onnavigate"? Feb 22, 2018
@hugoholgersson
Copy link
Author

hugoholgersson commented Feb 26, 2018

... that calls for use cases (specific examples of things authors are trying to achieve by catching those events).

Ah, right. I can't think of anything you can do with a "combo-event" but not with 3 separate events...

However, if one event can carry the same info, I'd like to think that it means less work for us later on when implementing and testing this API.

Lazy devs might appreciate that there's two less events to read up on and to handle.


This is how I imagine the "combo-event" onnavigate being used:

The API signals a failure by setting relatedTarget to null.

Here relatedTarget always means the same thing: where focus is about to go (null means nowhere).

function navigationHandler(e) {
  if (e.relatedTarget) {
    // The browser is about to move focus to relatedTarget. Let's intercept.
  }
  else {
    switch (e.reason) {
      case 'scroll':
        if (e.container == special-scroller) 
          ....
      case 'notarget':
       ...
    }
  }
}

Here e.relatedTarget != null means e.reason == 'focus'.
e.container was suggested in #33 .

... sometimes the action: notarget or action: scroll kind of navigation event fires, and Lars' code does totally the wrong thing ...

The event signals something like "the user tried to move focus, here's the result". With a usage/example pattern that shows how relatedTarget often is null, even lazy devs are forced to think about the "focus did not move"-cases?

While null-checking is usually something we would like to avoid, the mouse*-family of events also set relatedTarget to null to signal similar info. So maybe this pattern isn't too unusual after all?

Is there anything we can do with 3 events that we cannot do with a combo-event?

@anawhj
Copy link
Collaborator

anawhj commented Mar 5, 2018

Personally, I'm so in favor of a new event that would be the most prefered way to override a default heuristic algorithm of the spatial navigation. In this thread, there seems two approaches:

(1) navigate event (as a unified event type)
(2) navbeforefocus/navbeforescroll/navnotarget events (as separated event types)

In my eyes, It wouldn't different between them in terms of the functionality and would have some pros and cons for each ways according to the use cases. However I'm in favor of the single unified event type as the following analysis.

1. API design
In DOM4 spec, the following description is defined.

Typically events are dispatched by the user agent as
(1) the result of user interaction or (2) the completion of some task..

The all existing event types could be classified as two categories above. In general, events of (1) are cancelable, but events of (2) are not. Our proposed events would be in the middle phase of (1) and (2), because when the navigate event is invoked, the navigate operation is partially executed(getting several contexts) but isn't yet completed(cancelable) after the user interaction. It needs a cautious approach so that our new API could be merged into the existing event design and mechanism.

When I first see the three event types, the wording such as focus, scroll, notarget seems the results of the navigation. It might be triggered as the default results by user agent when user pushes a tab key or arrow keys. In addition, we would like to use these kinds of events to cancel the default and override a new operation. What the default operation of each events are described in its names of event type, would be a little weird in terms of the existing event mechanism.

Regarding relatedTarget property, its definition of the property used to be changed according to the context of each event types. In some of focus and mouse events, relatedTarget property have been used with several contexts for several purposes. In our proposed event, the property could be used as several contexts (moving focus, scrolling, and noop) like above.

2. Usage of the API
I expect that navbeforefocus would be the promising event type(more than 80%) in comparison with the other events. (navbeforescroll and navnotarget) The default result(moving focus, scrolling, none) of the navigation operation could be provided as a parameter in a scope of the navigate event so that we could override the default to a selection of other target in same operation or a change of other operation. I totally agree to support the three types of events if the events would frequently be invoked by authors, but at least for now, it doesn't seem to happen even after enabling spatial navigation as a default.

@frivoal
Copy link
Collaborator

frivoal commented Mar 9, 2018

Hi @hugoholgersson and @anawhj. Thanks for the feedback.

I think there is two parts in your suggestion:

1) An event that just lets the author know that the user triggered spatnav, without caring about details about how the UA would handle it (scrolling vs focusing vs not finding the target)

We can easily add a new event that does that. We can call it "navigate" or "beforenav" or "spatnav" something like that, and it would be fired before the first step of the spatial navigations steps (or maybe before the first step of the navigation steps), and would cancel the navigation if cancelled. relatedTarget would be null.

This could be useful for example if a user wants to completely cancel the existing spatial navigation, and do something completely different instead.

I am OK with this, and think it would be a good addition. It does not change implementation complexity much.

2) Merge the existing 3 events in 1 event, with the reason (focus/scroll/no-target) distinguished by a reason attribute instead of the type

I am much less convinced. Here are the details:

Regarding relatedTarget property, its definition of the property used to be changed according to the context of each event types. [...] In our proposed event, the property could be used as several contexts (moving focus, scrolling, and noop) like above.

That's already the case. We have one event interface (NavigationEvent), with several type of events navbeforefocus, navbeforescroll, navnotarget, and they used the relatedTarget property differently in different context.

In UI-Events, there are many cases of using relatedTarget with several meanings for a single interface (e.g. MouseEvents), but I count not find any event which uses several meaning for relatedTarget with a single event type.

This seems to be an argument in favor of the current design, not suggesting a merge of event types.

I expect that navbeforefocus would be the promising event type(more than 80%) in comparison with the other events.

That seems to be an argument for the opposite: if 80% of the cases the author cares only about navbeforefocus, then they should be able to listen to that event only. There are other cases where they only care about one of the other events (e.g. implementing nav-loop, want to listen to navnotarget), then they should be able to listen to that event only.

I totally agree to support the three types of events if the events would frequently be invoked by authors, but at least for now, it doesn't seem to happen even after enabling spatial navigation as a default.

We could skip firing events for the case of navbeforescroll and navnotarget, as they can always be added back later, but I don't think it would help, for two reasons:

  1. Removing these events will not simplify the specification meaningfully. For example, the normative text for navbeforescroll is just one step in section 7.2, nothing else. We could remove it, but spec complexity and implementation complexity would remain almost the same. Section 5.2 would become shorter, but that section whole section is only informative, and has 0 impact on implementations. It is just documentation.

  2. The spec as it is now is only level 1. I completely expect it will not be enough to cover all expectations from all authors, but it should make it easy for them to write JavaScript to solve their own problems. If we make it harder to polyfill features because of missing JavaScript primitives, we will have stronger demand for including features directly in the specification.

API design. In DOM4 spec, the following description is defined [...]

But then the DOM spec also says this:

Apart from signaling, events are sometimes also used to let an application control what happens next in an operation. For instance as part of form submission an event whose type attribute value is "submit"

Not a click event with action = sumbit, but a submit event.

I think this is similar to what we are doing here. Maybe we should ask the editors of DOM for their opinion on API design.

Lazy devs might appreciate that there's two less events to read up on and to handle.

I really don't understand this argument. Should we merge mouseenter, mousemove, and mousemove into an event call mouse? That's fewer events, so lazy devs would have fewer things to handle... But they are used for different things, so merging them would actually means more work: if you only care about one of the cases but the event gets sent in all cases, then you need a conditional inside the event handler to detect if you are in the correct case.

[...] the family of the mouse*-family of events [...]

As you say, it's a family of events. Only one thing happens (user moves the mouse), but the browser generates a bunch of events about different things that happens as the consequence of that, using relatedTarget to carry information if needed, or null otherwise. They all are instances of the same interface, using different values for the type attribute. That is what our spec is doing now as well.

Ah, right. I can't think of anything you can do with a "combo-event" but not with 3 separate events...

Of course you can do the same, since they carry the same information the question is which is more convenient. If you will always react to all 3 in the same way, then 1 merged event is better. If you will generally react in different ways, then 3 separate events is better. Since they happen in different case, and inform the author about a different thing happening, then you will want to react in different ways.

Unless you assume that all these events are useless, and the only thing the author wants to know is "spatnav happened", but I already gave examples in #32 (comment) that if you are trying to implement/polyfill nav-rule or nav-loop, you will want to catch different events, so there are already identified use cases.

it means less work for us later on when implementing

Really? You'll need the same code to check when to fire the events according to the processing model, the same code to check the return values... I know this will be implemented in browser internal code (C++) rather than in JS, in JS, the code to implement both approaches is almost identical, so I would be surprised if it was very different in C++

var ev1 = new NavigationEvent( "navbeforefocus", {dir: "up", relatedTarget: t } );
res = elm.dispatchEvent(ev1);
...
var ev2 = new NavigationEvent( "navbeforescroll", {dir: "up", relatedTarget: t } );
res = elm.dispatchEvent(ev2);
...
var ev3 = new NavigationEvent( "navnotarget", {dir: "up", relatedTarget: t } );
res = elm.dispatchEvent(ev3);

vs

var ev1 = new NavigationEvent( "navigate", {reason: "focus", dir: "up", relatedTarget: t } );
res = elm.dispatchEvent(ev1);
...
var ev2 = new NavigationEvent( "navigate", {reason: "scroll", dir: "up", relatedTarget: t } );
res = elm.dispatchEvent(ev2);
...
var ev3 = new NavigationEvent( "navigate", {reason: "notarget", dir: "up", relatedTarget: t } );
res = elm.dispatchEvent(ev3);

Also, this goes against the priority of constituencies.

it means less work for us later on when [...] testing

Why would it? You still need to check that the event with the correct reason got sent in the right scenario. That makes no big difference. Unless it makes a huge difference, paying attention to this is also against the priority of constituencies.

e.container was suggested in #33

Right. that is possible to do, but it has downsides to: DOM has multiple steps in the section on dispatching events that related to regretting relatedTarget when dispatching events, to correctly account for shadow DOM. If we introduce additional attributes pointing to elements (like e.container), we will need to add similar steps. That's possible, but it is additional complexity.

@hugoholgersson
Copy link
Author

e.container was suggested in #33

Right. that is possible to do, but it has downsides...

I agree. I closed #33 because finding the container can be done in pure JS.


  1. An event that just lets the author know that the user triggered spatnav, without caring about details about how the UA would handle it (scrolling vs focusing vs not finding the target)

Yes, that's almost what I want... But I think such an event actually can carry enough details.

  1. Merge the existing 3 events in 1 event, with the reason (focus/scroll/no-target) distinguished by a reason attribute instead of the type.

I realized, we don't actually need a reason-attribute...

First, say we remove the navbeforescroll-event from the spec, #36 as it might not be needed...

That makes it easier to combine navbeforefocus and navnotarget into one navigate-event: We won't need a "reason"-attribute because e.relatedTarget == null would signal "spatnav will move focus nowhere" = navnotarget.

(The navigate-event's target would be the current focus.)


As you say, it's a family of events. Only one thing happens (user moves the mouse), but the browser generates a bunch of events ...

mouseup happens because of an action that's quite different from mousemove's action etc. For spatnav, the triggering action is always the same; "user triggered spatnav".

@anawhj
Copy link
Collaborator

anawhj commented Mar 19, 2018

As I mentioned above, both approaches seem almost equivalent in terms of the functionality. When an arrow key is pushed for spatial navigation, new events should be invoked with the status (scrolling, focus moving, none) and a cancelable option so that authors use the events for several purpose. (e.g. overriding the heuristic spatial navigation, handling some edge cases, etc.)

The event could be invoked as a unified single type (e.g. spatnav or navigate) with an object of three status information (dataTransfer is referred from DragEvent interface) or three event types (navbeforefocus, navbeforescroll, navnotarget) in a single interface. We can get a decision with some advice from DOM event experts. Before that, I agree that we make progress on the three types of event to support some non-primitive features of spatial navigation.

To more easily understand the comparison between two approaches, I put some examples below where an author want to apply nav-rule and nav-loop (non-primitive features) in a container element's event handlers.

1. three types of event handlers (navbeforefocus, navbeforescroll, navnotarget)

function navbeforefocusHandler(event) {
  nav-rule-execute();
}

function navnotargetHandler(event) {
  nav-loop-execute();
}

container.addEventListener("navbeforefocus", navbeforefocusHandler);
container.addEventListener("navnotarget", navnotargetHandler);

2. a single unified event handler (spatnav)

function spatnavHandler(event) {
  switch (event.dataTransfer.type) {
    case 'focus': 
      nav-rule-execute(); break;
    case 'none': 
      nav-loop-execute(); break;
    case 'scroll': 
      return;
  }
}

container.addEventListener("spatnav", spatnavHandler);

I think we could see Drag and Drop events' design to review spatnav events. dataTransfer object could include some useful information such as types, container, and candidates in the direction. In addition, I think we could derive a unified DOM method (e.g. elm.getSpatNavInfo(...)) for getting an object like dataTransfer above.

@frivoal
Copy link
Collaborator

frivoal commented Apr 2, 2018

Based on the discussion above, and using example 5, example 6, and example 7, and in agreement with @jihyerish, I am closing #32 and #36 as no change.

Further discussions with DOM people or the browser community may result in additional API changes, but for now, this is the design we're going with.

@frivoal frivoal closed this as completed Apr 2, 2018
@frivoal frivoal added type:bug Describes a problem with the spec/tests/polyfill in their present form topic:spec closed:rejected Suggestion rejected by the editors labels Apr 2, 2018
@hugoholgersson hugoholgersson changed the title Merge events into "onnavigate"? Merge events into "onnavigate" or "onfocusmove"? Apr 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed:rejected Suggestion rejected by the editors topic:spec type:bug Describes a problem with the spec/tests/polyfill in their present form
Projects
None yet
Development

No branches or pull requests

3 participants