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

API and Property Bikeshedding #17

Closed
frivoal opened this issue Feb 16, 2018 · 12 comments
Closed

API and Property Bikeshedding #17

frivoal opened this issue Feb 16, 2018 · 12 comments
Labels
closed:resolved Changes have been made based on this issue topic:spec type:question Known unknows: things we need to investigate

Comments

@frivoal
Copy link
Collaborator

frivoal commented Feb 16, 2018

The property names and API names used in the spec are probably OK, as they described what they do reasonably well, but I would not go as far as calling them nice.

Better suggestions are very welcome.

  • In particular, spatnav-container is a bit questionable. Maybe we should not use the abbreviation, but if we called it spatial-navigation-container it would look like it may be a longhand of spatial-navigation, which it is not. Or maybe it is spatial-navigation that should be renamed into spatnav-something...

  • The event names (navnotarget navbeforefocus navbeforescroll) aren't too pretty either: the convention that event names should be all lower case without hyphens or underscrores does limit how nice they can be though.

  • The JS APIs are pretty descriptive and tied to the name of algorithms in the processing model, but they are quite a mouthful.: findSpatNavCandidates() selectSpatNavBestCandidate() sequentialNavSearch(). Are we happy with that?

@hugoholgersson
Copy link

Some ideas:

spatial-navigation: auto|active -> key-navigation: auto|active|jailed;

"jailed" would be shorthand for also creating a spatnav group. That makes me wonder; do we really need two properties?

selectSpatNavBestCandidate() -> closestFocusableArea(), see my feedback in #30.

findSpatNavCandidates() -> findFocusableAreas(), see #31.

navnotarget
navbeforefocus
navbeforescroll -> navigation, see #32.

@frivoal
Copy link
Collaborator Author

frivoal commented Feb 19, 2018

spatial-navigation: auto|active -> key-navigation: auto|active|jailed;

"jailed" would be shorthand for also creating a spatnav group. That makes me wonder; do we really need two properties?

Interesting. I like the naming on the value side, and this is probably enough for 99% of the cases (minus the ability to set jails without activating spatnav, but that can be added back later with longhands, and it's unclear how important that is).

On the property side, I think I'd rather call it "spatial-navigation" or "spatnav" than "key-navigation" though, because:

  1. key-navigation may mislead people into thinking this has something to do with the Tab key, which it does not
  2. Although spatnav is mostly about keyboard navigation it doesn't have to be limited to that. If you have a joystick or a gamepad, it could very well trigger spatnav. A bit more farfetched, voice commands or gestures also could. None of these would be well described by "key-navigation", but they would still pay attention to this property (at least for the jailed value, and possibly also for the active value if they're not always on).

I'd prefer spatnav to spatial-navigation, because it is less typing, but I don't care strongly either way.

However, it raises some interesting questions about the behavior.

property inheritance

auto vs active is a thing that inherits, jailed vs not jailed is a thing that doesn't.

We could say that spatnav (a.k.a. spatial-navigation or key-navigation) is a shorthand for spatnav-activation: auto | active and spatnav-jail: auto | jailed with auto -> auto+auto, active -> active+auto and jailed -> active+jailed, while saying that spatnav-activation is inherited, and spatnav-jail is not. This combination of inherited and non-inherited into a single shorthand is a bit unusual, but there's not reason it would be hard to implement and I believe there's precedent.

We can even do it without exposing the long hands, by describing "spatial-navigation" as a non inherited property, with special logic to handle the computed value of auto: if the parent is jailed or active, then the computed value of auto is active, else it is auto. There's precedent for that too. That's effectively equivalent to the above (minus the ability to set jails without activating spatnav).

I think I like this option. @jihyerish, what do you think? Also, spatnav or spatial-navigation or key-navigation?

We can possibly mark the longhands at risk and have a note explaining how you do this if you implement only the shorthand.

changing semantics of activation

An alternative to the above would be to make the property completely non inherited, but that requires a change in the semantics of how activation works.

If spatial-navigation: active is not inherited, then looking at the computed value of spatial-navigation on the current element is no longer enough to know if spatnav is active on that element or not. You'd need to look up the tree to find an ancestor with active (or jailed): you're active if you find one, not if you don't.

I think I like this alternative less, because it removes one thing which I suspect may be important: inside a page in which spatnav is generally activated, the ability for a sub-component to not activate it. I think the situation where a spatnav-using page wants to use a third party component that doesn't play well with spatnav will not be all that rare. We could restore that possiblity by adding an off value separate form auto, but that doesn't sound great: if you're on a device where spatnav is the only possible type of input, off would not actually turn it off. In that case, off means "off, unless you have no choice, in which case, activate", and auto means "look at my parent".
That seems messy.

I think we could specify it this way (which is why I've detailed it here), but I don't like it.


@hugoholgersson
Copy link

key-navigation may mislead people into thinking this has something to do with the Tab key, which it does not.

In chromium, spatnav shares quite some code with Tab-key navigation. So from implementors' perspective, it's not too off. Both advance focus.

If you have a joystick or a gamepad, it could very well trigger spatnav.

These have keys as well :)

A bit more farfetched, voice commands or gestures also could [trigger spatnav].

Voices also have keys :) although of a different kind...

We can possibly mark the longhands at risk and have a note explaining how you do this if you implement only the shorthand.

Perhaps it's better to leave the two longhands out from start? A smaller spec is easier to reason about, to study, to convince vendors about, to implement, to test, to ship ... and, in case it turns out to be a fiasco, to deprecate. Same argument applies to #32.

@Malvoz
Copy link
Contributor

Malvoz commented Feb 20, 2018

@frivoal

spatnav or spatial-navigation or key-navigation?

IMO either spatial-navigation or key-navigation over spatnav . spatnav is easy to write, but looks worse than the others.

@hugoholgersson

key-navigation: auto|active|jailed;

I don't oppose to jailed, but what about following the same pattern as overscroll-behavior and use contain? E.g. key-navigation: contain; or contained? But then there is the containment property named contain also. Just throwing it out.

@jihyerish
Copy link
Collaborator

jihyerish commented Feb 21, 2018

@hugoholgersson

spatial-navigation: auto|active -> key-navigation: auto|active|jailed;
"jailed" would be shorthand for also creating a spatnav group. That makes me wonder; do we really need two properties?

@frivoal

I think I like this option. @jihyerish, what do you think? Also, spatnav or spatial-navigation or key-navigation?

key-navigation can be presumed as a property for both the sequential navigation (by tab key) and the spatial navigation (with arrow keys).
key-navigation: active; seems ambiguous because the sequential navigation is always active.
I prefer to have 'spatnav' or 'spatial' for the name of the property.
Also, I realized that the value 'contain' may be confusing, so I like 'jailed'.

The reasons for proposing spatial-navigation property for activating the spatial navigation and spatnav-container property for creating a spatnav group (in the spec, spatnav containers) are:

  • Simplifying the feature of one property
    • Avoiding that one property has multiple features.
  • Grouping the element is a stand-alone feature from activating the spatial navigation.
    • The spatnav group specifies the priority for searching the best candidate (the next focus target).
      • When searching the best candidate (the next focus target) from the currently focused element, the element which is in the same group takes higher priority than others.
      • There are use cases that the whole page activated for the spatial navigation has several spatnav groups.
    • The spatnav group is possible to use for all kinds of focus navigation.
      • It is usable irrespective of the input method.

Ultimately, I like @florian's suggestion:
Having shorthand with no-inheritance and separate properties for activating the spatial navigation and creating a spatnav group.

@hugoholgersson
Copy link

Other naming ideas for the shorthand:

focus-navigation: auto|active|jailed <-- sounds generic enough, doesn't it?
navigation-aid: auto|active|jailed
focus-aid: auto|active|jailed

But instead of active, I imagine spatial could give readers a better hint.

focus-navigation: auto|spatial|jailed

@frivoal
Copy link
Collaborator Author

frivoal commented Feb 21, 2018

In chromium, spatnav shares quite some code with Tab-key navigation. So from implementors' perspective, it's not too off.

The underlying implementation of spatnav and sequential nav do share some code, but the effects of this property is not on both, so it should not be named in a way that suggests it affects both, otherwise we'll just confuse people. Unless you want to also change its effects to something that does have an impact on both, but I don't think you've mentioned anything to that effect.

I don't oppose to jailed, but what about following the same pattern as overscroll-behavior and use contain? E.g. key-navigation: contain; or contained? But then there is the containment property named contain also. Just throwing it out.

contain, container, containing and so on are frequently used used terms in CSS (grid container, flex container, block container, initial containing block...). I think reusing familiar terminology is good. And it is the word we use in the spec prose anyway, so let's not shy away from it.

spatnav is easy to write, but looks worse than the others.

I'm very used to it, so I don't mind, but I can see that it looks weird to people who don't have to say that work 50 times per day :)

[paraphrasing @jihyerish]: grouping is something you want to be able to decide on separately from activation.

I agree. That argues in favor of having the longhands as well. Even if people may just use the shorthand in most cases, the longhand are there when needed.


With all that in mind, my preference goes to:

  • Shorthand: spatial-navigation: auto | active | contain

  • Longhands:

    • spatial-navigation-activation: auto | active
    • spatial-navigation-contain: auto | contain
  • Mapping:

    spatial-navigation spatial-navigation-activation spatial-navigation-contain
    auto auto auto
    active active auto
    contain active contain

Unless someone has strong objections against that, I'm going to put it in the spec shortly. Even if we later find something even better, this seems a genuine improvement over what's in there currently anyway.

@hugoholgersson
Copy link

Why not call it focus-navigation or, say focus-moving (shorter but more vague)? @frivoal

Isn't "focus" more descriptive than "spatial"... ? It's shorter too.

My objection is that I've always found the word "spatial" a bit too spacy. It never made me think of "directional focus movement". What do you think? @jihyerish @Malvoz

I think reusing familiar terminology is good.

I see a risk when one keyword means different things in different contexts.


We can even do it without exposing the long hands, by describing ... @frivoal

Yes. And since we're all uncertain whether the longhands will actually be useful or not in the end, let's leave them out for now? Again, I'm only trying to minimize this spec's scope to make it easier to implement. More options, more complexity. @jihyerish, would it be OK if we leave them out?

@frivoal
Copy link
Collaborator Author

frivoal commented Feb 21, 2018

Isn't "focus" more descriptive than "spatial"...

Focus navigation implies that it also relates to sequential focus navigation. I would not be against that change in the name the the spec as a whole, since it does talk about both.

But this property is not about both, so "focus" alone isn't good. directional-focus-navigation or spatial-focus-navigatotion may be ok, but that's really long. And incidentally, since it can also trigger scrolling if there's nothing to focus, it's not only about focus navigation. So I think we should leave the word out.

My objection is that I've always found the word "spatial" a bit too spacy. It never made me think of "directional focus movement"

Spatial and Directional are effectively synonyms here, so replacing spatial with directional would not be wrong. It's just a question of what most people find easier to understand / remember. You prefer directional, so that's one data point. But https://en.wikipedia.org/wiki/Spatial_navigation talks about the concept we're using, while https://en.wikipedia.org/wiki/Directional_navigation does not exist. Since it is established terminology, I am not too keen on changing it.

Again, not totally against, since directional isn't wrong, but going with established terminology seems the path of least resistance.

Yes. And since we're all uncertain whether the longhands will actually be useful or not in the end, let's leave them out for now?

The path without longhands reduces the API surface, but it does not reduce implementation complexity (since you effectively have to reproduce that internally to handle the mixed inherited / non inherited behavior), nor does it reduce spec complexity (since explaining how the computed value logic for a single property works is not easier than declaring the longhands).

How about that: I put the longhands in, and mark them at risk, to signal that they may be dropped if implementer would rather skip them. That documents the full idea, without forcing anyone's hand.

@Malvoz
Copy link
Contributor

Malvoz commented Feb 21, 2018

@frivoal

spatnav is easy to write, but looks worse than the others.

I'm very used to it, so I don't mind, but I can see that it looks weird to people who don't have to say that work 50 times per day :)

I don't think spatnav looks bad but it is abbreviated while the majority of CSS properties aren't, does that speak against it? On the other hand, there are abbreviated properties defined in CSS-UI-4 Directional Focus Navigation: nav-up, nav-right, nav-down and nav-left.

For consistency across navigational properties, keep "navigation" abbreviated? Resulting in:

Shorthand:
spatial-nav: auto | active | contain

Longhands:
spatial-nav-activation: auto | active
spatial-nav-contain: auto | contain


@hugoholgersson

key-navigation: auto|active|jailed;

I don't oppose to jailed, but what about following the same pattern as overscroll-behavior and use contain? E.g. key-navigation: contain; or contained? But then there is the containment property named contain also. Just throwing it out.

I see a risk when one keyword means different things in different contexts.

Take the auto value for example, it can mean plenty of things depending on to which property it belongs, so I don't agree. Developers should know the values for a property before using them 😉

@hugoholgersson
Copy link

[spatnav] can also trigger scrolling if there's nothing to focus,

Conceptually, I see the scrolling as something that happens because spatnav had "hijacked" the arrow-keys. When it can't find anything, it returns control to the "normal arrow-key behavior", which is scrolling. I assume arrow-key scrolling is already defined elsewhere so we don't need to redefine it in this spec?

Focus navigation implies that it also relates to sequential focus navigation.

I see spatnav and tab-key navigation as two different methods for moving focus.

But this property is not about both...

Ah, right, ok... maybe that's a problem? I could imagine a property like focus-move: auto|all|contain|off to control both being even more useful.

all = arrow-keys aka spatnav + tab key navigation (if both are available).
off = disable focus advancements within a sub tree (avoids sprinkling tabindex=-1).

The path without longhands reduces the API surface, but it does not reduce implementation complexity

Hmm. One property means one enum in C++. One enum to check = low complexity? Inheritance can be checked by walking the DOM up to its root.

frivoal added a commit to frivoal/spatial-navigation that referenced this issue Mar 20, 2018
While it may be nice to have and doesn't cost much on top
of the processing model, it is non essential, and can be deferred to
level 2.

Keeping it in the spec for now was distracting:
not only were there open issues (especially naming) related to it,
but distracted readers from the essential point of this specification.

Closes WICG#16.

Addresses part of WICG#17.
frivoal added a commit to frivoal/spatial-navigation that referenced this issue Mar 20, 2018
While it may be nice to have and doesn't cost much on top
of the processing model, it is non essential, and can be deferred to
level 2.

Keeping it in the spec for now was distracting:
not only were there open issues (especially naming) related to it,
but distracted readers from the essential point of this specification.

Closes WICG#16.

Addresses part of WICG#17.
frivoal added a commit to frivoal/spatial-navigation that referenced this issue Mar 22, 2018
At the same time, I have renamed it to the better naming discussed in WICG#17
@frivoal
Copy link
Collaborator Author

frivoal commented Apr 2, 2018

Various changes based on this issue have been committed to the specification. Further bikeshedding should be discussed on a case by case basis. Closing.

@frivoal frivoal closed this as completed Apr 2, 2018
@frivoal frivoal added type:question Known unknows: things we need to investigate 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:question Known unknows: things we need to investigate
Projects
None yet
Development

No branches or pull requests

4 participants