feat(tabs): implement robust tab button click handling
Fixes tab buttons becoming unresponsive after multiple clicks by: 1. Prevent GTK focus interference: - Set SetFocusOnClick(false) on tab buttons - Set SetCanFocus(false) to prevent focus cycling - Tab buttons no longer steal focus from WebViews 2. Use mapping pattern (from StackedPaneManager): - Map unique button ID → tab instead of capturing indices - Look up current index dynamically on click - Handles tab reordering and closure correctly 3. Proper cleanup: - Remove button mappings when tabs are closed - Prevents stale references 4. Use ConnectClicked signal: - More reliable than GestureClick controllers - Works consistently regardless of focus state Changes: - Add buttonToTab map and nextButtonID counter to TabManager - Disable focus-on-click for tab buttons - Implement handleTabButtonClick with dynamic index lookup - Clean up button mappings in closeTabInternal
- commit
- 8289f2ab26db74bdb5b9a2cb81335dc74f0c882f
- author
- bnema · · 2025-11-17 19:09 UTC
- committer
- bnema · · 2025-11-17 20:20 UTC
- parent
- 2835e26
- signature
- unsigned
2 files · +76 -28
M internal/app/browser/tab_bar.go +58 -27
| @@ -2,6 +2,7 @@ package browser | |||
| 2 | 2 | ||
| 3 | 3 | import ( | |
| 4 | 4 | "fmt" | |
| 5 | + | "sync/atomic" | |
| 5 | 6 | ||
| 6 | 7 | "github.com/bnema/dumber/internal/logging" | |
| 7 | 8 | "github.com/bnema/dumber/pkg/webkit" | |
| @@ -39,6 +40,11 @@ func (tm *TabManager) createTabButton(tab *Tab) gtk.Widgetter { | |||
| 39 | 40 | return nil | |
| 40 | 41 | } | |
| 41 | 42 | ||
| 43 | + | // CRITICAL: Disable focus-on-click to prevent GTK focus system from interfering | |
| 44 | + | // Tab buttons should be clickable without grabbing focus from the WebView | |
| 45 | + | button.SetFocusOnClick(false) | |
| 46 | + | button.SetCanFocus(false) | |
| 47 | + | ||
| 42 | 48 | // Apply CSS classes | |
| 43 | 49 | button.AddCSSClass("tab-button") | |
| 44 | 50 | ||
| @@ -63,8 +69,7 @@ func (tm *TabManager) createTabButton(tab *Tab) gtk.Widgetter { | |||
| 63 | 69 | // Store button reference for later updates | |
| 64 | 70 | tab.titleButton = button | |
| 65 | 71 | ||
| 66 | - | // Attach click handler | |
| 67 | - | tm.attachTabClickHandler(button, tab) | |
| 72 | + | // Note: Click handler will be attached in createTabInternal after tab is added to slice | |
| 68 | 73 | ||
| 69 | 74 | // Set active class if this is the active tab | |
| 70 | 75 | if tab.isActive { | |
| @@ -77,43 +82,69 @@ func (tm *TabManager) createTabButton(tab *Tab) gtk.Widgetter { | |||
| 77 | 82 | } | |
| 78 | 83 | ||
| 79 | 84 | // attachTabClickHandler attaches a click handler to switch to a tab when clicked. | |
| 85 | + | // Uses the same pattern as StackedPaneManager: maps unique button ID -> tab, | |
| 86 | + | // then looks up current index at click time to handle tab reordering correctly. | |
| 80 | 87 | func (tm *TabManager) attachTabClickHandler(button gtk.Widgetter, tab *Tab) { | |
| 81 | - | // Find tab index | |
| 82 | - | tabIndex := -1 | |
| 88 | + | if button == nil || tab == nil { | |
| 89 | + | logging.Error("[tabs] Cannot attach click handler: nil button or tab") | |
| 90 | + | return | |
| 91 | + | } | |
| 92 | + | ||
| 93 | + | gtkButton, ok := button.(*gtk.Button) | |
| 94 | + | if !ok { | |
| 95 | + | logging.Error(fmt.Sprintf("[tabs] Cannot attach click handler: widget is not *gtk.Button for tab %s", tab.id)) | |
| 96 | + | return | |
| 97 | + | } | |
| 98 | + | ||
| 99 | + | // Generate unique button ID and store mapping (pattern from stacked panes) | |
| 100 | + | buttonID := atomic.AddUint64(&tm.nextButtonID, 1) | |
| 101 | + | tm.buttonToTab[buttonID] = tab | |
| 102 | + | ||
| 103 | + | gtkButton.ConnectClicked(func() { | |
| 104 | + | tm.handleTabButtonClick(buttonID) | |
| 105 | + | }) | |
| 106 | + | ||
| 107 | + | logging.Debug(fmt.Sprintf("[tabs] Click handler attached to tab %s (buttonID=%d)", tab.id, buttonID)) | |
| 108 | + | } | |
| 109 | + | ||
| 110 | + | // handleTabButtonClick handles clicks on tab buttons to switch tabs. | |
| 111 | + | // Looks up the current index dynamically to handle tab reordering (pattern from stacked panes). | |
| 112 | + | func (tm *TabManager) handleTabButtonClick(buttonID uint64) { | |
| 113 | + | tm.mu.Lock() | |
| 114 | + | defer tm.mu.Unlock() | |
| 115 | + | ||
| 116 | + | // Look up tab from button ID | |
| 117 | + | tab, exists := tm.buttonToTab[buttonID] | |
| 118 | + | if !exists || tab == nil { | |
| 119 | + | logging.Warn(fmt.Sprintf("[tabs] Tab button click for unknown ID: %d", buttonID)) | |
| 120 | + | return | |
| 121 | + | } | |
| 122 | + | ||
| 123 | + | // Find current index of this tab | |
| 124 | + | targetIndex := -1 | |
| 83 | 125 | for i, t := range tm.tabs { | |
| 84 | 126 | if t == tab { | |
| 85 | - | tabIndex = i | |
| 127 | + | targetIndex = i | |
| 86 | 128 | break | |
| 87 | 129 | } | |
| 88 | 130 | } | |
| 89 | 131 | ||
| 90 | - | if tabIndex < 0 { | |
| 91 | - | logging.Error(fmt.Sprintf("[tabs] Cannot attach click handler: tab %s not found", tab.id)) | |
| 132 | + | if targetIndex < 0 { | |
| 133 | + | logging.Error(fmt.Sprintf("[tabs] Tab %s not found in tabs slice", tab.id)) | |
| 92 | 134 | return | |
| 93 | 135 | } | |
| 94 | 136 | ||
| 95 | - | // Create click gesture controller | |
| 96 | - | clickController := gtk.NewGestureClick() | |
| 97 | - | clickController.SetButton(1) // Left mouse button | |
| 98 | - | ||
| 99 | - | // Connect click signal | |
| 100 | - | clickController.ConnectPressed(func(_ int, _, _ float64) { | |
| 101 | - | logging.Debug(fmt.Sprintf("[tabs] Tab button clicked: %s (index %d)", tab.id, tabIndex)) | |
| 137 | + | // Only switch if not already active | |
| 138 | + | if targetIndex == tm.activeIndex { | |
| 139 | + | logging.Debug(fmt.Sprintf("[tabs] Tab %s already active (index %d)", tab.id, targetIndex)) | |
| 140 | + | return | |
| 141 | + | } | |
| 102 | 142 | ||
| 103 | - | // Switch to this tab | |
| 104 | - | webkit.RunOnMainThread(func() { | |
| 105 | - | if err := tm.SwitchToTab(tabIndex); err != nil { | |
| 106 | - | logging.Error(fmt.Sprintf("[tabs] Failed to switch to tab %d: %v", tabIndex, err)) | |
| 107 | - | } | |
| 108 | - | }) | |
| 109 | - | }) | |
| 143 | + | logging.Info(fmt.Sprintf("[tabs] Tab button clicked: switching from tab %d to %d (%s)", tm.activeIndex, targetIndex, tab.id)) | |
| 110 | 144 | ||
| 111 | - | // Add gesture controller to button | |
| 112 | - | if gtkWidget, ok := button.(interface{ AddController(gtk.EventControllerer) }); ok { | |
| 113 | - | gtkWidget.AddController(clickController) | |
| 114 | - | logging.Debug(fmt.Sprintf("[tabs] Click handler attached to tab %s", tab.id)) | |
| 115 | - | } else { | |
| 116 | - | logging.Error(fmt.Sprintf("[tabs] Failed to attach click handler to tab %s", tab.id)) | |
| 145 | + | // Switch to this tab | |
| 146 | + | if err := tm.switchToTabInternal(targetIndex); err != nil { | |
| 147 | + | logging.Error(fmt.Sprintf("[tabs] Failed to switch to tab %d: %v", targetIndex, err)) | |
| 117 | 148 | } | |
| 118 | 149 | } | |
| 119 | 150 | ||
| @@ -2,6 +2,7 @@ package browser | |||||
| 2 | 2 | ||||
| 3 | import ( | 3 | import ( | ||
| 4 | "fmt" | 4 | "fmt" | ||
| 5 | + | "sync/atomic" | |||
| 5 | 6 | ||||
| 6 | "github.com/bnema/dumber/internal/logging" | 7 | "github.com/bnema/dumber/internal/logging" | ||
| 7 | "github.com/bnema/dumber/pkg/webkit" | 8 | "github.com/bnema/dumber/pkg/webkit" | ||
| @@ -39,6 +40,11 @@ func (tm *TabManager) createTabButton(tab *Tab) gtk.Widgetter { | |||||
| 39 | return nil | 40 | return nil | ||
| 40 | } | 41 | } | ||
| 41 | 42 | ||||
| 43 | + | // CRITICAL: Disable focus-on-click to prevent GTK focus system from interfering | |||
| 44 | + | // Tab buttons should be clickable without grabbing focus from the WebView | |||
| 45 | + | button.SetFocusOnClick(false) | |||
| 46 | + | button.SetCanFocus(false) | |||
| 47 | + | ||||
| 42 | // Apply CSS classes | 48 | // Apply CSS classes | ||
| 43 | button.AddCSSClass("tab-button") | 49 | button.AddCSSClass("tab-button") | ||
| 44 | 50 | ||||
| @@ -63,8 +69,7 @@ func (tm *TabManager) createTabButton(tab *Tab) gtk.Widgetter { | |||||
| 63 | // Store button reference for later updates | 69 | // Store button reference for later updates | ||
| 64 | tab.titleButton = button | 70 | tab.titleButton = button | ||
| 65 | 71 | ||||
| 66 | - | // Attach click handler | 72 | + | // Note: Click handler will be attached in createTabInternal after tab is added to slice |
| 67 | - | tm.attachTabClickHandler(button, tab) | |||
| 68 | 73 | ||||
| 69 | // Set active class if this is the active tab | 74 | // Set active class if this is the active tab | ||
| 70 | if tab.isActive { | 75 | if tab.isActive { | ||
| @@ -77,43 +82,69 @@ func (tm *TabManager) createTabButton(tab *Tab) gtk.Widgetter { | |||||
| 77 | } | 82 | } | ||
| 78 | 83 | ||||
| 79 | // attachTabClickHandler attaches a click handler to switch to a tab when clicked. | 84 | // attachTabClickHandler attaches a click handler to switch to a tab when clicked. | ||
| 85 | + | // Uses the same pattern as StackedPaneManager: maps unique button ID -> tab, | |||
| 86 | + | // then looks up current index at click time to handle tab reordering correctly. | |||
| 80 | func (tm *TabManager) attachTabClickHandler(button gtk.Widgetter, tab *Tab) { | 87 | func (tm *TabManager) attachTabClickHandler(button gtk.Widgetter, tab *Tab) { | ||
| 81 | - | // Find tab index | 88 | + | if button == nil || tab == nil { |
| 82 | - | tabIndex := -1 | 89 | + | logging.Error("[tabs] Cannot attach click handler: nil button or tab") |
| 90 | + | return | |||
| 91 | + | } | |||
| 92 | + | ||||
| 93 | + | gtkButton, ok := button.(*gtk.Button) | |||
| 94 | + | if !ok { | |||
| 95 | + | logging.Error(fmt.Sprintf("[tabs] Cannot attach click handler: widget is not *gtk.Button for tab %s", tab.id)) | |||
| 96 | + | return | |||
| 97 | + | } | |||
| 98 | + | ||||
| 99 | + | // Generate unique button ID and store mapping (pattern from stacked panes) | |||
| 100 | + | buttonID := atomic.AddUint64(&tm.nextButtonID, 1) | |||
| 101 | + | tm.buttonToTab[buttonID] = tab | |||
| 102 | + | ||||
| 103 | + | gtkButton.ConnectClicked(func() { | |||
| 104 | + | tm.handleTabButtonClick(buttonID) | |||
| 105 | + | }) | |||
| 106 | + | ||||
| 107 | + | logging.Debug(fmt.Sprintf("[tabs] Click handler attached to tab %s (buttonID=%d)", tab.id, buttonID)) | |||
| 108 | + | } | |||
| 109 | + | ||||
| 110 | + | // handleTabButtonClick handles clicks on tab buttons to switch tabs. | |||
| 111 | + | // Looks up the current index dynamically to handle tab reordering (pattern from stacked panes). | |||
| 112 | + | func (tm *TabManager) handleTabButtonClick(buttonID uint64) { | |||
| 113 | + | tm.mu.Lock() | |||
| 114 | + | defer tm.mu.Unlock() | |||
| 115 | + | ||||
| 116 | + | // Look up tab from button ID | |||
| 117 | + | tab, exists := tm.buttonToTab[buttonID] | |||
| 118 | + | if !exists || tab == nil { | |||
| 119 | + | logging.Warn(fmt.Sprintf("[tabs] Tab button click for unknown ID: %d", buttonID)) | |||
| 120 | + | return | |||
| 121 | + | } | |||
| 122 | + | ||||
| 123 | + | // Find current index of this tab | |||
| 124 | + | targetIndex := -1 | |||
| 83 | for i, t := range tm.tabs { | 125 | for i, t := range tm.tabs { | ||
| 84 | if t == tab { | 126 | if t == tab { | ||
| 85 | - | tabIndex = i | 127 | + | targetIndex = i |
| 86 | break | 128 | break | ||
| 87 | } | 129 | } | ||
| 88 | } | 130 | } | ||
| 89 | 131 | ||||
| 90 | - | if tabIndex < 0 { | 132 | + | if targetIndex < 0 { |
| 91 | - | logging.Error(fmt.Sprintf("[tabs] Cannot attach click handler: tab %s not found", tab.id)) | 133 | + | logging.Error(fmt.Sprintf("[tabs] Tab %s not found in tabs slice", tab.id)) |
| 92 | return | 134 | return | ||
| 93 | } | 135 | } | ||
| 94 | 136 | ||||
| 95 | - | // Create click gesture controller | 137 | + | // Only switch if not already active |
| 96 | - | clickController := gtk.NewGestureClick() | 138 | + | if targetIndex == tm.activeIndex { |
| 97 | - | clickController.SetButton(1) // Left mouse button | 139 | + | logging.Debug(fmt.Sprintf("[tabs] Tab %s already active (index %d)", tab.id, targetIndex)) |
| 98 | - | 140 | + | return | |
| 99 | - | // Connect click signal | 141 | + | } |
| 100 | - | clickController.ConnectPressed(func(_ int, _, _ float64) { | |||
| 101 | - | logging.Debug(fmt.Sprintf("[tabs] Tab button clicked: %s (index %d)", tab.id, tabIndex)) | |||
| 102 | 142 | ||||
| 103 | - | // Switch to this tab | 143 | + | logging.Info(fmt.Sprintf("[tabs] Tab button clicked: switching from tab %d to %d (%s)", tm.activeIndex, targetIndex, tab.id)) |
| 104 | - | webkit.RunOnMainThread(func() { | |||
| 105 | - | if err := tm.SwitchToTab(tabIndex); err != nil { | |||
| 106 | - | logging.Error(fmt.Sprintf("[tabs] Failed to switch to tab %d: %v", tabIndex, err)) | |||
| 107 | - | } | |||
| 108 | - | }) | |||
| 109 | - | }) | |||
| 110 | 144 | ||||
| 111 | - | // Add gesture controller to button | 145 | + | // Switch to this tab |
| 112 | - | if gtkWidget, ok := button.(interface{ AddController(gtk.EventControllerer) }); ok { | 146 | + | if err := tm.switchToTabInternal(targetIndex); err != nil { |
| 113 | - | gtkWidget.AddController(clickController) | 147 | + | logging.Error(fmt.Sprintf("[tabs] Failed to switch to tab %d: %v", targetIndex, err)) |
| 114 | - | logging.Debug(fmt.Sprintf("[tabs] Click handler attached to tab %s", tab.id)) | |||
| 115 | - | } else { | |||
| 116 | - | logging.Error(fmt.Sprintf("[tabs] Failed to attach click handler to tab %s", tab.id)) | |||
| 117 | } | 148 | } | ||
| 118 | } | 149 | } | ||
| 119 | 150 | ||||
M internal/app/browser/tab_manager.go +18 -1
| @@ -29,6 +29,10 @@ type TabManager struct { | |||
| 29 | 29 | tabModeActive bool | |
| 30 | 30 | tabModeTimer *time.Timer | |
| 31 | 31 | ||
| 32 | + | // Tab button click handling (pattern from StackedPaneManager) | |
| 33 | + | buttonToTab map[uint64]*Tab // Maps button ID to tab for click handling | |
| 34 | + | nextButtonID uint64 // Atomic counter for generating unique button IDs | |
| 35 | + | ||
| 32 | 36 | // Synchronization | |
| 33 | 37 | mu sync.RWMutex | |
| 34 | 38 | } | |
| @@ -50,6 +54,7 @@ func NewTabManager(app *BrowserApp, window *webkit.Window) *TabManager { | |||
| 50 | 54 | activeIndex: -1, | |
| 51 | 55 | window: window, | |
| 52 | 56 | app: app, | |
| 57 | + | buttonToTab: make(map[uint64]*Tab), | |
| 53 | 58 | } | |
| 54 | 59 | ||
| 55 | 60 | logging.Info("[tabs] Tab manager initialized") | |
| @@ -210,9 +215,12 @@ func (tm *TabManager) createTabInternal(url string) error { | |||
| 210 | 215 | ||
| 211 | 216 | // Add to tabs slice | |
| 212 | 217 | tm.tabs = append(tm.tabs, tab) | |
| 213 | - | newIndex := len(tm.tabs) - 1 | |
| 218 | + | ||
| 219 | + | // Attach click handler with unique ID (pattern from stacked panes) | |
| 220 | + | tm.attachTabClickHandler(tab.titleButton, tab) | |
| 214 | 221 | ||
| 215 | 222 | // Switch to new tab | |
| 223 | + | newIndex := len(tm.tabs) - 1 | |
| 216 | 224 | if err := tm.switchToTabInternal(newIndex); err != nil { | |
| 217 | 225 | return fmt.Errorf("failed to switch to new tab: %w", err) | |
| 218 | 226 | } | |
| @@ -341,6 +349,15 @@ func (tm *TabManager) closeTabInternal(index int) error { | |||
| 341 | 349 | ||
| 342 | 350 | // Cleanup workspace (WorkspaceManager cleanup is handled automatically when tabs are removed) | |
| 343 | 351 | ||
| 352 | + | // Clean up button mapping (find and remove this tab's button ID) | |
| 353 | + | for buttonID, mappedTab := range tm.buttonToTab { | |
| 354 | + | if mappedTab == tab { | |
| 355 | + | delete(tm.buttonToTab, buttonID) | |
| 356 | + | logging.Debug(fmt.Sprintf("[tabs] Cleaned up button mapping for closed tab %s (buttonID=%d)", tab.id, buttonID)) | |
| 357 | + | break | |
| 358 | + | } | |
| 359 | + | } | |
| 360 | + | ||
| 344 | 361 | // Remove tab button from tab bar | |
| 345 | 362 | tm.removeTabFromBar(tab) | |
| 346 | 363 | ||
| @@ -29,6 +29,10 @@ type TabManager struct { | |||||
| 29 | tabModeActive bool | 29 | tabModeActive bool | ||
| 30 | tabModeTimer *time.Timer | 30 | tabModeTimer *time.Timer | ||
| 31 | 31 | ||||
| 32 | + | // Tab button click handling (pattern from StackedPaneManager) | |||
| 33 | + | buttonToTab map[uint64]*Tab // Maps button ID to tab for click handling | |||
| 34 | + | nextButtonID uint64 // Atomic counter for generating unique button IDs | |||
| 35 | + | ||||
| 32 | // Synchronization | 36 | // Synchronization | ||
| 33 | mu sync.RWMutex | 37 | mu sync.RWMutex | ||
| 34 | } | 38 | } | ||
| @@ -50,6 +54,7 @@ func NewTabManager(app *BrowserApp, window *webkit.Window) *TabManager { | |||||
| 50 | activeIndex: -1, | 54 | activeIndex: -1, | ||
| 51 | window: window, | 55 | window: window, | ||
| 52 | app: app, | 56 | app: app, | ||
| 57 | + | buttonToTab: make(map[uint64]*Tab), | |||
| 53 | } | 58 | } | ||
| 54 | 59 | ||||
| 55 | logging.Info("[tabs] Tab manager initialized") | 60 | logging.Info("[tabs] Tab manager initialized") | ||
| @@ -210,9 +215,12 @@ func (tm *TabManager) createTabInternal(url string) error { | |||||
| 210 | 215 | ||||
| 211 | // Add to tabs slice | 216 | // Add to tabs slice | ||
| 212 | tm.tabs = append(tm.tabs, tab) | 217 | tm.tabs = append(tm.tabs, tab) | ||
| 213 | - | newIndex := len(tm.tabs) - 1 | 218 | + | |
| 219 | + | // Attach click handler with unique ID (pattern from stacked panes) | |||
| 220 | + | tm.attachTabClickHandler(tab.titleButton, tab) | |||
| 214 | 221 | ||||
| 215 | // Switch to new tab | 222 | // Switch to new tab | ||
| 223 | + | newIndex := len(tm.tabs) - 1 | |||
| 216 | if err := tm.switchToTabInternal(newIndex); err != nil { | 224 | if err := tm.switchToTabInternal(newIndex); err != nil { | ||
| 217 | return fmt.Errorf("failed to switch to new tab: %w", err) | 225 | return fmt.Errorf("failed to switch to new tab: %w", err) | ||
| 218 | } | 226 | } | ||
| @@ -341,6 +349,15 @@ func (tm *TabManager) closeTabInternal(index int) error { | |||||
| 341 | 349 | ||||
| 342 | // Cleanup workspace (WorkspaceManager cleanup is handled automatically when tabs are removed) | 350 | // Cleanup workspace (WorkspaceManager cleanup is handled automatically when tabs are removed) | ||
| 343 | 351 | ||||
| 352 | + | // Clean up button mapping (find and remove this tab's button ID) | |||
| 353 | + | for buttonID, mappedTab := range tm.buttonToTab { | |||
| 354 | + | if mappedTab == tab { | |||
| 355 | + | delete(tm.buttonToTab, buttonID) | |||
| 356 | + | logging.Debug(fmt.Sprintf("[tabs] Cleaned up button mapping for closed tab %s (buttonID=%d)", tab.id, buttonID)) | |||
| 357 | + | break | |||
| 358 | + | } | |||
| 359 | + | } | |||
| 360 | + | ||||
| 344 | // Remove tab button from tab bar | 361 | // Remove tab button from tab bar | ||
| 345 | tm.removeTabFromBar(tab) | 362 | tm.removeTabFromBar(tab) | ||
| 346 | 363 | ||||