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

Fix mouse backward and forward buttons not updating the in-app buttons #5242

Merged
merged 1 commit into from
Jun 11, 2024

Conversation

absidue
Copy link
Member

@absidue absidue commented Jun 9, 2024

Fix mouse backward and forward buttons not updating the in-app buttons

Pull Request Type

  • Bugfix

Related issue

closes #4408

Description

Inspired by a comment on the navigation history dropdown pull request about fixing the mouse button issue by switching to an Electron-only API, I decided to see if there was an alternative way to solve it that without making existing functionality Electron only. I found the Navigation API which is supported in Electron, Android WebView as well as Chromium browsers (both desktop and mobile) https://developer.mozilla.org/en-US/docs/Web/API/Navigation_API#browser_compatibility. While it isn't supported in Firefox and Safari, that will only affect the web build in the unofficial FreeTubeAndroid fork and I think there is a good argument to be made that the back and forward buttons aren't needed there anyway because you can just use the ones built-into the web browser, so in those two browsers, the back and forward buttons will always be enabled, so they are still usable, they just won't get disabled when you can't navigate in a certain direction.

With that out of the way, here is some information to the pull request itself. As the Navigation API provides canGoForward and canGoBack properties, we no longer need our logic that relies on trying to track the navigation index ourselves. It also means we no longer need to send an IPC message from the main process into the window when the user triggers the navigation through the app menu or the keyboard shortcuts and can just use the convenient .goBack() and .goForward() methods on the webContents instance, as the Navigation API keeps track of whether you can go forwards or back for us.

Testing

If you have a mouse that has extra buttons to let you navigate back and forwards, test that those work and that the back and forward buttons in the app update their enabled and disabled state correctly.
I know it doesn't count for much as I opened the pull request, but my mouse has those buttons and for me it worked correctly.

Regression tests:

  1. Test that pressing the back and forwards buttons still work and that they update their disabled state correctly.
  2. Test that the Alt+Left arrow and Alt+Right arrow keyboard shortcuts update the back and forward button disabled state correctly.
  3. Test that the Back and Forward entries in the View app menu still work and update the back and forward arrow button disabled state correctly.

Desktop

  • OS: Windows
  • OS Version: 10
  • FreeTube version: bf3304d

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) June 9, 2024 20:13
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Jun 9, 2024
@absidue absidue mentioned this pull request Jun 9, 2024
1 task
Copy link
Collaborator

@kommunarr kommunarr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't even know the Navigation API was available yet! Somehow I just thought the Electron access methods were something internal. Feels good to get rid of our side-logic trying to emulate it lol

@PikachuEXE
Copy link
Collaborator

Test that the Back and Forward entries in the View app menu still work and update the back and forward button disabled state correctly.

I am not sure if the disabled state exists on MacOS... (Clicking on it does nothing)
image

@absidue
Copy link
Member Author

absidue commented Jun 11, 2024

It's the arrows that get enabled and disabled, not the menu entries.

The reason we can't disable them in the menus is because we use the app-wide menu, so there is one menu for all windows. While yes it is possible to give each window it's own menu on Linux and Windows, macOS doesn't allow you to do that. So if we wanted to enable and disable the menu entries, we would have to do some complicated logic to keep track of which window is active and when you navigate in it and overwrite the app-wide menu every time you navigate or change windows. So that is definitely out of scope of this pull request.

Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya no point handling multi-window in app menu item (in this PR at least)
The arrow buttons are disabled correctly

@FreeTubeBot FreeTubeBot merged commit b9146b1 into FreeTubeApp:development Jun 11, 2024
5 checks passed
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Jun 11, 2024
@absidue absidue deleted the fix-navigation branch June 11, 2024 05:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Page navigation arrow indicators not updated by mouse buttons
5 participants