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

Slide away UI elements on scroll #1140

Merged
merged 17 commits into from
Oct 30, 2023
Merged

Conversation

Jaifroid
Copy link
Member

Closes #1139.

It's still a bit rough, but working in essence.

@Jaifroid Jaifroid added this to the v4.0 milestone Oct 27, 2023
@Jaifroid Jaifroid self-assigned this Oct 27, 2023
@Jaifroid Jaifroid marked this pull request as ready for review October 28, 2023 10:27
@Jaifroid
Copy link
Member Author

Seems ready. Checked on IE11 and Firefox OS. Need to check on Edge Legacy.

@Jaifroid
Copy link
Member Author

@Rishabhg71 Would you be able to test this and see if you think UX is good?

Any comments on the code would be appreciated, but optional!

If you don't have any comments and are happy with it, please approve. Thanks!

www/js/app.js Outdated Show resolved Hide resolved
@Jaifroid
Copy link
Member Author

PS, just so you're aware, sliding away of header/footer is disabled if the search bar (prefix) has focus. Since prefix is automatically focused on loading the landing page of a ZIM, it means that the landing page won't show the scroll effect unless you click somewhere in the article. Subsequent articles will work without the extra click.

@Jaifroid
Copy link
Member Author

Jaifroid commented Oct 28, 2023

Tests:

  • IE11
  • Edge Chromium
  • Firefox OS
  • Firefox
  • Edge 15 (Windows Mobile): the header remains hidden under the iframe until the user scrolls back to the very top of the document. I probably need to set the z-index to 1 Fixed with z-index
  • Edge 16
  • Edge 18

@Rishabhg71
Copy link
Collaborator

@Jaifroid
image
I have seen this behavior a lot of times usually refreshing the page the load it up properly. This only happens when running the vite server. Any idea why + Am I the only one getting this behavior?
try visiting this URL (GH codespaces) https://musical-yodel-xvpjvv4pwpvcprpp-5173.app.github.dev/www/index.html

@Jaifroid
Copy link
Member Author

Yes, that's a known issue with the vite server, and is usually fixed by ensuring Bypass AppCache setting is turned ON. When it's on, and when using vite server, it's not necessary to refresh the page when you make a change, because the server forces the browser to refresh. However, if you do get the disordered display, then a refresh will fix it, or sometimes Ctrl-Shift-R with devtools in focus.

This is nothing to do with this PR/branch, it's because Vite uses esbuild as a fast bundler when using Hot Module Replacement, whereas it uses rollup for production builds (but I don't actually use Vite's version of rollup, because it's configured way too aggressively -- we just use pure rollup for our bundling).

@Jaifroid
Copy link
Member Author

You can use http-server instead if it's bothersome (npm run web-server).

@Rishabhg71
Copy link
Collaborator

This is nothing to do with this PR/branch, it's because Vite uses esbuild as a fast bundler when using Hot Module Replacement, whereas it uses rollup for production builds (but I don't actually use Vite's version of rollup, because it's configured way too aggressively -- we just use pure rollup for our bundling).

I had an idea it was a problem with vite because the normal HTTP server works now coming to the main I feel UI/UX is good but IMHO I found it annoying that every time I scroll up and down. A simple solution is to only show the header/footer if a minimum distance has been crossed by the scroll wheel.
On a side note, There is a weird bug on Brave where the header hides but leaves that red area It might be an issue on Brave or all Chromium-based browsers. I know we don't officially support Brave so maybe we can leave it

2023-10-28.20-13-06.mp4

@Jaifroid
Copy link
Member Author

On a side note, There is a weird bug on Brave where the header hides but leaves that red area It might be an issue on Brave or all Chromium-based browsers. I know we don't officially support Brave so maybe we can leave it

Oh, well it's definitely not the case with all Chromium browsers, as it doesn't show on Edge. I'll check on Chrome. However, I should try to fix this, unless it really is a bug with Brave. I'll check.

A simple solution is to only show the header/footer if a minimum distance has been crossed by the scroll wheel.

Yes, I could try that. However, I think that should apply to scrolling down (in fact I did make it that the user needs to scroll at least 50 pixels from the top, but not on subsequent scrolls). On scrolling up, it's more likely the user wants to see the header so they can go somewhere else or search for something, so they might be annoyed if it doesn't appear straight away.

Anyway, thanks for checking, and I'll see if I can make improvements.

