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

Rename findSpatNavCandidates() to findFocusableAreas() #31

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

Rename findSpatNavCandidates() to findFocusableAreas() #31

hugoholgersson opened this issue Feb 16, 2018 · 4 comments
Labels
closed:resolved Changes have been made based on this issue topic:spec type:bug Describes a problem with the spec/tests/polyfill in their present form

Comments

@hugoholgersson
Copy link

Currently, the suggested API could be used like this:

anyelement.findSpatNavCandidates({dir: "left", somecontainerdiv});

I'd like to flip the perspective. Now we expose this method on the element from where we want to "measure reachability" of candidates. I'd like to call this method on the node whose sub tree I want to search. I think that would result in JS that's slightly more pleasant to read.

div.findFocusableAreas() // Returns all visible focusables within the div.

div.findFocusableAreas("left") // Visible focusables within the div that are left of document.activeElement().

div.findFocusableAreas("left", startelement) // Visible focusables in the div that are left of startelement.

Question: What if startelement isn't visible?

I think that reads better than the current draft's: "If c is null, set c to the nearest ancestor of eventTarget that is a spatnav container."

@frivoal
Copy link
Collaborator

frivoal commented Feb 19, 2018

I think there are two parts in your proposal:

  1. Omitting the direction means the same thing as specifying direction inside: look for all visible focusable areas in the container, regardless of direction.

    I'm OK with that. It is less verbose, and the semantics are intuitive. It works regardless of whether we take suggestion 2 as well or not (although I guess it is most compelling if we do take suggestion 2 as well). @jihyerish, what do you think?

  2. Between the container and the start element, swap with is the "this" parameter and which is the optional method argument.

    I'm not necessarily opposed to this, but I am a bit less sure: one the one hand, used out of context, maybe this is indeed a more intuitive way of phrasing it. On the other hand, if you're calling it from an "navbeforefocus" navigation event (regardless of whether it is merged and/or renamed as discussed in Merge events into "onnavigate" or "onfocusmove"? #32) — for example because you're trying to polyfill nav-rule — you have a handle on the start element, but you don't have one on the container. In that context, making the container mandatory and the start element optional is not super nice. Finding the container isn't super hard (it's just a walk up the tree), but if the browser could do it for you, that's even nicer.

    @jihyerish has suggested we may add a method for finding the container starting from an arbitrary element. I agree we could do that, and that would make using your suggested "flipped perspective" syntax easier to use

    something.addEventListener("navbeforefocus",funciton(e) {
      var container = e.target.getSpatnavContainer();
      var candidates = container.findFocusableAreas(e.dir, e.target);
      /* ... */
    });

    The reason I am not a little skeptical is that if the vast majority of invocations of this function end up looking like that, we might as well leave the container be an optional argument as currently designed. If on the other hand people frequently start from the container, then your syntax if more friendly.

    All in all, I'm not opposed, but I think "override some part of the spatnav logic from event handlers to write a polyfilll or extend the logic" is a realistic use case, which supports the current syntax. Do we have use cases that support the idea that starting with the container rather than the element may be a reasonably common thing to want?

@hugoholgersson
Copy link
Author

for example because you're trying to polyfill nav-rule — you have a handle on the start element, but you don't have one on the container.

We could include the used spatnav-container in an container-attribute of NavigationEvent. That might be something we wanna do regardless? See #33.

I think we better not let the container-argument be optional. That helps authors think about containers before using the API. Still, lazy developers can just do body.findFocusableAreas().

When a second, not so lazy, developer comes along, he/she could otherwise wonder "Was this really the intended search container?" "Did we forgot to specify a container here"?.

@jihyerish
Copy link
Collaborator

jihyerish commented Feb 27, 2018

@hugoholgersson

div.findFocusableAreas() // Returns all visible focusables within the div.
div.findFocusableAreas("left") // Visible focusables within the div that are left of document.activeElement().
div.findFocusableAreas("left", startelement) // Visible focusables in the div that are left of startelement.
Question: What if startelement isn't visible?

Though startelement is invisible, findFocusableAreas() returns visible focusables (candidates).
I think this API would work in the same way as pressing the arrow keys to move focus.
For example, if the startelement in the scrollable container becomes invisible after scrolling and

  1. there is any visible focusable within the container, navbeforefocus event occurs.
  2. there isn't any visible focusable within the container, but the container can be scrolled, navbeforescroll event can occur.
  3. there isn't any visible focusable within the container and the container cannot be scrolled, navnotarget event can occur.

At this point, I have another question. What if the container isn't visible?
I think in this case the API would return null or all focusables which are not only visible but also invisible.

We could include the used spatnav-container in an container-attribute of NavigationEvent. That might be something we wanna do regardless? See #33.

This is a good idea!
But I assume this suggestion can't cover all the cases in this issue because findSpatNavCandidates() (or findFocusableAreas()) doesn't always trigger NavigationEvent.
If there is any visible focusable within the container, the API doesn't trigger navbeforefocus, but selectSpatNavBestCandidate() does.

@florian

I'm OK with that. It is less verbose, and the semantics are intuitive. It works regardless of whether we take suggestion 2 as well or not (although I guess it is most compelling if we do take suggestion 2 as well). @jihyerish, what do you think?

I'm inclined to let the startelement mandatory and container optional.

Unless there is an API for finding the container, making the container mandatory seems less intuitive.

for example because you're trying to polyfill nav-rule

To polyfill nav-rule means to replace selectSpatNavBestCandidate() . Do I understand right?

@hugoholgersson
Copy link
Author

hugoholgersson commented Feb 27, 2018

findFocusableAreas() doesn't always trigger NavigationEvent ... but selectSpatNavBestCandidate() does.

My understanding is that none of the JS methods triggers NavigationEvent. They are only used to query information (but not change state). Authors will still move focus with element.focus().

The JS API will typically be used in a navigation-handler to override the browser's built-in focus movements.

To polyfill nav-rule means to replace selectSpatNavBestCandidate() . Do I understand right?

Hmm... Maybe not. To polyfill nav-rule, I think we only really need to provide a cancelable navigate-event. findFocusableAreas() would help devs find the visible focusables to consider, but I think it could be done without findFocusableAreas() as well.

Unless there is an API for finding the container,

Finding the parent scroller could be done manually [1] [2]. One could do if (myscroller.contains(e.relatedTarget)) to hande certain scrollers specifically. I'd guess typical web apps have <= 3 scrollers to handle so I think the resulting JS won't be too messy.

... making the container mandatory seems less intuitive.

I like it because it resembles querySelectorAll's syntax.

frivoal added a commit to frivoal/spatial-navigation that referenced this issue Mar 20, 2018
By moving the stepts that take the direction and starting point into
account from finding candidates to selecting the best one,
the "finding candidates" method can be made more generic.

This patches does that, and renames the relevant algorithm and API
accordingly.

This includes a convenience method to grab the nearest spatnav container
ancestor.

Closes WICG#31
frivoal added a commit to frivoal/spatial-navigation that referenced this issue Mar 20, 2018
By moving the stepts that take the direction and starting point into
account from finding candidates to selecting the best one,
the "finding candidates" method can be made more generic.

This patches does that, and renames the relevant algorithm and API
accordingly.

This includes a convenience method to grab the nearest spatnav container
ancestor.

Closes WICG#31
frivoal added a commit to frivoal/spatial-navigation that referenced this issue Mar 20, 2018
By moving the stepts that take the direction and starting point into
account from finding candidates to selecting the best one,
the "finding candidates" method can be made more generic.

This patches does that, and renames the relevant algorithm and API
accordingly.

This includes a convenience method to grab the nearest spatnav container
ancestor.

Closes WICG#31
frivoal added a commit to frivoal/spatial-navigation that referenced this issue Mar 20, 2018
By moving the stepts that take the direction and starting point into
account from finding candidates to selecting the best one,
the "finding candidates" method can be made more generic.

This patches does that, and renames the relevant algorithm and API
accordingly.

This includes a convenience method to grab the nearest spatnav container
ancestor.

Closes WICG#31
@frivoal frivoal added type:bug Describes a problem with the spec/tests/polyfill in their present form topic:spec closed:resolved Changes have been made based on this issue labels Apr 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed:resolved Changes have been made based on this issue 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