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

Navigation history dropdowns #5227

Merged
merged 28 commits into from
Dec 4, 2024

Conversation

kommunarr
Copy link
Collaborator

@kommunarr kommunarr commented Jun 4, 2024

Navigation history dropdowns

Pull Request Type

  • Feature Implementation

Related issue

closes #2740

Description

  • On long press (0.5s) or right-click of a non-disabled navigation arrow, load <=15 surrounding history entries. The history entry dropdown presentation behaviors are taken from FF:
    • Show 15 history entries at most. If at the 7th entry from start/last, show results from the start/last. Otherwise, show 7 earlier entries & 7 later entries.
    • The current entry is included in both menus. It's marked with a different color, checkmark, and higher boldness.
  • @contextmenu.prevent='' is set on the navigation arrows to prevent the context menu appearing on right-click in dev mode.
  • The titles are updated to reflect new behavior (language with inspiration from Chrome [Click to go forward, hold to see history] & FF). These titles are also now visible on hover even when the icons are disabled.
  • Implementation detail: The top-nav-is-colored mixin in top-nav.scss unfortunately is not compatible with the use of :deep, a selector which is required for ft-icon-buttons to actually work styling-wise in the top nav with the legacy non-icon-button-but-almost navIcon font-awesome-icon pattern we're using there (theme doesn't go far enough). So that's why we're doing .topNavBarColor ${selector} for the new SCSS code instead of using the mixin (which thankfully is very fine given that it's a very simple mixin).

Minor cleanup:

  • We now have an appTitle datum in the store for when you need to set or check document.title. This is needed because we update it asynchronously in a few different places and need to display the current title properly when the navigation dropdown is shown.
  • d18b549: Incidentally as a way of cleaning up an extra event handler to worry about in ft-icon-button, replaces the mouseDownOnIcon focusout logic in ft-icon-button in favor of more straightforward logic (adopted from List sorting & display settings #4231)

Work for a future PR

  • "Pull down" to open + pointerup after on the corresponding history entry to navigate to it.
  • Right-click + pointerup after on the corresponding history entry to navigate to it (Note: seems to be FF-specific; this behavior is not in Chrome)

Screenshots

Screenshot_20240603_222630
Screenshot_20240603_222749

Testing

  • Check that the result ordering behavior works correctly:

If at the 7th entry from start/last, show results from the start/last. Otherwise, show 7 earlier entries & 7 later entries.

  • Check that navigating to any arbitrary history entry works
  • Check that both right-click and long-press work to trigger the navigation history dropdown
  • Check long-press on mobile

I wouldn't recommend testing these ones for the sake of your time, but just to demonstrate that I have tested these:

  • (REG, optional) w.r.t. d18b549, check that focusout / Esc / keyboard navigation with ft-icon-button dropdowns still work as intended
  • (REG, optional) Check no weird behavior with custom landing page selection
  • (REG, optional) Check navigation arrow buttons appear the same with Match Top Bar with Main Color setting active
  • (REG, optional) Check that navigation history is still not shared between separate windows
  • (optional) Check that navigation history entries are re-translated after changing the language

Desktop

  • OS: OpenSUSE
  • OS Version: TW

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) June 4, 2024 01:38
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Jun 4, 2024
@PikachuEXE
Copy link
Collaborator

It doesn't feel like web browser UX:

  • Showing all entries when long press on either back / forward button, expect to see only previous / next entries
  • Expect not to see entry for current page

@kommunarr
Copy link
Collaborator Author

kommunarr commented Jun 4, 2024

It doesn't feel like web browser UX:

  • Showing all entries when long press on either back / forward button, expect to see only previous / next entries
  • Expect not to see entry for current page

I'm not sure what browser you use, but the implementation is the same behavior as Firefox:

Screenshot_20240603_213730

I wouldn't say it's the only way to do it, but it's a major browser

@PikachuEXE
Copy link
Collaborator

Even that's the case Firefox does show which one is the current entry which I don't see in this PR
image
image

@kommunarr
Copy link
Collaborator Author

Even that's the case

'Tis!

Firefox does show which one is the current entry which I don't see in this PR

Two visual indicators here, my friend! I can add a checkmark on top of that if desired, but please look at the PR & description in closer detail before formulating your criticisms.

Screenshot_20240603_215215

@PikachuEXE
Copy link
Collaborator

I am sorry that I didn't provide screenshot when providing my opinion
image

A more accurate description would be the
Firefox does show which one is the current entry in darker themes which I don't see in this PR

@PikachuEXE
Copy link
Collaborator

Additionally the colors for back / forward button are now inconsistent with other top-nav buttons in themes like light, pastel pink
image

@kommunarr
Copy link
Collaborator Author

kommunarr commented Jun 4, 2024

Firefox does show which one is the current entry in darker themes which I don't see in this PR

This is more of a pre-existing problem with certain themes like Dark not having a visually distinctive --side-nav-hover-color, which results in the top & side navs conveying less information, but I will add a checkmark to make it clearer.

Additionally the colors for back / forward button are now inconsistent with other top-nav buttons in themes like light, pastel pink

I think you're on an older version. I fixed this in an earlier commit. That also added the font-weight change.

@PikachuEXE
Copy link
Collaborator

With checkmark it seems better
However I re-tested after checking out Add checkmark icon to active history entry and the inconsistent button color is still an issue

@kommunarr
Copy link
Collaborator Author

You might need to rebase then, as I cannot replicate:
Screenshot_20240603_221630

@PikachuEXE
Copy link
Collaborator

Using theme pastel pink for testing:

What I see in this PR
image
image
image

development
image
image

@kommunarr
Copy link
Collaborator Author

Ah, it's not happening to me with hot pink, but I can see it with red as main color. Looking into it now, thank you

@PikachuEXE
Copy link
Collaborator

All issues resolved, pending idea from others especially on mobile

@kommunarr
Copy link
Collaborator Author

I had no idea there was a NavigationHistory class in Electron until @absidue pointed it out in the Matrix chat, so it depends if we want to put more eggs into this PR or save that refactor for a separate PR that adds title support, as this is currently functional as-is. That's more of an organizing question than a substantive one, so I'm fine with whichever choice. I will say the suggested change to using a context menu is probably a lot more lateral, so I would prefer if that one was kept to a follow-up PR.

With all that said, I would advise that any reviewers prioritize the higher-salience PRs first if possible. We're a lot closer than we think, and we're reaching a critical mass of feature requests for features we've already merged.

@kommunarr
Copy link
Collaborator Author

Here's where I'm at after looking into it some more.

I can see what was being gotten at with using a context menu as an easy way to grab the current browserWindow.webContents.navigationHistory, but I don't like that we have less control over the styling of the popup. We can discuss that aspect some more, but I'm not sure if that's something we would want, so I'm going to leave that as a separate future effort.

The NavigationHistory API as we're using it is very new to Electron, so I would imagine it's subject to change some. My current idea for how we would integrate with it today:

  • We use an ipcMain.handle call on each route navigation to get the navigationHistory.getActiveIndex() and navigationHistory.length() values, and use those to track the navigation arrow buttons' forward/backward state in lieu of our navigateHistory() function call and isForwardOrBackward logic, thus fixing [Bug]: Page navigation arrow indicators not updated by mouse buttons #4408.
  • When long-clicking or right-clicking on a navigation arrow, we can replace our sessionNavigationHistory logic with an ipcMain.handle call that gets the getEntryAtIndex(index) for the up to 15 entries that we want, and then we can get the corresponding title property for those. When one is clicked, I believe we should still be able to use window.history.go() to navigate to them without issue.

Note that the above implementation would be easier & improved if these properties were to be exposed to developers:

In a separate PR, duplicate and move other navigationHistory related endpoints like CanGoBack, GoBack, CanGoForward, GoForward, CanGoToOffset, GoToOffset, CanGoToIndex, GoToIndex to the new namespace after the first PR merges.

Because of that, it may be reasonable to wait on the NavigationHistory side of things until it's fleshed out some more with those utilities that would simplify the implementation.

@ArthurKun21
Copy link

Would it be possible to have an alternative way to open it like a very small arrow down button

Sorry for the bad drawing, just imagine it was small

image

Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@efb4f5ff-1298-471a-8973-3d47447115dc

Not sure I understand the second concern.

When going to a community posts its says FreeTube

It also says FreeTube when doing a search now

@kommunarr
Copy link
Collaborator Author

TIL we had navigable posts! Fixed both FreeTube issues, incl. on initial search navigation

Screenshot_20241123_151010

@ChunkyProgrammer ChunkyProgrammer added the PR: waiting for review For PRs that are complete, tested, and ready for review label Nov 27, 2024
@github-actions github-actions bot added PR: merge conflicts / rebase needed and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Dec 2, 2024
Copy link
Contributor

github-actions bot commented Dec 2, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link
Contributor

github-actions bot commented Dec 3, 2024

Conflicts have been resolved. A maintainer will review the pull request shortly.

@kommunarr kommunarr added the PR: waiting for review For PRs that are complete, tested, and ready for review label Dec 3, 2024
@github-actions github-actions bot added PR: merge conflicts / rebase needed and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Dec 3, 2024
Copy link
Contributor

github-actions bot commented Dec 3, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link
Contributor

github-actions bot commented Dec 3, 2024

Conflicts have been resolved. A maintainer will review the pull request shortly.

@efb4f5ff-1298-471a-8973-3d47447115dc

Hmm the change in #5227 (comment) made me think about cases like the ones listed below.

  • Both querys say meow but are different pages.
VirtualBoxVM_xWI1v3DeS9.mp4
  • I searched for something that has the same name as on of our pages. Maybe best to bring back the Search Results as a differentiator?
VirtualBoxVM_vcutRF2YBV.mp4

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added the PR: waiting for review For PRs that are complete, tested, and ready for review label Dec 3, 2024
@kommunarr
Copy link
Collaborator Author

The same type of issue exists with many of the pages. My plan is merge this, then #5003, then open a separate PR for adding the route icons added in the second to the nav history dropdown entries. Don't want to make these two larger than they currently are, though.

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.

Icons are definitely needed...

@FreeTubeBot FreeTubeBot merged commit 0eb8271 into FreeTubeApp:development Dec 4, 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 Dec 4, 2024
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.

[Feature Request]: Right click on navigation buttons show history
7 participants