diff --git a/configs/default-config.toml b/configs/default-config.toml index 085862011..ecaa94f1f 100644 --- a/configs/default-config.toml +++ b/configs/default-config.toml @@ -100,8 +100,8 @@ visible_check_enabled = false "Left" = "action move_mouse_relative --dx=-10 --dy=0" "Right" = "action move_mouse_relative --dx=10 --dy=0" -[hints.additional_ax_support] -enable = false # Enable enhanced AX for Electron/Chromium/Firefox/WebKit apps +[hints.web_content_hints] +enabled = false # Chromium/Firefox web-page hints via AXEnhancedUserInterface. Electron works without it additional_electron_bundles = [] additional_chromium_bundles = [] additional_firefox_bundles = [] diff --git a/configs/hints-only-config.toml b/configs/hints-only-config.toml index 32d512ec5..16cbdfe02 100644 --- a/configs/hints-only-config.toml +++ b/configs/hints-only-config.toml @@ -51,8 +51,8 @@ ignore_clickable_check = false # "Shift+L" = "__disabled__" # } -[hints.additional_ax_support] -enable = true +[hints.web_content_hints] +enabled = true additional_electron_bundles = [] additional_chromium_bundles = [] diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index f45d03479..35872e760 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -474,21 +474,21 @@ y_offset = 24 width = 320 ``` -### Additional AX Support +### Web Content Hints -Framework-specific accessibility improvements for Electron, Chromium, Firefox, and WebKit apps: +Makes web-page content inside browsers hintable. Electron apps (VS Code, Slack, and any other) are woken automatically on focus and do not need this setting. -| Option | Type | Default | Description | -| ----------------------------- | ----- | ------- | ---------------------------- | -| `enable` | bool | `false` | Enable additional AX support | -| `additional_electron_bundles` | array | `[]` | Bundle IDs of Electron apps | -| `additional_chromium_bundles` | array | `[]` | Bundle IDs of Chromium apps | -| `additional_firefox_bundles` | array | `[]` | Bundle IDs of Firefox apps | -| `additional_webkit_bundles` | array | `[]` | Bundle IDs of WebKit apps | +| Option | Type | Default | Description | +| ----------------------------- | ----- | ------- | ---------------------------------------------------------------------------------------------------------- | +| `enabled` | bool | `false` | Set `AXEnhancedUserInterface` on Chromium and Firefox browsers so hints reach their web-page content | +| `additional_electron_bundles` | array | `[]` | Extra Electron bundle IDs, so neru recognises their web view and prunes the noisy DOM tree while scanning | +| `additional_chromium_bundles` | array | `[]` | Extra Chromium browser bundle IDs that should get web-content hints | +| `additional_firefox_bundles` | array | `[]` | Extra Firefox browser bundle IDs that should get web-content hints | +| `additional_webkit_bundles` | array | `[]` | Extra WebKit browser bundle IDs, so neru prunes their web DOM tree while scanning | ```toml -[hints.additional_ax_support] -enable = false +[hints.web_content_hints] +enabled = false additional_electron_bundles = [] additional_chromium_bundles = [] additional_firefox_bundles = [] @@ -497,12 +497,15 @@ additional_webkit_bundles = [] Find bundle IDs: `osascript -e 'id of app "Safari"'` +> [!WARNING] +> `enabled` sets the `AXEnhancedUserInterface` attribute on Chromium and Firefox browsers. Under a tiling or window-snapping manager (yabai, Rectangle, Magnet, Amethyst), turning this attribute on can make macOS relayout or move the browser window. neru sets it only while `enabled` is on, and only on apps it treats as Chromium or Firefox browsers. Native apps never receive it, and Electron apps receive it only if you list them in one of the browser arrays yourself. Leave `enabled` off if the window-move side effect bothers you. + > [!TIP] > To support installed PWA apps, add a wildcard bundle ID to the appropriate browser array. For example, to support Brave-installed PWAs, add `"com.brave.Browser.app.*"` to `additional_chromium_bundles`: > > ```toml -> [hints.additional_ax_support] -> enable = true +> [hints.web_content_hints] +> enabled = true > additional_chromium_bundles = ["com.brave.Browser.app.*"] > ``` > diff --git a/docs/TROUBLESHOOTING.md b/docs/TROUBLESHOOTING.md index 7a7f83368..942e7c25e 100644 --- a/docs/TROUBLESHOOTING.md +++ b/docs/TROUBLESHOOTING.md @@ -122,13 +122,13 @@ neru hints # CLI works? log_level = "debug" ``` -### Electron/Chromium/Firefox issues +### Browser web content has no hints -**Enable additional AX support:** +Chromium and Firefox keep their web-page accessibility tree asleep until an app asks for it. Turn on the enhanced attribute so hints reach page content (Electron apps do not need this): ```toml -[hints.additional_ax_support] -enable = true +[hints.web_content_hints] +enabled = true ``` ### Menubar/Dock hints missing @@ -163,35 +163,30 @@ tail -f ~/Library/Logs/neru/app.log ### Hints don't appear in Electron apps -**Electron apps need additional AX support.** +Electron apps (VS Code, Slack, Discord, and any other) are woken automatically the moment they gain focus, with no configuration. neru sets `AXManualAccessibility` on every focused app, which wakes the Electron accessibility tree. -**Solution:** - -Edit `~/.config/neru/config.toml`: +If hints still do not appear, turn on debug logging and confirm the wake happened: ```toml -[hints.additional_ax_support] -enable = true - -# If your app isn't auto-detected, add it: -additional_electron_bundles = [ - "com.your.electronapp", -] +[logging] +log_level = "debug" ``` -Restart Neru: +Restart Neru and watch the log: ```bash pkill neru && neru launch +tail -f ~/Library/Logs/neru/app.log ``` -**Check logs for:** +**Check the log for:** ``` -App requires Electron support -Enabled AXManualAccessibility for: com.your.app +manual accessibility set ``` +If instead you see `manual accessibility set failed`, macOS refused the attribute. For an Electron app that usually means neru is missing macOS Accessibility permission, so check that first. Ordinary native apps that do not implement the attribute log the same line harmlessly. If neither line appears for the app, confirm the app was frontmost when you opened hints. + ### Hints don't appear in Chrome/Firefox content **Browser needs additional AX support.** @@ -434,14 +429,12 @@ pkill -9 neru ### VS Code: Hints don't appear in editor -**Electron AX support needed.** - -**Solution:** +VS Code is an Electron app and is woken automatically on focus, so hints work on its buttons, tabs, and sidebar without configuration. The editor surface itself exposes a thin accessibility tree, so hints can be sparse inside the editing area. For that surface, switch VS Code to the Vision strategy, which detects elements from the rendered window instead of the AX tree: ```toml -[hints.additional_ax_support] -enable = true -# VS Code is auto-detected +[[hints.app_configs]] +bundle_id = "com.microsoft.VSCode" +strategy = "vision" ``` ### Adobe apps: Hints misaligned or missing @@ -632,9 +625,15 @@ grep "com.apple.Safari" ~/Library/Logs/neru/app.log ### Common log messages -**"App requires Electron support"** - Electron app detected, needs AX support enabled +**"manual accessibility set"** - neru woke an app's accessibility tree by setting AXManualAccessibility on it + +**"manual accessibility set failed"** - macOS refused AXManualAccessibility; harmless for a native app that does not implement it, a permission problem for an app that should + +**"enhanced accessibility set for web content"** - neru set AXEnhancedUserInterface on a Chromium/Firefox browser to expose its web page + +**"enhanced accessibility set failed"** - macOS refused AXEnhancedUserInterface for a browser; its web-page content may not be hintable -**"Enabled AXManualAccessibility"** - Electron support activated successfully +**"enhanced accessibility cleared"** - neru removed AXEnhancedUserInterface from a browser after web-content hint support was turned off, so its tiling-window side effect does not persist **"Hints mode activated"** - Hint overlay is active; includes hint count when available diff --git a/internal/app/lifecycle.go b/internal/app/lifecycle.go index f89906cdc..c9ef48f52 100644 --- a/internal/app/lifecycle.go +++ b/internal/app/lifecycle.go @@ -195,6 +195,13 @@ func (a *App) setupAppWatcherCallbacks() { a.handleAppActivation(bundleID) }) + // Drop an app's cached accessibility state when it quits, so a later process + // reusing its pid has the attributes set again instead of inheriting stale + // per-process flags. + a.appWatcher.OnTerminate(func(_, bundleID string) { + electron.ForgetAppAccessibility(bundleID) + }) + // Watch for display parameter changes (monitor unplug/plug, resolution changes) a.appWatcher.OnScreenParametersChanged(func() { a.handleScreenParametersChange() @@ -423,66 +430,29 @@ func (a *App) handleAppActivation(bundleID string) { } if cfg.Hints.Enabled { - if cfg.Hints.AdditionalAXSupport.Enable { - a.handleAdditionalAccessibility(bundleID, cfg) - } + a.handleAdditionalAccessibility(bundleID, cfg) } } -// handleAdditionalAccessibility configures accessibility support for Electron/Chromium/Firefox applications. +// handleAdditionalAccessibility wakes the focused application's accessibility +// tree so hints can read it. It sets `AXManualAccessibility` on any focused app +// (a no-op on apps that do not implement it), as well as +// `AXEnhancedUserInterface` on Chromium and Firefox browsers when +// `hints.web_content_hints` is enabled. The latter is necessary in order to +// expose web-area content but can cause windows to move and prevent tiling +// window managers from working correctly, so it stays off every other app. +// +// The work runs on a background goroutine because EnsureAppAccessibility +// retries a freshly launched app with backoff and would otherwise block the +// activation handler while it waits. func (a *App) handleAdditionalAccessibility(bundleID string, cfg *config.Config) { - config := cfg.Hints.AdditionalAXSupport - - isElectron := electron.ShouldEnableElectronSupport(bundleID, config.AdditionalElectronBundles) - isChromium := electron.ShouldEnableChromiumSupport(bundleID, config.AdditionalChromiumBundles) - isFirefox := electron.ShouldEnableFirefoxSupport(bundleID, config.AdditionalFirefoxBundles) - - if !isElectron && !isChromium && !isFirefox { - return - } - - go func() { - // Apps may need time to initialize their accessibility tree after launch. - // We retry a few times to ensure the accessibility attributes are successfully set. - // Use exponential backoff to minimize latency for fast-booting apps while - // still accommodating slow-booting ones. - const ( - maxRetries = 5 - initialDelay = 100 * time.Millisecond - backoffFactor = 2 - ) - - delay := initialDelay - for range maxRetries { - allSuccess := true + axCfg := cfg.Hints.WebContentHints - if isElectron { - if !electron.EnsureElectronAccessibility(bundleID, a.logger) { - allSuccess = false - } - } - - if isChromium { - if !electron.EnsureChromiumAccessibility(bundleID, a.logger) { - allSuccess = false - } - } - - if isFirefox { - if !electron.EnsureFirefoxAccessibility(bundleID, a.logger) { - allSuccess = false - } - } - - if allSuccess { - return - } + useEnhanced := axCfg.Enabled && + (electron.ShouldEnableChromiumSupport(bundleID, axCfg.AdditionalChromiumBundles) || + electron.ShouldEnableFirefoxSupport(bundleID, axCfg.AdditionalFirefoxBundles)) - // Wait before retrying - time.Sleep(delay) - delay *= backoffFactor - } - }() + go electron.EnsureAppAccessibility(bundleID, useEnhanced, a.logger) } // printStartupInfo displays startup information including registered hotkeys. diff --git a/internal/config/config.go b/internal/config/config.go index 9a8887b60..f4f661bee 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -722,7 +722,7 @@ type HintsConfig struct { AppConfigs []AppConfig `json:"appConfigs" toml:"app_configs"` - AdditionalAXSupport AdditionalAXSupport `json:"additionalAxSupport" toml:"additional_ax_support"` + WebContentHints WebContentHints `json:"webContentHints" toml:"web_content_hints"` Hotkeys map[string]StringOrStringArray `json:"hotkeys" toml:"-"` } @@ -919,9 +919,9 @@ type HeldRepeatConfig struct { Interval int `json:"interval" toml:"interval_ms"` // Interval between subsequent repeats (ms) } -// AdditionalAXSupport defines accessibility support for specific application frameworks. -type AdditionalAXSupport struct { - Enable bool `json:"enable" toml:"enable"` +// WebContentHints configures accessibility for hints inside browser web-page content. +type WebContentHints struct { + Enabled bool `json:"enabled" toml:"enabled"` AdditionalElectronBundles []string `json:"additionalElectronBundles" toml:"additional_electron_bundles"` AdditionalChromiumBundles []string `json:"additionalChromiumBundles" toml:"additional_chromium_bundles"` AdditionalFirefoxBundles []string `json:"additionalFirefoxBundles" toml:"additional_firefox_bundles"` diff --git a/internal/config/config_defaults.go b/internal/config/config_defaults.go index 25f167368..dd1a9a750 100644 --- a/internal/config/config_defaults.go +++ b/internal/config/config_defaults.go @@ -437,8 +437,8 @@ func newDefaultConfig() *Config { AppConfigs: []AppConfig{}, - AdditionalAXSupport: AdditionalAXSupport{ - Enable: false, + WebContentHints: WebContentHints{ + Enabled: false, AdditionalElectronBundles: []string{}, AdditionalChromiumBundles: []string{}, AdditionalFirefoxBundles: []string{}, diff --git a/internal/config/service.go b/internal/config/service.go index 44fed17db..e46bf5dc2 100644 --- a/internal/config/service.go +++ b/internal/config/service.go @@ -313,6 +313,45 @@ func (s *Service) LoadWithValidation(path string) *LoadResult { return configResult } + // [hints.additional_ax_support] is no longer a valid section; it was replaced + // by [hints.web_content_hints]. The loader ignores unknown keys, so a config + // still using the old name would silently lose its settings. Reject it and + // point at the new key. + if hints, ok := raw["hints"].(map[string]any); ok { + if _, present := hints["additional_ax_support"]; present { + configResult.ValidationError = derrors.New( + derrors.CodeInvalidConfig, + "[hints.additional_ax_support] has been renamed to [hints.web_content_hints]. "+ + "Rename that section in your config to keep browser web-content hints.", + ) + configResult.Config = DefaultConfig() + + s.logger.Warn("Config uses the renamed key hints.additional_ax_support", + zap.Error(configResult.ValidationError)) + + return configResult + } + + // web_content_hints.enable was renamed to web_content_hints.enabled. The + // loader ignores unknown keys, so the old field would silently disable + // web-content hints. Reject it and point at the new key. + if webContentHints, ok := hints["web_content_hints"].(map[string]any); ok { + if _, present := webContentHints["enable"]; present { + configResult.ValidationError = derrors.New( + derrors.CodeInvalidConfig, + "[hints.web_content_hints] enable has been renamed to enabled. "+ + "Rename that key in your config to keep browser web-content hints.", + ) + configResult.Config = DefaultConfig() + + s.logger.Warn("Config uses the renamed key hints.web_content_hints.enable", + zap.Error(configResult.ValidationError)) + + return configResult + } + } + } + // Decode into typed config struct (separate pass for validation) _, err := toml.DecodeFile(configResult.ConfigPath, configResult.Config) if err != nil { diff --git a/internal/config/service_test.go b/internal/config/service_test.go index bafc2aa78..b9d9303e3 100644 --- a/internal/config/service_test.go +++ b/internal/config/service_test.go @@ -277,3 +277,95 @@ bundle_id = "com.example.app" }) } } + +func TestService_LoadWithValidation_RenamedWebContentHintsKey(t *testing.T) { + service := config.NewService(config.DefaultConfig(), "", zap.NewNop(), nil) + + t.Run("old additional_ax_support key is rejected with guidance", func(t *testing.T) { + configPath := writeTempToml(t, ` +[hints.additional_ax_support] +enable = true +`) + + result := service.LoadWithValidation(configPath) + + if result.ValidationError == nil { + t.Fatal("LoadWithValidation() expected a ValidationError for the renamed key, got nil") + } + + errMsg := result.ValidationError.Error() + if !strings.Contains(errMsg, "additional_ax_support") || + !strings.Contains(errMsg, "web_content_hints") { + t.Errorf("error = %q, want it to name both the old and new keys", errMsg) + } + }) + + t.Run("new web_content_hints key loads", func(t *testing.T) { + configPath := writeTempToml(t, ` +[hints.web_content_hints] +enabled = true +additional_chromium_bundles = ["com.example.browser"] +`) + + result := service.LoadWithValidation(configPath) + + if result.ValidationError != nil { + t.Fatalf("LoadWithValidation() unexpected ValidationError: %v", result.ValidationError) + } + + if !result.Config.Hints.WebContentHints.Enabled { + t.Error("web_content_hints.enabled did not load as true") + } + + got := result.Config.Hints.WebContentHints.AdditionalChromiumBundles + if len(got) != 1 || got[0] != "com.example.browser" { + t.Errorf("additional_chromium_bundles = %v, want [com.example.browser]", got) + } + }) + + t.Run("old web_content_hints.enable field is rejected with guidance", func(t *testing.T) { + configPath := writeTempToml(t, ` +[hints.web_content_hints] +enable = true +`) + + result := service.LoadWithValidation(configPath) + + if result.ValidationError == nil { + t.Fatal("LoadWithValidation() expected a ValidationError for the renamed field, got nil") + } + + errMsg := result.ValidationError.Error() + if !strings.Contains(errMsg, "enable") || !strings.Contains(errMsg, "enabled") { + t.Errorf("error = %q, want it to name both the old and new field", errMsg) + } + }) + + t.Run("empty old section is still rejected", func(t *testing.T) { + configPath := writeTempToml(t, "[hints.additional_ax_support]\n") + + result := service.LoadWithValidation(configPath) + + if result.ValidationError == nil { + t.Fatal("expected the bare old section to be rejected, got nil") + } + + if !strings.Contains(result.ValidationError.Error(), "web_content_hints") { + t.Errorf("error = %q, want it to name the new key", result.ValidationError.Error()) + } + }) + + t.Run("non-table hints does not misfire the guard", func(t *testing.T) { + configPath := writeTempToml(t, "hints = 5\n") + + // A non-table [hints] must skip the rename guard without panicking. The + // typed decode reports the real type error; the guard must not claim it + // fired. + result := service.LoadWithValidation(configPath) + + if result.ValidationError != nil && + strings.Contains(result.ValidationError.Error(), "has been renamed") { + t.Errorf("rename guard misfired on a non-table hints: %v", result.ValidationError) + } + }) +} diff --git a/internal/core/infra/accessibility/tree.go b/internal/core/infra/accessibility/tree.go index 6aabb8e4a..be880bd28 100644 --- a/internal/core/infra/accessibility/tree.go +++ b/internal/core/infra/accessibility/tree.go @@ -954,12 +954,12 @@ func isUserConfiguredChromiumElectron(bundleID string, configProvider config.Pro return false } - chromiumBundles := cfg.Hints.AdditionalAXSupport.AdditionalChromiumBundles + chromiumBundles := cfg.Hints.WebContentHints.AdditionalChromiumBundles if config.MatchesAdditionalBundle(bundleID, chromiumBundles) { return true } - electronBundles := cfg.Hints.AdditionalAXSupport.AdditionalElectronBundles + electronBundles := cfg.Hints.WebContentHints.AdditionalElectronBundles return config.MatchesAdditionalBundle(bundleID, electronBundles) } @@ -988,6 +988,6 @@ func isWebKit(bundleID string, configProvider config.Provider) bool { return config.MatchesAdditionalBundle( bundleID, - cfg.Hints.AdditionalAXSupport.AdditionalWebKitBundles, + cfg.Hints.WebContentHints.AdditionalWebKitBundles, ) } diff --git a/internal/core/infra/electron/electron.go b/internal/core/infra/electron/electron.go index 04345c76d..6713e5508 100644 --- a/internal/core/infra/electron/electron.go +++ b/internal/core/infra/electron/electron.go @@ -12,239 +12,213 @@ import ( ) const ( - electronAttributeName = "AXManualAccessibility" + manualAttributeName = "AXManualAccessibility" enhancedAttributeName = "AXEnhancedUserInterface" ) -var ( - electronPIDsMu sync.Mutex - electronEnabledPIDs = make(map[int]struct{}) - chromiumPIDsMu sync.Mutex - chromiumEnabledPIDs = make(map[int]struct{}) - firefoxPIDsMu sync.Mutex - firefoxEnabledPIDs = make(map[int]struct{}) -) +// setAttribute is the platform accessibility setter. It is a package variable +// so tests can substitute a fake for the cross-process AX call. +var setAttribute = platformSetApplicationAttribute + +// axState records which accessibility attributes have been set on one running +// application. bundle is the application's bundle id, used to detect pid reuse +// (see enabledPIDs). The *Failed flags remember that a set already failed and +// was logged, so an application that does not support an attribute is retried on +// later focus without repeating the log line. +type axState struct { + bundle string + manual bool + manualFailed bool + enhanced bool + enhancedFailed bool +} -const ( - accessibilityRetryCount = 10 - accessibilityRetryDelay = 100 * time.Millisecond - maxAccessibilityDepth = 10 +// enabledPIDs caches the accessibility state of running applications, keyed by +// pid. An entry is dropped when its application terminates (see +// ForgetAppAccessibility), so a later process that reuses the retired pid starts +// from a clean slate. The axState.bundle field covers the window before that +// termination is processed: if the OS hands the pid to a different application +// first, the mismatched bundle resets the record so the new application has its +// attributes set too. +var ( + enabledPIDsMu sync.Mutex + enabledPIDs = make(map[int]axState) ) -// EnsureElectronAccessibility ensures Electron accessibility is enabled for the provided bundle ID. -func EnsureElectronAccessibility(bundleID string, logger *zap.Logger) bool { - return ensureAccessibility( - bundleID, - &electronPIDsMu, - electronEnabledPIDs, - logger, - true, - ) -} +// EnsureAppAccessibility sets the accessibility attributes that make an +// application's hint targets readable. +// +// `AXManualAccessibility` is set on every application. It wakes Electron and +// Chromium accessibility trees, and is a harmless no-op on applications that do +// not implement it. +// +// `AXEnhancedUserInterface` is set only when useEnhanced is true, which the +// caller restricts to Chromium/Firefox browsers with web-content hints turned +// on. It exposes browser web-area content but can relayout or move windows +// under tiling window managers, so it stays off every other application. +// +// A successful set is cached per pid, so re-focusing an already-woken app does +// no further work. A failed set is not cached, so a later focus retries it. +// +// A freshly launched app may not have its accessibility tree ready the moment +// it first gains focus, so the attributes are set with exponential-backoff +// retries. The retry burst is confined to an app's first encounter: an app +// already handled (woken, or one that does not take the attribute) returns +// after a single attempt, so ordinary native apps are not put through the whole +// burst on every focus. The retries sleep between attempts, so callers run this +// on a goroutine. +func EnsureAppAccessibility(bundleID string, useEnhanced bool, logger *zap.Logger) { + if logger == nil { + logger = zap.NewNop() + } -// EnsureChromiumAccessibility ensures Chromium accessibility is enabled for the provided bundle ID. -func EnsureChromiumAccessibility(bundleID string, logger *zap.Logger) bool { - return ensureAccessibility( - bundleID, - &chromiumPIDsMu, - chromiumEnabledPIDs, - logger, - false, - ) -} + logger = logger.Named("electron") -// EnsureFirefoxAccessibility ensures Firefox accessibility is enabled for the provided bundle ID. -func EnsureFirefoxAccessibility(bundleID string, logger *zap.Logger) bool { - return ensureAccessibility( - bundleID, - &firefoxPIDsMu, - firefoxEnabledPIDs, - logger, - false, + const ( + maxAttempts = 5 + initialDelay = 100 * time.Millisecond + backoffFactor = 2 ) -} -func ensureAccessibility( - bundleID string, - pidsMu *sync.Mutex, - enabledPIDs map[int]struct{}, - logger *zap.Logger, - isElectron bool, -) bool { - if logger == nil { - logger = zap.NewNop() + ready, retry := ensureAppAccessibilityOnce(bundleID, useEnhanced, logger) + if ready || !retry { + return } - logger = logger.Named("electron") + delay := initialDelay + + for attempt := 1; attempt < maxAttempts; attempt++ { + time.Sleep(delay) + delay *= backoffFactor + + if ready, _ := ensureAppAccessibilityOnce(bundleID, useEnhanced, logger); ready { + return + } + } +} +// ensureAppAccessibilityOnce resolves the application for bundleID and applies +// its accessibility attributes once. It reports whether every wanted attribute +// is in place (ready) and whether a backoff retry may still help (retry): true +// while the app is not yet resolvable or is seen for the first time, and false +// once it has been handled. +func ensureAppAccessibilityOnce(bundleID string, useEnhanced bool, logger *zap.Logger) (ready, retry bool) { app := accessibility.ApplicationByBundleID(bundleID) if app == nil { logger.Debug("Application not found for bundle ID", zap.String("bundle_id", bundleID)) - return false + return false, true } info, infoErr := app.Info() if infoErr != nil { - return false + return false, true } pid := info.PID() - if pid <= 0 { - return false - } - - pidsMu.Lock() - - _, already := enabledPIDs[pid] - - pidsMu.Unlock() - - if already { - return true + return false, true } - if hasUsableAccessibilityTree(app, logger) { - markPIDEnabled(pidsMu, enabledPIDs, pid) + return ensurePIDAccessibility(pid, bundleID, useEnhanced, logger) +} - return true +// ensurePIDAccessibility applies and caches the accessibility attributes for a +// resolved pid. It is separated from the application lookup so the caching and +// gating rules can be tested without a live accessibility tree. It reports +// whether every wanted attribute is in place (ready) and whether this is the +// first time the process behind the pid is seen (retry), which the caller uses +// to limit the backoff retry burst to freshly launched apps. +func ensurePIDAccessibility(pid int, bundleID string, useEnhanced bool, logger *zap.Logger) (ready, retry bool) { + enabledPIDsMu.Lock() + state := enabledPIDs[pid] + firstEncounter := !strings.EqualFold(state.bundle, bundleID) + if firstEncounter { + state = axState{bundle: bundleID} } + enabledPIDsMu.Unlock() - if isElectron { - success := platformSetApplicationAttribute(pid, electronAttributeName, true) + if !state.manual { + if setAttribute(pid, manualAttributeName, true) { + state.manual = true + state.manualFailed = false - if !success { logger.Debug( - "Failed to set AXManualAccessibility", - zap.Int("pid", pid), + "manual accessibility set", zap.String("bundle_id", bundleID), + zap.Int("pid", pid), ) - } - } - - if waitForAccessibility(app, logger) { - markPIDEnabled(pidsMu, enabledPIDs, pid) - - return true - } - - success := platformSetApplicationAttribute(pid, enhancedAttributeName, true) - - if !success { - logger.Debug( - "Failed to enable AXEnhancedUserInterface", - zap.Int("pid", pid), - zap.String("bundle_id", bundleID), - ) - - return false - } - - if waitForAccessibility(app, logger) { - markPIDEnabled(pidsMu, enabledPIDs, pid) - - return true - } - - logger.Warn( - "Accessibility could not be enabled", - zap.Int("pid", pid), - zap.String("bundle_id", bundleID), - ) - - return false -} + } else if !state.manualFailed { + state.manualFailed = true -func waitForAccessibility(app *accessibility.Element, logger *zap.Logger) bool { - for range accessibilityRetryCount { - if hasUsableAccessibilityTree(app, logger) { - return true + logger.Debug( + "manual accessibility set failed", + zap.String("bundle_id", bundleID), + zap.Int("pid", pid), + ) } - - time.Sleep(accessibilityRetryDelay) - } - - return false -} - -func hasUsableAccessibilityTree(root *accessibility.Element, logger *zap.Logger) bool { - if root == nil { - return false - } - - type entry struct { - el *accessibility.Element - depth int } - queue := []entry{{root, 0}} - - for len(queue) > 0 { - cur := queue[0] - queue = queue[1:] - - if cur.el == nil { - continue - } - - info, err := cur.el.Info() - if err != nil || info == nil { - continue - } - - role := info.Role() - - switch role { - case "AXWebArea", "AXScrollArea": - logger.Info("Found usable accessibility tree", zap.String("role", role)) - - return true - } - - if cur.depth >= maxAccessibilityDepth { - continue + if useEnhanced { + if !state.enhanced { + if setAttribute(pid, enhancedAttributeName, true) { + state.enhanced = true + state.enhancedFailed = false + + logger.Debug( + "enhanced accessibility set for web content", + zap.String("bundle_id", bundleID), + zap.Int("pid", pid), + ) + } else if !state.enhancedFailed { + state.enhancedFailed = true + + logger.Debug( + "enhanced accessibility set failed", + zap.String("bundle_id", bundleID), + zap.Int("pid", pid), + ) + } } + } else if state.enhanced { + // AXEnhancedUserInterface can relayout or move windows under tiling + // window managers, so when web-content hints are off it is cleared to + // keep that side effect from outlasting the setting. + if setAttribute(pid, enhancedAttributeName, false) { + state.enhanced = false - children, err := cur.el.Children(role) - if err != nil { - continue - } - - for _, child := range children { - queue = append(queue, entry{child, cur.depth + 1}) + logger.Debug( + "enhanced accessibility cleared", + zap.String("bundle_id", bundleID), + zap.Int("pid", pid), + ) } } - return false -} - -func markPIDEnabled( - pidsMu *sync.Mutex, - enabledPIDs map[int]struct{}, - pid int, -) { - pidsMu.Lock() - defer pidsMu.Unlock() + enabledPIDsMu.Lock() + enabledPIDs[pid] = state + enabledPIDsMu.Unlock() - enabledPIDs[pid] = struct{}{} + return state.manual && (!useEnhanced || state.enhanced), firstEncounter } -// ShouldEnableElectronSupport determines if the provided bundle identifier -// should have Electron accessibility manually toggled based on defaults and -// user-specified overrides. -func ShouldEnableElectronSupport(bundleID string, additionalBundles []string) bool { +// ForgetAppAccessibility drops the cached accessibility state for every pid +// belonging to the given bundle id. It is called when an application terminates +// so a later process that reuses a retired pid, or a fresh instance of the same +// app, starts from a clean slate and has its attributes set again. +func ForgetAppAccessibility(bundleID string) { if bundleID == "" { - return false + return } - if config.MatchesAdditionalBundle(bundleID, additionalBundles) { - return true - } - - result := IsLikelyElectronBundle(bundleID) + enabledPIDsMu.Lock() + defer enabledPIDsMu.Unlock() - return result + for pid, state := range enabledPIDs { + if strings.EqualFold(state.bundle, bundleID) { + delete(enabledPIDs, pid) + } + } } // ShouldEnableChromiumSupport determines if Chromium accessibility should be enabled for the provided bundle. @@ -279,23 +253,6 @@ func ShouldEnableFirefoxSupport(bundleID string, additionalBundles []string) boo return result } -// IsLikelyElectronBundle returns true if the provided bundle identifier -// matches a known Electron signature. -func IsLikelyElectronBundle(bundleID string) bool { - lower := strings.ToLower(strings.TrimSpace(bundleID)) - if lower == "" { - return false - } - - for _, exact := range config.KnownElectronBundles { - if strings.EqualFold(strings.TrimSpace(exact), lower) { - return true - } - } - - return false -} - // IsLikelyChromiumBundle returns true if the provided bundle identifier matches a known Chromium signature. // This helps identify applications that would benefit from AXEnhancedUserInterface accessibility improvements. func IsLikelyChromiumBundle(bundleID string) bool { diff --git a/internal/core/infra/electron/electron_test.go b/internal/core/infra/electron/electron_test.go index 6ae0570ed..c6177437a 100644 --- a/internal/core/infra/electron/electron_test.go +++ b/internal/core/infra/electron/electron_test.go @@ -16,54 +16,6 @@ const ( bundleIDMyAppUpper = "COM.EXAMPLE.MYAPP" ) -func TestIsLikelyElectronBundle(t *testing.T) { - tests := []struct { - name string - bundleID string - want bool - }{ - { - name: "known electron bundle", - bundleID: "com.microsoft.VSCode", - want: true, - }, - { - name: "case insensitive match", - bundleID: "COM.MICROSOFT.VSCODE", - want: true, - }, - { - name: testNameUnknownBundle, - bundleID: bundleIDSafari, - want: false, - }, - { - name: testNameEmptyBundle, - bundleID: "", - want: false, - }, - { - name: "whitespace bundle", - bundleID: " ", - want: false, - }, - } - - for _, testCase := range tests { - t.Run(testCase.name, func(t *testing.T) { - got := electron.IsLikelyElectronBundle(testCase.bundleID) - if got != testCase.want { - t.Errorf( - "IsLikelyElectronBundle(%q) = %v, want %v", - testCase.bundleID, - got, - testCase.want, - ) - } - }) - } -} - func TestIsLikelyChromiumBundle(t *testing.T) { tests := []struct { name string @@ -112,64 +64,6 @@ func TestIsLikelyChromiumBundle(t *testing.T) { } } -func TestShouldEnableElectronSupport(t *testing.T) { - tests := []struct { - name string - bundleID string - additionalBundles []string - want bool - }{ - { - name: "known electron bundle", - bundleID: "com.microsoft.VSCode", - additionalBundles: []string{}, - want: true, - }, - { - name: testNameAdditionalBundle, - bundleID: bundleIDMyApp, - additionalBundles: []string{bundleIDMyApp}, - want: true, - }, - { - name: testNameUnknownBundle, - bundleID: bundleIDSafari, - additionalBundles: []string{}, - want: false, - }, - { - name: testNameEmptyBundle, - bundleID: "", - additionalBundles: []string{}, - want: false, - }, - { - name: testNameCaseInsensitiveAdditionalBundle, - bundleID: bundleIDMyApp, - additionalBundles: []string{bundleIDMyAppUpper}, - want: true, - }, - } - - for _, testCase := range tests { - t.Run(testCase.name, func(t *testing.T) { - got := electron.ShouldEnableElectronSupport( - testCase.bundleID, - testCase.additionalBundles, - ) - if got != testCase.want { - t.Errorf( - "ShouldEnableElectronSupport(%q, %v) = %v, want %v", - testCase.bundleID, - testCase.additionalBundles, - got, - testCase.want, - ) - } - }) - } -} - func TestShouldEnableChromiumSupport(t *testing.T) { tests := []struct { name string diff --git a/internal/core/infra/electron/enable_test.go b/internal/core/infra/electron/enable_test.go new file mode 100644 index 000000000..57598c3f2 --- /dev/null +++ b/internal/core/infra/electron/enable_test.go @@ -0,0 +1,321 @@ +package electron + +import ( + "testing" + + "go.uber.org/zap" + "go.uber.org/zap/zaptest/observer" +) + +// fakeSetter stands in for the cross-process AX attribute setter. It records +// which attributes it was asked to set, and with which value, and returns a +// programmed result per attribute (a missing entry means the set fails). +type fakeSetter struct { + calls []string + values []bool + results map[string]bool +} + +func (f *fakeSetter) set(_ int, attribute string, value bool) bool { + f.calls = append(f.calls, attribute) + f.values = append(f.values, value) + + return f.results[attribute] +} + +func (f *fakeSetter) countOf(attribute string) int { + count := 0 + + for _, called := range f.calls { + if called == attribute { + count++ + } + } + + return count +} + +// lastValueOf returns the value of the most recent set of the given attribute, +// and whether the attribute was set at all. +func (f *fakeSetter) lastValueOf(attribute string) (value, found bool) { + for i := len(f.calls) - 1; i >= 0; i-- { + if f.calls[i] == attribute { + return f.values[i], true + } + } + + return false, false +} + +// newEnableTest swaps in a fake setter, clears the pid cache, and returns the +// fake plus a log observer. Everything is restored when the test ends. +func newEnableTest(t *testing.T, results map[string]bool) (*fakeSetter, *observer.ObservedLogs, *zap.Logger) { + t.Helper() + + fake := &fakeSetter{results: results} + + prev := setAttribute + setAttribute = fake.set + + t.Cleanup(func() { + setAttribute = prev + + enabledPIDsMu.Lock() + enabledPIDs = make(map[int]axState) + enabledPIDsMu.Unlock() + }) + + enabledPIDsMu.Lock() + enabledPIDs = make(map[int]axState) + enabledPIDsMu.Unlock() + + core, logs := observer.New(zap.DebugLevel) + + return fake, logs, zap.New(core) +} + +func stateForPID(pid int) axState { + enabledPIDsMu.Lock() + defer enabledPIDsMu.Unlock() + + return enabledPIDs[pid] +} + +func TestEnsurePIDManualSetOnceAndCached(t *testing.T) { + fake, logs, logger := newEnableTest(t, map[string]bool{manualAttributeName: true}) + + ensurePIDAccessibility(100, "com.example.app", false, logger) + ensurePIDAccessibility(100, "com.example.app", false, logger) + + if got := fake.countOf(manualAttributeName); got != 1 { + t.Fatalf("manual set attempts = %d, want 1 (cached after success)", got) + } + + if got := fake.countOf(enhancedAttributeName); got != 0 { + t.Fatalf("enhanced set attempts = %d, want 0 (not requested)", got) + } + + state := stateForPID(100) + if !state.manual || state.enhanced { + t.Fatalf("state = %+v, want manual only", state) + } + + if got := logs.FilterMessage("manual accessibility set").Len(); got != 1 { + t.Fatalf("manual success logs = %d, want 1", got) + } +} + +func TestEnsurePIDManualFailureRetriesAndLogsOnce(t *testing.T) { + fake, logs, logger := newEnableTest(t, map[string]bool{manualAttributeName: false}) + + ensurePIDAccessibility(100, "com.example.app", false, logger) + ensurePIDAccessibility(100, "com.example.app", false, logger) + + if got := fake.countOf(manualAttributeName); got != 2 { + t.Fatalf("manual set attempts = %d, want 2 (a failed set is retried)", got) + } + + state := stateForPID(100) + if state.manual { + t.Fatalf("state.manual = true, want false after a failed set") + } + + if !state.manualFailed { + t.Fatalf("state.manualFailed = false, want true") + } + + if got := logs.FilterMessage("manual accessibility set failed").Len(); got != 1 { + t.Fatalf("manual failure logs = %d, want 1 (logged once, not per focus)", got) + } +} + +func TestEnsurePIDEnhancedOnlyWhenRequested(t *testing.T) { + results := map[string]bool{manualAttributeName: true, enhancedAttributeName: true} + + fake, logs, logger := newEnableTest(t, results) + + ensurePIDAccessibility(100, "com.brave.Browser", false, logger) + + if got := fake.countOf(enhancedAttributeName); got != 0 { + t.Fatalf("enhanced set attempts = %d, want 0 when useEnhanced is false", got) + } + + if state := stateForPID(100); state.enhanced { + t.Fatalf("state.enhanced = true, want false when useEnhanced is false") + } + + if got := logs.FilterMessage("enhanced accessibility set for web content").Len(); got != 0 { + t.Fatalf("enhanced logs = %d, want 0", got) + } +} + +func TestEnsurePIDEnhancedSetWhenRequested(t *testing.T) { + results := map[string]bool{manualAttributeName: true, enhancedAttributeName: true} + + fake, logs, logger := newEnableTest(t, results) + + ensurePIDAccessibility(100, "com.brave.Browser", true, logger) + ensurePIDAccessibility(100, "com.brave.Browser", true, logger) + + if got := fake.countOf(enhancedAttributeName); got != 1 { + t.Fatalf("enhanced set attempts = %d, want 1 (cached after success)", got) + } + + state := stateForPID(100) + if !state.manual || !state.enhanced { + t.Fatalf("state = %+v, want manual and enhanced", state) + } + + if got := logs.FilterMessage("enhanced accessibility set for web content").Len(); got != 1 { + t.Fatalf("enhanced success logs = %d, want 1", got) + } +} + +func TestEnsurePIDEnhancedFailureRetriesAndLogsOnce(t *testing.T) { + results := map[string]bool{manualAttributeName: true, enhancedAttributeName: false} + + fake, logs, logger := newEnableTest(t, results) + + ensurePIDAccessibility(100, "com.brave.Browser", true, logger) + ensurePIDAccessibility(100, "com.brave.Browser", true, logger) + + if got := fake.countOf(enhancedAttributeName); got != 2 { + t.Fatalf("enhanced set attempts = %d, want 2 (a failed set is retried)", got) + } + + state := stateForPID(100) + if state.enhanced || !state.enhancedFailed { + t.Fatalf("state = %+v, want enhanced false and enhancedFailed true", state) + } + + if got := logs.FilterMessage("enhanced accessibility set failed").Len(); got != 1 { + t.Fatalf("enhanced failure logs = %d, want 1", got) + } +} + +func TestEnsurePIDReuseResetsState(t *testing.T) { + fake, _, logger := newEnableTest(t, map[string]bool{manualAttributeName: true}) + + ensurePIDAccessibility(100, "com.first.app", false, logger) + ensurePIDAccessibility(100, "com.second.app", false, logger) + + if got := fake.countOf(manualAttributeName); got != 2 { + t.Fatalf("manual set attempts = %d, want 2 (pid reused by a different bundle)", got) + } + + if state := stateForPID(100); state.bundle != "com.second.app" { + t.Fatalf("state.bundle = %q, want com.second.app", state.bundle) + } +} + +func TestEnsurePIDSameBundleCaseInsensitiveKeepsCache(t *testing.T) { + fake, _, logger := newEnableTest(t, map[string]bool{manualAttributeName: true}) + + ensurePIDAccessibility(100, "com.example.App", false, logger) + ensurePIDAccessibility(100, "COM.EXAMPLE.APP", false, logger) + + if got := fake.countOf(manualAttributeName); got != 1 { + t.Fatalf("manual set attempts = %d, want 1 (same bundle, different case)", got) + } +} + +func TestEnsurePIDReportsReadiness(t *testing.T) { + fake, _, logger := newEnableTest(t, map[string]bool{ + manualAttributeName: true, + enhancedAttributeName: false, + }) + + // Manual only: ready as soon as the manual set succeeds. + if ready, _ := ensurePIDAccessibility(100, "com.example.app", false, logger); !ready { + t.Fatal("manual-only ready = false, want true after the manual set succeeds") + } + + // Enhanced wanted but its set fails: not ready, so the caller retries. + if ready, _ := ensurePIDAccessibility(200, "com.brave.Browser", true, logger); ready { + t.Fatal("ready = true, want false while the enhanced set keeps failing") + } + + fake.results[enhancedAttributeName] = true + + if ready, _ := ensurePIDAccessibility(200, "com.brave.Browser", true, logger); !ready { + t.Fatal("ready = false, want true once every wanted attribute is set") + } +} + +func TestEnsurePIDFirstEncounterGuidesRetry(t *testing.T) { + _, _, logger := newEnableTest(t, map[string]bool{manualAttributeName: false}) + + // First time this pid is seen: worth a backoff retry in case the app is + // still bringing its accessibility tree up. + if _, retry := ensurePIDAccessibility(100, "com.example.app", false, logger); !retry { + t.Fatal("retry = false on first encounter, want true so a slow launch is retried") + } + + // Seen before and still failing: a known app, so no retry burst on a later + // focus. + if _, retry := ensurePIDAccessibility(100, "com.example.app", false, logger); retry { + t.Fatal("retry = true on a later focus, want false so native apps are not re-probed") + } + + // A different process reusing the pid is a fresh encounter again. + if _, retry := ensurePIDAccessibility(100, "com.other.app", false, logger); !retry { + t.Fatal("retry = false after pid reuse, want true for the new process") + } +} + +func TestEnsurePIDEnhancedClearedWhenDisabled(t *testing.T) { + results := map[string]bool{manualAttributeName: true, enhancedAttributeName: true} + + fake, logs, logger := newEnableTest(t, results) + + // Enhanced is on for a browser, then web-content hints are turned off. + ensurePIDAccessibility(100, "com.brave.Browser", true, logger) + ensurePIDAccessibility(100, "com.brave.Browser", false, logger) + + if value, found := fake.lastValueOf(enhancedAttributeName); !found || value { + t.Fatalf("last enhanced set = (value=%v, found=%v), want (false, true) to clear it", value, found) + } + + if state := stateForPID(100); state.enhanced { + t.Fatal("state.enhanced = true, want false after the attribute is cleared") + } + + if got := logs.FilterMessage("enhanced accessibility cleared").Len(); got != 1 { + t.Fatalf("enhanced cleared logs = %d, want 1", got) + } + + // Re-focusing with hints still off does not clear again. + ensurePIDAccessibility(100, "com.brave.Browser", false, logger) + + if got := fake.countOf(enhancedAttributeName); got != 2 { + t.Fatalf("enhanced set attempts = %d, want 2 (one to set, one to clear)", got) + } +} + +func TestForgetAppAccessibilityResetsState(t *testing.T) { + fake, _, logger := newEnableTest(t, map[string]bool{manualAttributeName: true}) + + ensurePIDAccessibility(100, "com.example.App", false, logger) + + // An empty bundle id must not wipe unrelated entries. + ForgetAppAccessibility("") + + if state := stateForPID(100); state.bundle == "" { + t.Fatal("state was cleared by an empty-bundle forget, want it kept") + } + + // The app terminates; its cached state is dropped (bundle match is + // case-insensitive, like the pid-reuse guard). + ForgetAppAccessibility("COM.EXAMPLE.APP") + + if state := stateForPID(100); state.bundle != "" { + t.Fatalf("state = %+v, want empty after the app is forgotten", state) + } + + // A new process reuses the retired pid: the attribute is set again. + ensurePIDAccessibility(100, "com.example.App", false, logger) + + if got := fake.countOf(manualAttributeName); got != 2 { + t.Fatalf("manual set attempts = %d, want 2 (cache cleared on terminate, so the reused pid is set again)", got) + } +}