Navigation history dropdowns

[kommunarr]

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

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

[PikachuEXE]

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]

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:

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

[PikachuEXE]

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

[kommunarr]

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.

[PikachuEXE]

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

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]

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

[kommunarr]

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]

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]

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

[PikachuEXE]

Using theme pastel pink for testing:

What I see in this PR

development

[kommunarr]

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]

All issues resolved, pending idea from others especially on mobile

[kommunarr]

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]

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]

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

[github-actions]

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

[kommunarr]

Updated to use the NavigationHistory component. Almost done, just have to see what is blowing up in the yarn run pack:web. I have no earthly idea.

[kommunarr]

TIL checking !process.env.IS_ELECTRON can cause things to blow up. That was surprisingly annoying to debug!

[absidue]

@kommunarr Your problem wasn't !process.env.IS_ELECTRON but that you weren't wrapping the Electron only code in the if, webpack can only ignore imports if they are inside of the if-else. Early returns are too far away from the actual import for webpack to analyze them (otherwise it would have to meticulously analyze every single piece of code in the file which would take significantly longer and would have to be a lot more advanced to cover all possible edge cases).

[kommunarr]

@absidue Thanks for sharing that. While that explanation makes sense, I'm certainly not too proud to admit that I wouldn't have figured that explanation out on my own just by looking at the code.

[efb4f5ff-1298-471a-8973-3d47447115dc]

Got these errors on app launch

[kommunarr]

Oop, it looks like the optional chaining got removed before I committed it. Fixed and revalidated now

[PikachuEXE]

Same as before
pending idea from others especially on mobile