Skip to content

[css-nav-1] changing the spatnav scroll controls from JS to declarative #3401

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

Closed
frivoal opened this issue Dec 6, 2018 · 5 comments
Closed
Assignees

Comments

@frivoal
Copy link
Collaborator

frivoal commented Dec 6, 2018

During TPAC, several engineers from Google (including I believe @eaenet and @chrishtr ) expressed concern over the navbeforescroll event, on the grounds that having to run any kind of synchronous JS right before scrolling (and even more so in the middle of scrolling, as could be the case if the user tries to repeatedly navigate in the same direction) was a performance concern.

Changing from an even to an observer would slightly attenuate that concern as it is cheaper, but not completely as that still involves running JS synchronously at that point.

In theory, I believe the 3 events (navbeforescroll, navbeforefocus, and navnotarget) are necessary and sufficient to be able to intercept spatnav before anything happens. However, in practice, I can only think of one use case that calls for navbeforescroll: the one illustrated in example 5, which allows the authors to switch the behavior from searching for candidates from within just the visible elements in the container to within all elements in the container regardless of visibility.

In the spatial navigation steps, steps 5.1 and 7 invoke finding focusable areas, relying on the optional visibleOnly argument defaulting to true. Invoking it with visibleOnly set to false would switch to the other behavior we're trying to cover here.

Providing a declarative solution to this problem is very easy, and has none of the performance downsides: an inherited css property whose computed value on searchOrigin in step 5.1 or on container in step 7 would act as the switch between the two modes. Strawman name:

spatial-navigation-candidates: visible | all

If we add this, I think we could remove navbeforescroll and not lose any useful capability, solving the performance concern.

@jihyerish
Copy link
Contributor

I agree with removing navbeforescroll event, in terms of the scroll latency.

After dropping navbeforescroll event, when the navnotarget event occurs?
(1) When there isn’t any candidate in the spatial navigation container and it cannot be scrolled at the same time. (current spec)
(2) When there isn’t any visible candidate in the spatial navigation container no matter what it can be scrolled more or not.

@frivoal
Copy link
Collaborator Author

frivoal commented Feb 15, 2019

If spatial-navigation-candidates: visible (the initial value), then it would be (1). If spatial-navigation-candidates: all, then it would be:
(3) when there isn't any candidate (visible or not) in the spatial navigation container no matter what it can be scrolled more or not.

@jihyerish
Copy link
Contributor

jihyerish commented Feb 18, 2019

I agree with the overall definition of navnotarget after dropping navbeforescroll.

But the moment when navnotarget occurs differs from the condition of candidates (all or visible) seems unnatural to me. It causes inconstant behavior when the scroll container can be scrolled more or not.

Instead, how about define the CSS property as below?

spatial-navigation-action: auto | focus | scroll
(or spatial-navigation-behavior)
  • auto: The directional input will move the focus to the visible candidates inside the spatial navigation container. If there isn't any visible candidate, the user can manually scroll the container. (current spec)
  • focus: The directional input will move the focus to candidates(visible or not) inside the spatial navigation container. The user cannot manually scroll the container.

( In the future maybe we need the additional scroll value when the directional input will only scroll the spatial navigation container; moving the focus inside the container doesn't work with the directional input.

  • e.g. The container made by the iframe with RSS feeds. In this case, the author may want to make it scroll, not move the focus inside it. )

This demo shows how it works.

I guess in comparison with your proposal,
spatial-navigation-candidates: visible works the same with spatial-navigation-action: auto
and
spatial-navigation-candidates: all is same with spatial-navigation-action: focus.

Also, the definition of navnotarget would be changed to "The event occurs when there won't be any action with the directional input for the spatial navigation container before going up the tree to search candidates in the nearest ancestor spatial navigation container".

"action with the directional input " above will be
moving the focus and scrolling if the container is specified as spatial-navigation-action: auto,
or
just moving the focus (without scrolling) if the container is specified as spatial-navigation-action: focus.

This way can support the feature of navbeforescroll and solve the performance issue.
More detail about the feature of navbeforescroll, there will be
(1) Move the focus directly to the invisible candidates in the spatial navigation container
(2) Move the focus directly to a virtual candidate which will be created and added inside the spatial navigation container when the scroll reaches to the end (as the virtual scroller).