(By the way, if a user is really annoyed, they can always turn the feature off in Configuration.)

@Jaifroid
Copy link
Member Author

Quick question, do you think this feature should be off by default, and users have to turn it on if they want it?

I was kind of taking my cue from the Android app where it's always on (and I don't think it can be turned off).

@Rishabhg71
Copy link
Collaborator

Quick question, do you think this feature should be off by default, and users have to turn it on if they want it?

Turning this feature on by default makes more sense. Most of the users don't care for these minor inconveniences and also get used to them after some time. One more suggestion you can try playing around with the animation time I feel it's a little on the slow side

@Jaifroid
Copy link
Member Author

So I've done these changes:

  • Animation reduced to 0.3s instead of 0.5s. You're right, this makes it feel more responsive.
  • User has to scroll down at least 50 pixels (from any starting position) to activate hiding. This means that really slow scrolling won't activate it (due to my intentional throttling of the onscroll event), but I think that's quite good because the result is that you need a slightly more decisive scroll in order to activate it. Keyboard down-arrow mostly activates it, but occasionally only after second down-arrow. Scrolling up doesn't have the 50 pixel margin, because users will usually be doing that after they've finished reading and want to show the header/footer so they can navigate.
  • I now only apply the z-index and position: relative workaround in IE11 and Edge Legacy (only Edge Legacy actually needs it).
  • I think the last change (z-index) has fixed the Brave bug. At least, I can't reproduce it now using portable Brave (https://portapps.io/app/brave-portable/) installed in Windows Sandbox. I believe having changed the position of the header from static to relative is what caused the Brave bug. It's back to static for all browsers except IE11 and Edge Legacy.

www/js/init.js Outdated Show resolved Hide resolved
@Jaifroid
Copy link
Member Author

@Rishabhg71 If you're happy that the changes address the issues you identified, I'd be grateful if you could approve the PR! Thanks for helping to improve this PR -- it's always good to have a second opinion, especially for UI changes which can be quite contentious for users.

@Jaifroid
Copy link
Member Author

Jaifroid commented Oct 29, 2023

Last tests:

  • Firefox 64 JQuery and SW modes
  • FFOS double-check - see bug in comment below, now defaults to off for this OS + warning
  • IE11 double-check
  • Edge legacy check links issue - links work, elements of header need z-index setting (WONTFIX)
  • As a Chromium extension
  • As a Firefox extension - MV2 and MV3

@Jaifroid
Copy link
Member Author

Jaifroid commented Oct 29, 2023

Encountering a bug on Firefox OS whereby links clicked with a mouse stop working after a few clicks. Touch still works. Very odd. Need to check if this is also happening on main, to see if this PR caused it.

EDIT: It's intermittent, but it's definitely caused by this PR. Some links just don't register, always the same ones, but they do register on main. Since we use FFOS as a proxy for issues that could be encountered in other older Firefox browsers, I need to look into this.

@Jaifroid
Copy link
Member Author

Jaifroid commented Oct 29, 2023

FFOS bug appears to be triggered when the transition is set on the iframe. It appears to cause some href elements in the document to lose their click event listeners, though touch still works.. It doesn't happen if the feature is turned off.

It might be time to implement the first bullet point of #725 -- in fact #732, which is an overdue simplification of link handling, and probably really quick to do. I'll test.

@Jaifroid
Copy link
Member Author

OK, so I've determined that #732 doesn't fix this. It's caused by a weird bug whereby the event.target of a clicked link no longer points to the right bit of HTML due to the iframe having been shifted up by several pixels (by the transform translateY style having been added and shifting the iframe up). This of course should have nothing to do with the document inside the iframe, so it's definitely a framework bug.

I tried restoring the position of the iframe immediatley on click, but this is too late, and doesn't change what the event.target points to.

Now there are two mitigating factors:

  1. Real FFOS devices are touchscreen devices, and at least in the simulator, touch seems to provide the correct information, and is unaffected;
  2. It can be turned off.

So, I propose to add a check for FFOS, and NOT turn this feature on by default for that platform. When user turns it on anyway, a warning can be added if it's FFOS, to use at own risk.

@Jaifroid Jaifroid merged commit 897a1ff into main Oct 30, 2023
10 checks passed
@Jaifroid Jaifroid deleted the Slide-away-UI-elements-on-scroll branch October 30, 2023 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Slide away the search bar when scrolling down, slide it back in when scrolling up
2 participants