Open PhrozenByte opened 7 months ago
netlify[bot]
commented
7 months ago | Name | Link |
|---|---|
| Latest commit | 0e59b315bd15df24ffdb0dd5706860aa983dcec0 |
| Latest deploy log | https://app.netlify.com/sites/selfoss/deploys/656354b30c9430000808274b |
jtojnar
commented
7 months ago Keyboard navigation is virtually impossible with
scroll_to_article_header=0, because buttons likeSpacealso move the view point.
I cannot reproduce this in either Firefox or Chromium – we are already preventing the default browser behaviour:
The only time the space should move the viewport is when the next item does not fit into the viewport:
https://github.com/fossar/selfoss/blob/e0c9805050e83351059e19d56a0cd36680c35e58/client/js/templates/EntriesPage.jsx#L1001-L1001 https://github.com/fossar/selfoss/blob/e0c9805050e83351059e19d56a0cd36680c35e58/client/js/helpers/navigation.js#L18
but I believe that visually scrolling to the selected entry always is the expected behaviour when using keyboard navigation.
Hmm, I guess that depends on how you use selfoss. Personally, I read multiple item titles in a row and only open those that I find interesting – when it opens in place, I can just continue reading on the next line. Of course there will still be jump when near the end but that is inevitable.
Also, as is the PR makes it scroll to article header even when clicking a title, not just for keyboard navigation.
PhrozenByte
commented
7 months ago Something weird is going on here :see_no_evil: I definitely once had the issue that Space was moving the viewport, but I no longer can reproduce that. Maybe it was just some intermediate issue while developing my other PRs.
However, I did write this fix after all my other PRs, thus there definitely still is a similar issue with moving the viewport - and after some research I managed to reproduce it. Due to your explanations (thanks! :+1:) I now also understand what the issue is: It's not that Space moves the viewport, but there's an issue with autoScroll(), so I might just have mixed these up.
Assume you have four entries: The 1st, 2nd and 4th all consist of just a single line of content. The 3rd in contrast is huge, longer than the screen. You hit Space and the first entry opens, easily fitting into the screen. You hit Space again, collapsing the first and opening the second entry; both still easily fitting into the screen. You hit Space again, the second entry collapses and the third entry opens, now overflowing the screen. Hitting Space again sends your viewport way down, because it calculates the target viewport based on this huge 3rd entry that wasn't collapsed yet, but which is about to collapse.
In native JavaScript we just have to wait for another animation frame, i.e. wrap this
into window.requestAnimationFrame(() => { /* */ });. I just commited that (see force-push of 0e59b315bd15df24ffdb0dd5706860aa983dcec0) - and it works now.
However, I still know next to nothing about React, so I might rather use some feature provided by React instead, I just don't know which. Just let me know, I'll change it accordingly. :+1:
Also, as is the PR makes it scroll to article header even when clicking a title, not just for keyboard navigation.
This happens when one writes patches in a hurry and doesn't properly test it :unamused: Sorry about that.
Keyboard navigation is virtually impossible with
scroll_to_article_header=0, because buttons likeSpacealso move the view point. We could also prevent the browser from moving the view point, but I believe that visually scrolling to the selected entry always is the expected behaviour when using keyboard navigation.