(1) is implemented with spatial-navigation-action: focus.

(2) can be implemented using navnotarget and spatial-navigation-action: focus as below:
i) when the spatial navigation container (the related target of navnotarget) is specified with spatial-navigation-action: focus
ii) navnotarget occurs if there isn't any candidate (visible or not) in the spatial navigation container no matter what it can be scrolled more or not

@jihyerish
Copy link
Contributor

jihyerish commented Feb 25, 2019

If navbeforescroll is removed, there will be no way to block scrolling behavior.
Therefore it's hard to implement the infinite scroll in the page with the spatial navigation.

But it's much easier if we have the suggesting property, spatial-navigation-action because,
spatial-navigation-action: focus can separate the native scroll and the spatial navigation behavior.
We have navnotarget to know the end of the current scroller and prevent the focus to move out of the spatial navigation container.

You can see how to implement infinite scroll with the suggested feature on the following page:
https://raw.githack.com/jihyerish/spatial-navigation/spatnav-behavior/demo/infinite-scroller/index.html

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed drop navbeforescroll, and agreed to the following:

  • RESOLVED: drop navbeforescroll and add `spatial-navigation-action` with at least two values
The full IRC log of that discussion <astearns> topic: drop navbeforescroll
<astearns> github: https://github.com//issues/3401
<emilio> jihye: we have three events, navbeforescroll triggers when there's no visible focusable element in the scroll container
<emilio> jihye: it has some perf issues so we want to drop it, but there are some use cases like moving the focus directly using arrow keys or focusing the non-visible content in the scroll container
<jihye> https://raw.githack.com/jihyerish/spatial-navigation/spatnav-behavior/demo/scroller/index.html
<emilio> jihye: so we'd like to propose a new CSS property, spatial-navigation-behavior, see ^ for demo
<emilio> jihye: this property is aimed to split scrolling and moving the focus behavior, to avoid scroll latency
<jihye> https://raw.githack.com/jihyerish/spatial-navigation/spatnav-behavior/demo/infinite-scroller/index.html
<emilio> jihye: and it helps to implement the use-cases like infinite scrolling
<emilio> jihye: we have to decide exact naming and values, and I wanted to gather opinion about whether the new property is reasonable
<emilio> eae: I think it's a very good idea, and it helps to avoid preventing composited scrolling, so I'm very happy to see you're moving in this direction
<emilio> florian: initially we wanted to go with events only rather than declarative syntax, so people could experiment
<emilio> florian: but given the only use-case for this event can be done declaratively, and it solves the perf issue that concerned google, it sounds like a good idea
<emilio> eae: yeah, thanks for coming up with that proposal
<emilio> florian: we've iterated quite a bit on naming, last proposal is spatial-navigation-action: auto / focus / scroll
<emilio> florian: normal behavior is a combination of focus and scroll
<emilio> florian: if you use focus, it'll just jumps across focusable elements
<emilio> florian: instead of scrolling step by step, like auto
<emilio> florian: scroll is the opposite, it will just scroll it and skip focusable elements inside
<emilio> florian: so for example if you have a focusable scroller with focusable things in it, in which you're not strongly interested (like a twitter feed in the page), you could use the scroll value to avoid going into focusing the descendants until you focus one of them
<emilio> florian: we didn't feel strongly that we needed `scroll`, but having it now helps the bike-shedding for names and such
<emilio> astearns: would it make sense to have three values with scroll noted like "maybe dropped"
<emilio> jihye: maybe just add it later
<emilio> astearns: since the value helped you, maybe better either leaving it out and noting that it might be added, or the opposite
<emilio> florian: if somebody finds a better name, then even better
<emilio> bkardell_: so currently this demo works setting some DOM properties?
<emilio> jihye: I showed two demos, which one?
<emilio> bkardell_: let's take spatial-navigation-action? Is it just using a dom property in the demo?
<emilio> florian: the demo uses a polyfill?
<emilio> s/?//
<emilio> bkardell_: I was wondering how it was implemented, custom props?
<emilio> jihye: yeah, it's a custom property
<florian> s/polyfill?/polyfill/
<emilio> RESOLVED: drop navbeforescroll and add `spatial-navigation-action` with at least two values

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

4 participants