-
Notifications
You must be signed in to change notification settings - Fork 714
[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
Comments
I agree with removing After dropping |
If |
I agree with the overall definition of But the moment when Instead, how about define the CSS property as below? spatial-navigation-action: auto | focus | scroll
(or spatial-navigation-behavior)
( In the future maybe we need the additional
This demo shows how it works. I guess in comparison with your proposal, Also, the definition of "action with the directional input " above will be This way can support the feature of (1) is implemented with (2) can be implemented using |
If But it's much easier if we have the suggesting property, You can see how to implement infinite scroll with the suggested feature on the following page: |
The CSS Working Group just discussed
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 |
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
, andnavnotarget
) 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 fornavbeforescroll
: 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 optionalvisibleOnly
argument defaulting totrue
. Invoking it withvisibleOnly
set tofalse
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 oncontainer
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.The text was updated successfully, but these errors were encountered: