Harden browsing context popup cleanup and defaults
Reason: GitHub review comments found a few real follow-ups after the main popup refactor: duplicated browsing-context defaults, stale popup-hosting state on reused WebViews, and small correctness gaps around popup observation and routing metadata. Scope: - deduplicate default browsing-context config initialization and refresh the port seam comment - clear WebKit browsing-context host state during destroy/pool reuse and cover it with regression tests - tighten native OAuth observer ordering, normalize `_blank` inference, and propagate middle-click host decisions - replace popup window content instead of appending across repeated SetContent calls Description: This commit applies the valid Copilot and CodeRabbit review findings for the popup browsing-context branch. It removes default drift risk, prevents reused WebViews from inheriting stale native-popup decisions, avoids leaking tracked OAuth popup state when callback capability is absent, makes `_blank` inference whitespace/case tolerant, preserves host-decision metadata on middle-click panes, and keeps native popup shells from accumulating children on repeated content replacement.
- commit
- 8354b8a4969bb074ba3ef2162df21879f1992a6a
- author
- brice · · 2026-05-13 05:34 UTC
- committer
- brice · · 2026-05-13 05:34 UTC
- parent
- 32ee89e
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
11 files · +153 -33
M internal/application/port/browsing_context.go +2 -2
| @@ -1,6 +1,6 @@ | |||
| 1 | 1 | // Package port defines interfaces (ports) for use cases to depend on. | |
| 2 | - | // These types are the seam for the approved browsing-context architecture. | |
| 3 | - | // They are standalone for now; no callbacks are wired yet. | |
| 2 | + | // These types are the active seam for the approved browsing-context architecture | |
| 3 | + | // shared across policy, UI, and engine integrations. | |
| 4 | 4 | ||
| 5 | 5 | package port | |
| 6 | 6 | ||
| @@ -1,6 +1,6 @@ | |||||
| 1 | // Package port defines interfaces (ports) for use cases to depend on. | 1 | // Package port defines interfaces (ports) for use cases to depend on. | ||
| 2 | - | // These types are the seam for the approved browsing-context architecture. | 2 | + | // These types are the active seam for the approved browsing-context architecture |
| 3 | - | // They are standalone for now; no callbacks are wired yet. | 3 | + | // shared across policy, UI, and engine integrations. |
| 4 | 4 | ||||
| 5 | package port | 5 | package port | ||
| 6 | 6 | ||||
M internal/infrastructure/config/defaults.go +16 -18
| @@ -76,6 +76,18 @@ const ( | |||
| 76 | 76 | defaultSkiaGPUPaintingThreads = -1 // -1 means unset; 0 would disable GPU tile painting | |
| 77 | 77 | ) | |
| 78 | 78 | ||
| 79 | + | func defaultBrowsingContextConfig() BrowsingContextConfig { | |
| 80 | + | return BrowsingContextConfig{ | |
| 81 | + | Behavior: PopupBehaviorSplit, | |
| 82 | + | Placement: defaultPopupPlacement, | |
| 83 | + | OpenInNewPane: true, | |
| 84 | + | FollowPaneContext: true, | |
| 85 | + | BlankTargetBehavior: "stacked", | |
| 86 | + | EnableSmartDetection: true, | |
| 87 | + | OAuthAutoClose: true, | |
| 88 | + | } | |
| 89 | + | } | |
| 90 | + | ||
| 79 | 91 | // getDefaultLogDir returns the default log directory, falls back to empty string on error | |
| 80 | 92 | func getDefaultLogDir() string { | |
| 81 | 93 | logDir, err := GetLogDir() | |
| @@ -87,6 +99,8 @@ func getDefaultLogDir() string { | |||
| 87 | 99 | ||
| 88 | 100 | // DefaultConfig returns the default configuration values for dumber. | |
| 89 | 101 | func DefaultConfig() *Config { | |
| 102 | + | browsingContextDefaults := defaultBrowsingContextConfig() | |
| 103 | + | ||
| 90 | 104 | return &Config{ | |
| 91 | 105 | Database: DatabaseConfig{ | |
| 92 | 106 | // Path is set dynamically in config.Load() | |
| @@ -254,24 +268,8 @@ func DefaultConfig() *Config { | |||
| 254 | 268 | }, | |
| 255 | 269 | TabBarPosition: defaultTabBarPosition, | |
| 256 | 270 | HideTabBarWhenSingleTab: true, | |
| 257 | - | BrowsingContexts: BrowsingContextConfig{ | |
| 258 | - | Behavior: PopupBehaviorSplit, // Default: open JavaScript popups in split panes | |
| 259 | - | Placement: defaultPopupPlacement, | |
| 260 | - | OpenInNewPane: true, | |
| 261 | - | FollowPaneContext: true, | |
| 262 | - | BlankTargetBehavior: "stacked", // Default: open _blank links in stacked mode | |
| 263 | - | EnableSmartDetection: true, // Use WindowProperties to detect popup vs tab | |
| 264 | - | OAuthAutoClose: true, // Auto-close OAuth popups on success | |
| 265 | - | }, | |
| 266 | - | Popups: PopupBehaviorConfig{ | |
| 267 | - | Behavior: PopupBehaviorSplit, // Default: open JavaScript popups in split panes | |
| 268 | - | Placement: defaultPopupPlacement, | |
| 269 | - | OpenInNewPane: true, | |
| 270 | - | FollowPaneContext: true, | |
| 271 | - | BlankTargetBehavior: "stacked", // Default: open _blank links in stacked mode | |
| 272 | - | EnableSmartDetection: true, // Use WindowProperties to detect popup vs tab | |
| 273 | - | OAuthAutoClose: true, // Auto-close OAuth popups on success | |
| 274 | - | }, | |
| 271 | + | BrowsingContexts: browsingContextDefaults, | |
| 272 | + | Popups: browsingContextDefaults, | |
| 275 | 273 | Styling: WorkspaceStylingConfig{ | |
| 276 | 274 | BorderWidth: defaultBorderWidth, | |
| 277 | 275 | BorderColor: defaultBorderColor, | |
| @@ -76,6 +76,18 @@ const ( | |||||
| 76 | defaultSkiaGPUPaintingThreads = -1 // -1 means unset; 0 would disable GPU tile painting | 76 | defaultSkiaGPUPaintingThreads = -1 // -1 means unset; 0 would disable GPU tile painting | ||
| 77 | ) | 77 | ) | ||
| 78 | 78 | ||||
| 79 | + | func defaultBrowsingContextConfig() BrowsingContextConfig { | |||
| 80 | + | return BrowsingContextConfig{ | |||
| 81 | + | Behavior: PopupBehaviorSplit, | |||
| 82 | + | Placement: defaultPopupPlacement, | |||
| 83 | + | OpenInNewPane: true, | |||
| 84 | + | FollowPaneContext: true, | |||
| 85 | + | BlankTargetBehavior: "stacked", | |||
| 86 | + | EnableSmartDetection: true, | |||
| 87 | + | OAuthAutoClose: true, | |||
| 88 | + | } | |||
| 89 | + | } | |||
| 90 | + | ||||
| 79 | // getDefaultLogDir returns the default log directory, falls back to empty string on error | 91 | // getDefaultLogDir returns the default log directory, falls back to empty string on error | ||
| 80 | func getDefaultLogDir() string { | 92 | func getDefaultLogDir() string { | ||
| 81 | logDir, err := GetLogDir() | 93 | logDir, err := GetLogDir() | ||
| @@ -87,6 +99,8 @@ func getDefaultLogDir() string { | |||||
| 87 | 99 | ||||
| 88 | // DefaultConfig returns the default configuration values for dumber. | 100 | // DefaultConfig returns the default configuration values for dumber. | ||
| 89 | func DefaultConfig() *Config { | 101 | func DefaultConfig() *Config { | ||
| 102 | + | browsingContextDefaults := defaultBrowsingContextConfig() | |||
| 103 | + | ||||
| 90 | return &Config{ | 104 | return &Config{ | ||
| 91 | Database: DatabaseConfig{ | 105 | Database: DatabaseConfig{ | ||
| 92 | // Path is set dynamically in config.Load() | 106 | // Path is set dynamically in config.Load() | ||
| @@ -254,24 +268,8 @@ func DefaultConfig() *Config { | |||||
| 254 | }, | 268 | }, | ||
| 255 | TabBarPosition: defaultTabBarPosition, | 269 | TabBarPosition: defaultTabBarPosition, | ||
| 256 | HideTabBarWhenSingleTab: true, | 270 | HideTabBarWhenSingleTab: true, | ||
| 257 | - | BrowsingContexts: BrowsingContextConfig{ | 271 | + | BrowsingContexts: browsingContextDefaults, |
| 258 | - | Behavior: PopupBehaviorSplit, // Default: open JavaScript popups in split panes | 272 | + | Popups: browsingContextDefaults, |
| 259 | - | Placement: defaultPopupPlacement, | |||
| 260 | - | OpenInNewPane: true, | |||
| 261 | - | FollowPaneContext: true, | |||
| 262 | - | BlankTargetBehavior: "stacked", // Default: open _blank links in stacked mode | |||
| 263 | - | EnableSmartDetection: true, // Use WindowProperties to detect popup vs tab | |||
| 264 | - | OAuthAutoClose: true, // Auto-close OAuth popups on success | |||
| 265 | - | }, | |||
| 266 | - | Popups: PopupBehaviorConfig{ | |||
| 267 | - | Behavior: PopupBehaviorSplit, // Default: open JavaScript popups in split panes | |||
| 268 | - | Placement: defaultPopupPlacement, | |||
| 269 | - | OpenInNewPane: true, | |||
| 270 | - | FollowPaneContext: true, | |||
| 271 | - | BlankTargetBehavior: "stacked", // Default: open _blank links in stacked mode | |||
| 272 | - | EnableSmartDetection: true, // Use WindowProperties to detect popup vs tab | |||
| 273 | - | OAuthAutoClose: true, // Auto-close OAuth popups on success | |||
| 274 | - | }, | |||
| 275 | Styling: WorkspaceStylingConfig{ | 273 | Styling: WorkspaceStylingConfig{ | ||
| 276 | BorderWidth: defaultBorderWidth, | 274 | BorderWidth: defaultBorderWidth, | ||
| 277 | BorderColor: defaultBorderColor, | 275 | BorderColor: defaultBorderColor, | ||
M internal/infrastructure/webkit/webview.go +7 -1
| @@ -1924,9 +1924,12 @@ func (wv *WebView) DestroyWithPolicy(policy string) { | |||
| 1924 | 1924 | wv.OnWebProcessTerminated = nil | |
| 1925 | 1925 | wv.OnPermissionRequest = nil | |
| 1926 | 1926 | ||
| 1927 | - | // 3. Clear async callback references | |
| 1927 | + | // 3. Clear async callback references and popup-hosting state | |
| 1928 | 1928 | wv.mu.Lock() | |
| 1929 | 1929 | wv.asyncCallbacks = nil | |
| 1930 | + | wv.browsingContextDecision = port.HostDecision{} | |
| 1931 | + | wv.hasBrowsingContextDecision = false | |
| 1932 | + | wv.nativePopupHostAbort = nil | |
| 1930 | 1933 | wv.mu.Unlock() | |
| 1931 | 1934 | ||
| 1932 | 1935 | // 4. Unparent from GTK hierarchy (must happen before process termination) | |
| @@ -1994,6 +1997,9 @@ func (wv *WebView) ResetForPoolReuse() { | |||
| 1994 | 1997 | wv.isLoading = false | |
| 1995 | 1998 | wv.asyncCallbacks = nil | |
| 1996 | 1999 | wv.runJSErrorStats = make(map[string]runJSErrorStat) | |
| 2000 | + | wv.browsingContextDecision = port.HostDecision{} | |
| 2001 | + | wv.hasBrowsingContextDecision = false | |
| 2002 | + | wv.nativePopupHostAbort = nil | |
| 1997 | 2003 | wv.lastProgressUpdate.Store(0) | |
| 1998 | 2004 | wv.mu.Unlock() | |
| 1999 | 2005 | ||
| @@ -1924,9 +1924,12 @@ func (wv *WebView) DestroyWithPolicy(policy string) { | |||||
| 1924 | wv.OnWebProcessTerminated = nil | 1924 | wv.OnWebProcessTerminated = nil | ||
| 1925 | wv.OnPermissionRequest = nil | 1925 | wv.OnPermissionRequest = nil | ||
| 1926 | 1926 | ||||
| 1927 | - | // 3. Clear async callback references | 1927 | + | // 3. Clear async callback references and popup-hosting state |
| 1928 | wv.mu.Lock() | 1928 | wv.mu.Lock() | ||
| 1929 | wv.asyncCallbacks = nil | 1929 | wv.asyncCallbacks = nil | ||
| 1930 | + | wv.browsingContextDecision = port.HostDecision{} | |||
| 1931 | + | wv.hasBrowsingContextDecision = false | |||
| 1932 | + | wv.nativePopupHostAbort = nil | |||
| 1930 | wv.mu.Unlock() | 1933 | wv.mu.Unlock() | ||
| 1931 | 1934 | ||||
| 1932 | // 4. Unparent from GTK hierarchy (must happen before process termination) | 1935 | // 4. Unparent from GTK hierarchy (must happen before process termination) | ||
| @@ -1994,6 +1997,9 @@ func (wv *WebView) ResetForPoolReuse() { | |||||
| 1994 | wv.isLoading = false | 1997 | wv.isLoading = false | ||
| 1995 | wv.asyncCallbacks = nil | 1998 | wv.asyncCallbacks = nil | ||
| 1996 | wv.runJSErrorStats = make(map[string]runJSErrorStat) | 1999 | wv.runJSErrorStats = make(map[string]runJSErrorStat) | ||
| 2000 | + | wv.browsingContextDecision = port.HostDecision{} | |||
| 2001 | + | wv.hasBrowsingContextDecision = false | |||
| 2002 | + | wv.nativePopupHostAbort = nil | |||
| 1997 | wv.lastProgressUpdate.Store(0) | 2003 | wv.lastProgressUpdate.Store(0) | ||
| 1998 | wv.mu.Unlock() | 2004 | wv.mu.Unlock() | ||
| 1999 | 2005 | ||||
A internal/infrastructure/webkit/webview_reuse_state_test.go +42 -0
| @@ -0,0 +1,42 @@ | |||
| 1 | + | package webkit | |
| 2 | + | ||
| 3 | + | import ( | |
| 4 | + | "testing" | |
| 5 | + | ||
| 6 | + | "github.com/bnema/dumber/internal/application/port" | |
| 7 | + | "github.com/stretchr/testify/assert" | |
| 8 | + | ) | |
| 9 | + | ||
| 10 | + | func TestResetForPoolReuse_ClearsBrowsingContextHostState(t *testing.T) { | |
| 11 | + | wv := &WebView{} | |
| 12 | + | abortCalled := false | |
| 13 | + | ||
| 14 | + | wv.SetBrowsingContextHostDecision(port.HostDecision{Kind: port.HostDecisionCreateNativeWin}) | |
| 15 | + | wv.SetNativePopupHostAbort(func() { abortCalled = true }) | |
| 16 | + | ||
| 17 | + | wv.ResetForPoolReuse() | |
| 18 | + | ||
| 19 | + | decision, hasDecision := wv.BrowsingContextHostDecision() | |
| 20 | + | assert.False(t, hasDecision) | |
| 21 | + | assert.Equal(t, port.HostDecision{}, decision) | |
| 22 | + | ||
| 23 | + | wv.AbortNativePopupHost() | |
| 24 | + | assert.False(t, abortCalled) | |
| 25 | + | } | |
| 26 | + | ||
| 27 | + | func TestDestroyWithPolicy_ClearsBrowsingContextHostState(t *testing.T) { | |
| 28 | + | wv := &WebView{} | |
| 29 | + | abortCalled := false | |
| 30 | + | ||
| 31 | + | wv.SetBrowsingContextHostDecision(port.HostDecision{Kind: port.HostDecisionCreatePane}) | |
| 32 | + | wv.SetNativePopupHostAbort(func() { abortCalled = true }) | |
| 33 | + | ||
| 34 | + | wv.DestroyWithPolicy(terminatePolicyNever) | |
| 35 | + | ||
| 36 | + | decision, hasDecision := wv.BrowsingContextHostDecision() | |
| 37 | + | assert.False(t, hasDecision) | |
| 38 | + | assert.Equal(t, port.HostDecision{}, decision) | |
| 39 | + | ||
| 40 | + | wv.AbortNativePopupHost() | |
| 41 | + | assert.False(t, abortCalled) | |
| 42 | + | } | |
| @@ -0,0 +1,42 @@ | |||||
| 1 | + | package webkit | |||
| 2 | + | ||||
| 3 | + | import ( | |||
| 4 | + | "testing" | |||
| 5 | + | ||||
| 6 | + | "github.com/bnema/dumber/internal/application/port" | |||
| 7 | + | "github.com/stretchr/testify/assert" | |||
| 8 | + | ) | |||
| 9 | + | ||||
| 10 | + | func TestResetForPoolReuse_ClearsBrowsingContextHostState(t *testing.T) { | |||
| 11 | + | wv := &WebView{} | |||
| 12 | + | abortCalled := false | |||
| 13 | + | ||||
| 14 | + | wv.SetBrowsingContextHostDecision(port.HostDecision{Kind: port.HostDecisionCreateNativeWin}) | |||
| 15 | + | wv.SetNativePopupHostAbort(func() { abortCalled = true }) | |||
| 16 | + | ||||
| 17 | + | wv.ResetForPoolReuse() | |||
| 18 | + | ||||
| 19 | + | decision, hasDecision := wv.BrowsingContextHostDecision() | |||
| 20 | + | assert.False(t, hasDecision) | |||
| 21 | + | assert.Equal(t, port.HostDecision{}, decision) | |||
| 22 | + | ||||
| 23 | + | wv.AbortNativePopupHost() | |||
| 24 | + | assert.False(t, abortCalled) | |||
| 25 | + | } | |||
| 26 | + | ||||
| 27 | + | func TestDestroyWithPolicy_ClearsBrowsingContextHostState(t *testing.T) { | |||
| 28 | + | wv := &WebView{} | |||
| 29 | + | abortCalled := false | |||
| 30 | + | ||||
| 31 | + | wv.SetBrowsingContextHostDecision(port.HostDecision{Kind: port.HostDecisionCreatePane}) | |||
| 32 | + | wv.SetNativePopupHostAbort(func() { abortCalled = true }) | |||
| 33 | + | ||||
| 34 | + | wv.DestroyWithPolicy(terminatePolicyNever) | |||
| 35 | + | ||||
| 36 | + | decision, hasDecision := wv.BrowsingContextHostDecision() | |||
| 37 | + | assert.False(t, hasDecision) | |||
| 38 | + | assert.Equal(t, port.HostDecision{}, decision) | |||
| 39 | + | ||||
| 40 | + | wv.AbortNativePopupHost() | |||
| 41 | + | assert.False(t, abortCalled) | |||
| 42 | + | } | |||
M internal/ui/coordinator/content/auth_window_observer.go +1 -1
| @@ -17,7 +17,6 @@ func (c *Coordinator) ObserveNativePopupAuth(ctx context.Context, input NativePo | |||
| 17 | 17 | } | |
| 18 | 18 | ||
| 19 | 19 | popupID := input.PopupWebView.ID() | |
| 20 | - | c.trackOAuthPopup(popupID, input.ParentPaneID, input.ParentURIAtOpen) | |
| 21 | 20 | ||
| 22 | 21 | oauthWV, ok := input.PopupWebView.(port.OAuthCallbackCapable) | |
| 23 | 22 | if !ok { | |
| @@ -26,6 +25,7 @@ func (c *Coordinator) ObserveNativePopupAuth(ctx context.Context, input NativePo | |||
| 26 | 25 | Msg("native popup auth observation skipped: webview lacks OAuth callbacks") | |
| 27 | 26 | return | |
| 28 | 27 | } | |
| 28 | + | c.trackOAuthPopup(popupID, input.ParentPaneID, input.ParentURIAtOpen) | |
| 29 | 29 | ||
| 30 | 30 | requestClose := func(reason string) { | |
| 31 | 31 | logging.FromContext(ctx).Info(). | |
| @@ -17,7 +17,6 @@ func (c *Coordinator) ObserveNativePopupAuth(ctx context.Context, input NativePo | |||||
| 17 | } | 17 | } | ||
| 18 | 18 | ||||
| 19 | popupID := input.PopupWebView.ID() | 19 | popupID := input.PopupWebView.ID() | ||
| 20 | - | c.trackOAuthPopup(popupID, input.ParentPaneID, input.ParentURIAtOpen) | |||
| 21 | 20 | ||||
| 22 | oauthWV, ok := input.PopupWebView.(port.OAuthCallbackCapable) | 21 | oauthWV, ok := input.PopupWebView.(port.OAuthCallbackCapable) | ||
| 23 | if !ok { | 22 | if !ok { | ||
| @@ -26,6 +25,7 @@ func (c *Coordinator) ObserveNativePopupAuth(ctx context.Context, input NativePo | |||||
| 26 | Msg("native popup auth observation skipped: webview lacks OAuth callbacks") | 25 | Msg("native popup auth observation skipped: webview lacks OAuth callbacks") | ||
| 27 | return | 26 | return | ||
| 28 | } | 27 | } | ||
| 28 | + | c.trackOAuthPopup(popupID, input.ParentPaneID, input.ParentURIAtOpen) | |||
| 29 | 29 | ||||
| 30 | requestClose := func(reason string) { | 30 | requestClose := func(reason string) { | ||
| 31 | logging.FromContext(ctx).Info(). | 31 | logging.FromContext(ctx).Info(). | ||
M internal/ui/coordinator/content/browsing_context_policy.go +1 -1
| @@ -91,7 +91,7 @@ func inferPopupWindowDisposition(req port.PopupRequest) port.WindowDisposition { | |||
| 91 | 91 | if req.TargetDisposition != "" { | |
| 92 | 92 | return req.TargetDisposition | |
| 93 | 93 | } | |
| 94 | - | if req.FrameName == "_blank" { | |
| 94 | + | if strings.EqualFold(strings.TrimSpace(req.FrameName), "_blank") { | |
| 95 | 95 | return port.WindowDispositionNewTab | |
| 96 | 96 | } | |
| 97 | 97 | return port.WindowDispositionNewPopup | |
| @@ -91,7 +91,7 @@ func inferPopupWindowDisposition(req port.PopupRequest) port.WindowDisposition { | |||||
| 91 | if req.TargetDisposition != "" { | 91 | if req.TargetDisposition != "" { | ||
| 92 | return req.TargetDisposition | 92 | return req.TargetDisposition | ||
| 93 | } | 93 | } | ||
| 94 | - | if req.FrameName == "_blank" { | 94 | + | if strings.EqualFold(strings.TrimSpace(req.FrameName), "_blank") { |
| 95 | return port.WindowDispositionNewTab | 95 | return port.WindowDispositionNewTab | ||
| 96 | } | 96 | } | ||
| 97 | return port.WindowDispositionNewPopup | 97 | return port.WindowDispositionNewPopup | ||
M internal/ui/coordinator/content/browsing_context_policy_test.go +28 -0
| @@ -143,3 +143,31 @@ func TestInferPopupTriggerKind_ClassifiesCurrentTabAsNavigation(t *testing.T) { | |||
| 143 | 143 | }) | |
| 144 | 144 | } | |
| 145 | 145 | } | |
| 146 | + | ||
| 147 | + | func TestInferPopupWindowDisposition_NormalizesBlankTarget(t *testing.T) { | |
| 148 | + | t.Parallel() | |
| 149 | + | ||
| 150 | + | tests := []struct { | |
| 151 | + | name string | |
| 152 | + | req port.PopupRequest | |
| 153 | + | want port.WindowDisposition | |
| 154 | + | }{ | |
| 155 | + | { | |
| 156 | + | name: "mixed-case blank target is treated as new tab", | |
| 157 | + | req: port.PopupRequest{FrameName: " _BlAnK "}, | |
| 158 | + | want: port.WindowDispositionNewTab, | |
| 159 | + | }, | |
| 160 | + | { | |
| 161 | + | name: "named target still defaults to popup", | |
| 162 | + | req: port.PopupRequest{FrameName: "shared-pane"}, | |
| 163 | + | want: port.WindowDispositionNewPopup, | |
| 164 | + | }, | |
| 165 | + | } | |
| 166 | + | ||
| 167 | + | for _, tt := range tests { | |
| 168 | + | t.Run(tt.name, func(t *testing.T) { | |
| 169 | + | t.Parallel() | |
| 170 | + | assert.Equal(t, tt.want, inferPopupWindowDisposition(tt.req)) | |
| 171 | + | }) | |
| 172 | + | } | |
| 173 | + | } | |
| @@ -143,3 +143,31 @@ func TestInferPopupTriggerKind_ClassifiesCurrentTabAsNavigation(t *testing.T) { | |||||
| 143 | }) | 143 | }) | ||
| 144 | } | 144 | } | ||
| 145 | } | 145 | } | ||
| 146 | + | ||||
| 147 | + | func TestInferPopupWindowDisposition_NormalizesBlankTarget(t *testing.T) { | |||
| 148 | + | t.Parallel() | |||
| 149 | + | ||||
| 150 | + | tests := []struct { | |||
| 151 | + | name string | |||
| 152 | + | req port.PopupRequest | |||
| 153 | + | want port.WindowDisposition | |||
| 154 | + | }{ | |||
| 155 | + | { | |||
| 156 | + | name: "mixed-case blank target is treated as new tab", | |||
| 157 | + | req: port.PopupRequest{FrameName: " _BlAnK "}, | |||
| 158 | + | want: port.WindowDispositionNewTab, | |||
| 159 | + | }, | |||
| 160 | + | { | |||
| 161 | + | name: "named target still defaults to popup", | |||
| 162 | + | req: port.PopupRequest{FrameName: "shared-pane"}, | |||
| 163 | + | want: port.WindowDispositionNewPopup, | |||
| 164 | + | }, | |||
| 165 | + | } | |||
| 166 | + | ||||
| 167 | + | for _, tt := range tests { | |||
| 168 | + | t.Run(tt.name, func(t *testing.T) { | |||
| 169 | + | t.Parallel() | |||
| 170 | + | assert.Equal(t, tt.want, inferPopupWindowDisposition(tt.req)) | |||
| 171 | + | }) | |||
| 172 | + | } | |||
| 173 | + | } | |||
M internal/ui/coordinator/content/oauth_test.go +19 -0
| @@ -129,6 +129,25 @@ func TestObserveNativePopupAuth_CapturesOAuthStateAndSchedulesParentResumeOnClos | |||
| 129 | 129 | assert.NotNil(t, refreshTimer) | |
| 130 | 130 | } | |
| 131 | 131 | ||
| 132 | + | func TestObserveNativePopupAuth_DoesNotTrackWhenWebViewLacksOAuthCallbacks(t *testing.T) { | |
| 133 | + | popupID := port.WebViewID(1001) | |
| 134 | + | wv := portmocks.NewMockWebView(t) | |
| 135 | + | wv.EXPECT().ID().Return(popupID).Once() | |
| 136 | + | ||
| 137 | + | c := &Coordinator{popups: newPopupManager()} | |
| 138 | + | c.ObserveNativePopupAuth(context.Background(), NativePopupInput{ | |
| 139 | + | ParentPaneID: entity.PaneID("parent-pane"), | |
| 140 | + | ParentURIAtOpen: "https://x.com/i/flow/login", | |
| 141 | + | PopupWebView: wv, | |
| 142 | + | TargetURI: "https://accounts.google.com/o/oauth2/v2/auth", | |
| 143 | + | }) | |
| 144 | + | ||
| 145 | + | c.popups.mu.RLock() | |
| 146 | + | _, ok := c.popups.popupOAuth[popupID] | |
| 147 | + | c.popups.mu.RUnlock() | |
| 148 | + | assert.False(t, ok) | |
| 149 | + | } | |
| 150 | + | ||
| 132 | 151 | func TestSetupOAuthAutoClose_NavigationIgnoresNonTerminalURI(t *testing.T) { | |
| 133 | 152 | popupID := port.WebViewID(1000) | |
| 134 | 153 | wv := &popupOAuthAutoCloseWebViewStub{MockWebView: portmocks.NewMockWebView(t)} | |
| @@ -129,6 +129,25 @@ func TestObserveNativePopupAuth_CapturesOAuthStateAndSchedulesParentResumeOnClos | |||||
| 129 | assert.NotNil(t, refreshTimer) | 129 | assert.NotNil(t, refreshTimer) | ||
| 130 | } | 130 | } | ||
| 131 | 131 | ||||
| 132 | + | func TestObserveNativePopupAuth_DoesNotTrackWhenWebViewLacksOAuthCallbacks(t *testing.T) { | |||
| 133 | + | popupID := port.WebViewID(1001) | |||
| 134 | + | wv := portmocks.NewMockWebView(t) | |||
| 135 | + | wv.EXPECT().ID().Return(popupID).Once() | |||
| 136 | + | ||||
| 137 | + | c := &Coordinator{popups: newPopupManager()} | |||
| 138 | + | c.ObserveNativePopupAuth(context.Background(), NativePopupInput{ | |||
| 139 | + | ParentPaneID: entity.PaneID("parent-pane"), | |||
| 140 | + | ParentURIAtOpen: "https://x.com/i/flow/login", | |||
| 141 | + | PopupWebView: wv, | |||
| 142 | + | TargetURI: "https://accounts.google.com/o/oauth2/v2/auth", | |||
| 143 | + | }) | |||
| 144 | + | ||||
| 145 | + | c.popups.mu.RLock() | |||
| 146 | + | _, ok := c.popups.popupOAuth[popupID] | |||
| 147 | + | c.popups.mu.RUnlock() | |||
| 148 | + | assert.False(t, ok) | |||
| 149 | + | } | |||
| 150 | + | ||||
| 132 | func TestSetupOAuthAutoClose_NavigationIgnoresNonTerminalURI(t *testing.T) { | 151 | func TestSetupOAuthAutoClose_NavigationIgnoresNonTerminalURI(t *testing.T) { | ||
| 133 | popupID := port.WebViewID(1000) | 152 | popupID := port.WebViewID(1000) | ||
| 134 | wv := &popupOAuthAutoCloseWebViewStub{MockWebView: portmocks.NewMockWebView(t)} | 153 | wv := &popupOAuthAutoCloseWebViewStub{MockWebView: portmocks.NewMockWebView(t)} | ||
M internal/ui/coordinator/content/popup_manager.go +11 -5
| @@ -113,6 +113,15 @@ func (pm *popupManager) setWindowIDResolver(fn func(entity.PaneID) (string, bool | |||
| 113 | 113 | pm.windowIDForPane = fn | |
| 114 | 114 | } | |
| 115 | 115 | ||
| 116 | + | func popupTabInsertionConfig(cfg *entity.BrowsingContextConfig) (entity.PopupBehavior, string) { | |
| 117 | + | behavior := GetBehavior(PopupTypeTab, cfg) | |
| 118 | + | placement := "right" | |
| 119 | + | if cfg != nil { | |
| 120 | + | placement = cfg.Placement | |
| 121 | + | } | |
| 122 | + | return behavior, placement | |
| 123 | + | } | |
| 124 | + | ||
| 116 | 125 | func (*popupManager) setBrowsingContextDecision(wv port.WebView, decision port.HostDecision) { | |
| 117 | 126 | if carrier, ok := wv.(port.BrowsingContextHostDecisionCapable); ok { | |
| 118 | 127 | carrier.SetBrowsingContextHostDecision(decision) | |
| @@ -813,6 +822,7 @@ func (pm *popupManager) handleLinkMiddleClick( | |||
| 813 | 822 | log.Error().Err(err).Msg("failed to create webview for middle-click") | |
| 814 | 823 | return false | |
| 815 | 824 | } | |
| 825 | + | pm.setBrowsingContextDecision(newWV, decision) | |
| 816 | 826 | ||
| 817 | 827 | var paneID entity.PaneID | |
| 818 | 828 | if pm.generatePaneID != nil { | |
| @@ -832,11 +842,7 @@ func (pm *popupManager) handleLinkMiddleClick( | |||
| 832 | 842 | hooks.setupWebViewCallbacks(ctx, paneID, newWV) | |
| 833 | 843 | } | |
| 834 | 844 | ||
| 835 | - | behavior := GetBehavior(PopupTypeTab, pm.popupConfig) | |
| 836 | - | placement := "right" | |
| 837 | - | if pm.popupConfig != nil { | |
| 838 | - | placement = pm.popupConfig.Placement | |
| 839 | - | } | |
| 845 | + | behavior, placement := popupTabInsertionConfig(pm.popupConfig) | |
| 840 | 846 | ||
| 841 | 847 | if pm.onInsertPopup != nil { | |
| 842 | 848 | popupInput := InsertPopupInput{ | |
| @@ -113,6 +113,15 @@ func (pm *popupManager) setWindowIDResolver(fn func(entity.PaneID) (string, bool | |||||
| 113 | pm.windowIDForPane = fn | 113 | pm.windowIDForPane = fn | ||
| 114 | } | 114 | } | ||
| 115 | 115 | ||||
| 116 | + | func popupTabInsertionConfig(cfg *entity.BrowsingContextConfig) (entity.PopupBehavior, string) { | |||
| 117 | + | behavior := GetBehavior(PopupTypeTab, cfg) | |||
| 118 | + | placement := "right" | |||
| 119 | + | if cfg != nil { | |||
| 120 | + | placement = cfg.Placement | |||
| 121 | + | } | |||
| 122 | + | return behavior, placement | |||
| 123 | + | } | |||
| 124 | + | ||||
| 116 | func (*popupManager) setBrowsingContextDecision(wv port.WebView, decision port.HostDecision) { | 125 | func (*popupManager) setBrowsingContextDecision(wv port.WebView, decision port.HostDecision) { | ||
| 117 | if carrier, ok := wv.(port.BrowsingContextHostDecisionCapable); ok { | 126 | if carrier, ok := wv.(port.BrowsingContextHostDecisionCapable); ok { | ||
| 118 | carrier.SetBrowsingContextHostDecision(decision) | 127 | carrier.SetBrowsingContextHostDecision(decision) | ||
| @@ -813,6 +822,7 @@ func (pm *popupManager) handleLinkMiddleClick( | |||||
| 813 | log.Error().Err(err).Msg("failed to create webview for middle-click") | 822 | log.Error().Err(err).Msg("failed to create webview for middle-click") | ||
| 814 | return false | 823 | return false | ||
| 815 | } | 824 | } | ||
| 825 | + | pm.setBrowsingContextDecision(newWV, decision) | |||
| 816 | 826 | ||||
| 817 | var paneID entity.PaneID | 827 | var paneID entity.PaneID | ||
| 818 | if pm.generatePaneID != nil { | 828 | if pm.generatePaneID != nil { | ||
| @@ -832,11 +842,7 @@ func (pm *popupManager) handleLinkMiddleClick( | |||||
| 832 | hooks.setupWebViewCallbacks(ctx, paneID, newWV) | 842 | hooks.setupWebViewCallbacks(ctx, paneID, newWV) | ||
| 833 | } | 843 | } | ||
| 834 | 844 | ||||
| 835 | - | behavior := GetBehavior(PopupTypeTab, pm.popupConfig) | 845 | + | behavior, placement := popupTabInsertionConfig(pm.popupConfig) |
| 836 | - | placement := "right" | |||
| 837 | - | if pm.popupConfig != nil { | |||
| 838 | - | placement = pm.popupConfig.Placement | |||
| 839 | - | } | |||
| 840 | 846 | ||||
| 841 | if pm.onInsertPopup != nil { | 847 | if pm.onInsertPopup != nil { | ||
| 842 | popupInput := InsertPopupInput{ | 848 | popupInput := InsertPopupInput{ | ||
M internal/ui/coordinator/content/popup_test.go +16 -2
| @@ -333,7 +333,7 @@ func TestHandleLinkMiddleClick_UsesRelatedWebView(t *testing.T) { | |||
| 333 | 333 | parentWV := mocks.NewMockWebView(t) | |
| 334 | 334 | parentWV.EXPECT().ID().Return(port.WebViewID(101)).Twice() | |
| 335 | 335 | ||
| 336 | - | newWV := mocks.NewMockWebView(t) | |
| 336 | + | newWV := &popupNavigationWebViewStub{MockWebView: mocks.NewMockWebView(t)} | |
| 337 | 337 | newWV.EXPECT().ID().Return(port.WebViewID(301)).Maybe() | |
| 338 | 338 | newWV.EXPECT().Generation().Return(uint64(1)).Maybe() | |
| 339 | 339 | newWV.EXPECT().SetCallbacks(mock.Anything).Maybe() | |
| @@ -359,6 +359,9 @@ func TestHandleLinkMiddleClick_UsesRelatedWebView(t *testing.T) { | |||
| 359 | 359 | handled := c.handleLinkMiddleClick(ctx, parentPaneID, "https://example.com/middle-click") | |
| 360 | 360 | assert.True(t, handled) | |
| 361 | 361 | assert.Equal(t, 1, insertCalls) | |
| 362 | + | decision, hasDecision := newWV.BrowsingContextHostDecision() | |
| 363 | + | require.True(t, hasDecision) | |
| 364 | + | assert.Equal(t, port.HostDecisionCreatePane, decision.Kind) | |
| 362 | 365 | } | |
| 363 | 366 | ||
| 364 | 367 | func TestHandlePopupCreate_OpensNativePopupForAuthIntent(t *testing.T) { | |
| @@ -674,13 +677,24 @@ func TestSetPopupConfig_NilConfigAllowed(t *testing.T) { | |||
| 674 | 677 | ||
| 675 | 678 | type popupNavigationWebViewStub struct { | |
| 676 | 679 | *mocks.MockWebView | |
| 677 | - | primed []string | |
| 680 | + | primed []string | |
| 681 | + | browsingContextDecision port.HostDecision | |
| 682 | + | hasBrowsingContextDecision bool | |
| 678 | 683 | } | |
| 679 | 684 | ||
| 680 | 685 | func (s *popupNavigationWebViewStub) PrimePopupNavigation(uri string) { | |
| 681 | 686 | s.primed = append(s.primed, uri) | |
| 682 | 687 | } | |
| 683 | 688 | ||
| 689 | + | func (s *popupNavigationWebViewStub) SetBrowsingContextHostDecision(decision port.HostDecision) { | |
| 690 | + | s.browsingContextDecision = decision | |
| 691 | + | s.hasBrowsingContextDecision = true | |
| 692 | + | } | |
| 693 | + | ||
| 694 | + | func (s *popupNavigationWebViewStub) BrowsingContextHostDecision() (port.HostDecision, bool) { | |
| 695 | + | return s.browsingContextDecision, s.hasBrowsingContextDecision | |
| 696 | + | } | |
| 697 | + | ||
| 684 | 698 | func (*popupNavigationWebViewStub) SetOnReadyToShow(func()) {} | |
| 685 | 699 | func (*popupNavigationWebViewStub) SetOnClose(func()) {} | |
| 686 | 700 | func (*popupNavigationWebViewStub) Show() {} | |
| @@ -333,7 +333,7 @@ func TestHandleLinkMiddleClick_UsesRelatedWebView(t *testing.T) { | |||||
| 333 | parentWV := mocks.NewMockWebView(t) | 333 | parentWV := mocks.NewMockWebView(t) | ||
| 334 | parentWV.EXPECT().ID().Return(port.WebViewID(101)).Twice() | 334 | parentWV.EXPECT().ID().Return(port.WebViewID(101)).Twice() | ||
| 335 | 335 | ||||
| 336 | - | newWV := mocks.NewMockWebView(t) | 336 | + | newWV := &popupNavigationWebViewStub{MockWebView: mocks.NewMockWebView(t)} |
| 337 | newWV.EXPECT().ID().Return(port.WebViewID(301)).Maybe() | 337 | newWV.EXPECT().ID().Return(port.WebViewID(301)).Maybe() | ||
| 338 | newWV.EXPECT().Generation().Return(uint64(1)).Maybe() | 338 | newWV.EXPECT().Generation().Return(uint64(1)).Maybe() | ||
| 339 | newWV.EXPECT().SetCallbacks(mock.Anything).Maybe() | 339 | newWV.EXPECT().SetCallbacks(mock.Anything).Maybe() | ||
| @@ -359,6 +359,9 @@ func TestHandleLinkMiddleClick_UsesRelatedWebView(t *testing.T) { | |||||
| 359 | handled := c.handleLinkMiddleClick(ctx, parentPaneID, "https://example.com/middle-click") | 359 | handled := c.handleLinkMiddleClick(ctx, parentPaneID, "https://example.com/middle-click") | ||
| 360 | assert.True(t, handled) | 360 | assert.True(t, handled) | ||
| 361 | assert.Equal(t, 1, insertCalls) | 361 | assert.Equal(t, 1, insertCalls) | ||
| 362 | + | decision, hasDecision := newWV.BrowsingContextHostDecision() | |||
| 363 | + | require.True(t, hasDecision) | |||
| 364 | + | assert.Equal(t, port.HostDecisionCreatePane, decision.Kind) | |||
| 362 | } | 365 | } | ||
| 363 | 366 | ||||
| 364 | func TestHandlePopupCreate_OpensNativePopupForAuthIntent(t *testing.T) { | 367 | func TestHandlePopupCreate_OpensNativePopupForAuthIntent(t *testing.T) { | ||
| @@ -674,13 +677,24 @@ func TestSetPopupConfig_NilConfigAllowed(t *testing.T) { | |||||
| 674 | 677 | ||||
| 675 | type popupNavigationWebViewStub struct { | 678 | type popupNavigationWebViewStub struct { | ||
| 676 | *mocks.MockWebView | 679 | *mocks.MockWebView | ||
| 677 | - | primed []string | 680 | + | primed []string |
| 681 | + | browsingContextDecision port.HostDecision | |||
| 682 | + | hasBrowsingContextDecision bool | |||
| 678 | } | 683 | } | ||
| 679 | 684 | ||||
| 680 | func (s *popupNavigationWebViewStub) PrimePopupNavigation(uri string) { | 685 | func (s *popupNavigationWebViewStub) PrimePopupNavigation(uri string) { | ||
| 681 | s.primed = append(s.primed, uri) | 686 | s.primed = append(s.primed, uri) | ||
| 682 | } | 687 | } | ||
| 683 | 688 | ||||
| 689 | + | func (s *popupNavigationWebViewStub) SetBrowsingContextHostDecision(decision port.HostDecision) { | |||
| 690 | + | s.browsingContextDecision = decision | |||
| 691 | + | s.hasBrowsingContextDecision = true | |||
| 692 | + | } | |||
| 693 | + | ||||
| 694 | + | func (s *popupNavigationWebViewStub) BrowsingContextHostDecision() (port.HostDecision, bool) { | |||
| 695 | + | return s.browsingContextDecision, s.hasBrowsingContextDecision | |||
| 696 | + | } | |||
| 697 | + | ||||
| 684 | func (*popupNavigationWebViewStub) SetOnReadyToShow(func()) {} | 698 | func (*popupNavigationWebViewStub) SetOnReadyToShow(func()) {} | ||
| 685 | func (*popupNavigationWebViewStub) SetOnClose(func()) {} | 699 | func (*popupNavigationWebViewStub) SetOnClose(func()) {} | ||
| 686 | func (*popupNavigationWebViewStub) Show() {} | 700 | func (*popupNavigationWebViewStub) Show() {} | ||
M internal/ui/window/popup_window.go +10 -3
| @@ -15,9 +15,10 @@ const ( | |||
| 15 | 15 | ||
| 16 | 16 | // PopupWindow is a lightweight top-level GTK host for native-required popup flows. | |
| 17 | 17 | type PopupWindow struct { | |
| 18 | - | window *gtk.ApplicationWindow | |
| 19 | - | content *gtk.Box | |
| 20 | - | logger zerolog.Logger | |
| 18 | + | window *gtk.ApplicationWindow | |
| 19 | + | content *gtk.Box | |
| 20 | + | currentContent *gtk.Widget | |
| 21 | + | logger zerolog.Logger | |
| 21 | 22 | } | |
| 22 | 23 | ||
| 23 | 24 | func NewPopup(ctx context.Context, app *gtk.Application) (*PopupWindow, error) { | |
| @@ -48,11 +49,16 @@ func (pw *PopupWindow) SetContent(widget *gtk.Widget) { | |||
| 48 | 49 | if pw == nil || pw.content == nil { | |
| 49 | 50 | return | |
| 50 | 51 | } | |
| 52 | + | if pw.currentContent != nil { | |
| 53 | + | pw.content.Remove(pw.currentContent) | |
| 54 | + | pw.currentContent = nil | |
| 55 | + | } | |
| 51 | 56 | if widget == nil { | |
| 52 | 57 | return | |
| 53 | 58 | } | |
| 54 | 59 | widget.SetVisible(true) | |
| 55 | 60 | pw.content.Append(widget) | |
| 61 | + | pw.currentContent = widget | |
| 56 | 62 | } | |
| 57 | 63 | ||
| 58 | 64 | func (pw *PopupWindow) Show() { | |
| @@ -76,6 +82,7 @@ func (pw *PopupWindow) Destroy() { | |||
| 76 | 82 | pw.window.Destroy() | |
| 77 | 83 | pw.window = nil | |
| 78 | 84 | pw.content = nil | |
| 85 | + | pw.currentContent = nil | |
| 79 | 86 | } | |
| 80 | 87 | ||
| 81 | 88 | func (pw *PopupWindow) Window() *gtk.ApplicationWindow { | |
| @@ -15,9 +15,10 @@ const ( | |||||
| 15 | 15 | ||||
| 16 | // PopupWindow is a lightweight top-level GTK host for native-required popup flows. | 16 | // PopupWindow is a lightweight top-level GTK host for native-required popup flows. | ||
| 17 | type PopupWindow struct { | 17 | type PopupWindow struct { | ||
| 18 | - | window *gtk.ApplicationWindow | 18 | + | window *gtk.ApplicationWindow |
| 19 | - | content *gtk.Box | 19 | + | content *gtk.Box |
| 20 | - | logger zerolog.Logger | 20 | + | currentContent *gtk.Widget |
| 21 | + | logger zerolog.Logger | |||
| 21 | } | 22 | } | ||
| 22 | 23 | ||||
| 23 | func NewPopup(ctx context.Context, app *gtk.Application) (*PopupWindow, error) { | 24 | func NewPopup(ctx context.Context, app *gtk.Application) (*PopupWindow, error) { | ||
| @@ -48,11 +49,16 @@ func (pw *PopupWindow) SetContent(widget *gtk.Widget) { | |||||
| 48 | if pw == nil || pw.content == nil { | 49 | if pw == nil || pw.content == nil { | ||
| 49 | return | 50 | return | ||
| 50 | } | 51 | } | ||
| 52 | + | if pw.currentContent != nil { | |||
| 53 | + | pw.content.Remove(pw.currentContent) | |||
| 54 | + | pw.currentContent = nil | |||
| 55 | + | } | |||
| 51 | if widget == nil { | 56 | if widget == nil { | ||
| 52 | return | 57 | return | ||
| 53 | } | 58 | } | ||
| 54 | widget.SetVisible(true) | 59 | widget.SetVisible(true) | ||
| 55 | pw.content.Append(widget) | 60 | pw.content.Append(widget) | ||
| 61 | + | pw.currentContent = widget | |||
| 56 | } | 62 | } | ||
| 57 | 63 | ||||
| 58 | func (pw *PopupWindow) Show() { | 64 | func (pw *PopupWindow) Show() { | ||
| @@ -76,6 +82,7 @@ func (pw *PopupWindow) Destroy() { | |||||
| 76 | pw.window.Destroy() | 82 | pw.window.Destroy() | ||
| 77 | pw.window = nil | 83 | pw.window = nil | ||
| 78 | pw.content = nil | 84 | pw.content = nil | ||
| 85 | + | pw.currentContent = nil | |||
| 79 | } | 86 | } | ||
| 80 | 87 | ||||
| 81 | func (pw *PopupWindow) Window() *gtk.ApplicationWindow { | 88 | func (pw *PopupWindow) Window() *gtk.ApplicationWindow { | ||