Dismiss Pinned Tab Previews when exiting full screen or hovering the semaphore buttons #2788
Conversation
…en hovering on the semaphore buttons and exiting fullscreen
github-actions
bot
added
the
bot: not in app board
alessandroboron
removed
the
bot: not in app board
There was a problem hiding this comment.
Unfortunately this didn‘t fix the whole issue:
- Open window in full screen
- Show a preview on a tab
- Hit 🌐+F to exit full screen: the preview stays
I think it should be more of a Tracking Area related fix that also takes major screen updates (like full screen switch or something else maybe).
Do you think there may be a better solution than this (pretty hacky and fragile) patch?
alessandroboron
force-pushed
the
alessandro/tab-preview-not-dismissed
branch
4 times, most recently
from
fe7453a to
b280ef9
Compare
@mallexxx Thanks for the comment. It took a bit of time, but I’ve refactored a bit. I’ve updated the description with what I’ve done. I converted this into a draft PR before I implemented tests. Could you please test the scenarios again? I’ve also CCd @tomasstrba as he worked on tab preview. |
| // More Info: | ||
| // - RXMarbles: https://rxmarbles.com/#withLatestFrom | ||
| // - https://jasdev.me/notes/with-latest-from | ||
| extension Publisher { |
There was a problem hiding this comment.
We could also import CombineExt package which has same implementation by jasdev but if we’re not using the whole set of operators, probably no need it.
| @@ -65,6 +65,8 @@ final class PinnedTabsViewModel: ObservableObject { | |||
| } | |||
| } | |||
|
|
|||
| @Published var mouseMoving: Void = () | |||
There was a problem hiding this comment.
In the beginning I was passing setting the CGPoint where the mouse location was, but I wasn’t really using it so I changed to Void. If it feels weird we can always change to CGPoint as we may need it in the future we need it
| collectionModel?.hoveredItem = isHovered ? model : nil | ||
| } | ||
| stack | ||
| .onHover { [weak collectionModel, weak model] isHovered in |
There was a problem hiding this comment.
An approach I tried was to make an operator called onContinuousHovering. The problem with that was that the preview would show again after exiting full screen. And removing mouseEntered(_:) from the Tracking Area view would cause issues to the overlay state of the view when exiting full screen. 😞
| DispatchQueue.main.asyncAfter(deadline: .now() + 0.2) { | ||
| if self.view.isMouseLocationInsideBounds() == false { | ||
| self.hideTabPreview(allowQuickRedisplay: true) | ||
| private func subscribeToTabPreviewEvents() { |
There was a problem hiding this comment.
This method receive events to show both pinned tabs and non pinned tabs.
| tabPreviewEventsHandler.unpinnedTabMouseExited() | ||
| } | ||
|
|
||
| func tabBarViewItemMouseIsMoving(_ tabBarViewItem: TabBarViewItem) { |
There was a problem hiding this comment.
When the mouse moves within the tab we show the preview
| @@ -1020,15 +1029,17 @@ extension TabBarViewController: NSCollectionViewDelegate { | |||
| extension TabBarViewController: TabBarViewItemDelegate { | |||
|
|
|||
| func tabBarViewItem(_ tabBarViewItem: TabBarViewItem, isMouseOver: Bool) { | |||
| guard !isMouseOver else { return } | |||
There was a problem hiding this comment.
When the mouse exit the area we dismiss the preview
| import Foundation | ||
| import Combine | ||
|
|
||
| final class TabPreviewEventsHandler { |
There was a problem hiding this comment.
The idea of this class is to have the logic that leads to a TabPreviewEvent in one place and to avoid having subjects in the VC for the non pinned tabs.
It should technically be easy to test this as something like “when an event on a publisher happens, then this class should send a TabPreviewEvent in a specific state".
alessandroboron
force-pushed
the
alessandro/tab-preview-not-dismissed
branch
from
b280ef9 to
b071dd2
Compare
Task/Issue URL: https://app.asana.com/0/1177771139624306/1205566350070653/f
Cc: @tomasstrba
Description:
Fix an issue that causes the pinned tab previews to stay on screen when hovering on the semaphore buttons and exiting fullscreen.
Video
https://github.com/duckduckgo/macos-browser/assets/1089358/99f7558d-2324-410d-bcbf-85fdbb9a7c35
Steps to test this PR:
Scenario 1 - Full Screen
Expected Result: The preview should disappear.
Scenario 2 - Standard Screen
Expected Result: The preview should disappear.
** UPDATE **
I converted this PR into a draft PR to get feedback before I implement tests.
I refactored the logic to present the Tab previews due to entering/exiting full screen.
When we exit full screen the mouse jumps on the tab due to the semaphore buttons appearing on the left-hand side of the tabs. This causes the preview to appear again. And if we prevent the preview from appearing by listening to the exit full screen notification, the preview won’t reappear when the user moves the mouse because mouseEntered/onHover is already triggered.
Instead of presenting the preview when the mouse enters the view, we present it when the mouse moves within the view. In this way, when the mouse jumps due to exiting full screen, the preview is not presented automatically, but it will when the user moves the mouse (the same behaviour as Safari).
I created a TabPreviewEventsHandler. Pinned tabs used a reactive stream to show/dismiss the preview, while non-pinned tabs used imperative code. I actually merged the events of pinned and non-pinned tabs into a publisher so there’s a single source.
** NEW VIDEO **
Screen.Recording.2024-05-23.at.8.12.10.PM.mov
—
Internal references:
Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation