fix(ai-chat): let the composer's focus highlight be switched off, and draw a focus ring when it is - #3010
Merged
Conversation
… draw a focus ring when it is
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
Signed-off-by: Ngô Quốc Đạt <datlechin@gmail.com>
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2995.
What the reporter asked for
The switch ships as a context menu item on the composer, Highlight When Focused, rather than a Settings row, so it sits where the annoyance is. The highlight itself is unchanged and stays the default.
Why the "off" state needed more than deleting the highlight
Measured on macOS 27 with a compiled
swiftcprobe in a real key window, countingdrawFocusRingMaskcalls:focusRingType.exterioron theNSTextView.exterioron theNSScrollViewSo the composer drew no native focus ring at all, and the highlight was the only thing marking the field as focused. A toggle that merely removed it would have left a text field with nothing to say it holds the keyboard.
The off state therefore adopts the system focus ring, using the same three overrides
ShortcutRecorderNSViewalready uses (ShortcutRecorderView.swift:59-67):focusRingType = .exterior,drawFocusRingMask(),focusRingMaskBounds. Per the table, the ring has to sit on the scroll view, not the text view. A second probe confirmed no ancestor in the real SwiftUI hosting tree clips it (masksToBoundsis false from the scroll view up toNSThemeFrame), so the exterior ring is not cut off.ComposerHighlightPreferenceis the single answer to "which affordance", so the two can never both paint and never both go missing.ChatComposerMetrics.cornerRadiusgives the SwiftUI shape and the ring mask one owner, the wayQuickSwitcherMetrics.cornerRadiusalready does for the Quick Switcher panel.Two bugs in the highlight, fixed while it stays
Both measured twice with probes. Both made the highlight appear when the field was not focused, which is the same complaint the issue is about.
resignFirstResponderis never sent when the window resigns key, so the SwiftUIisFocusedmirror latched true and the full-strength highlight kept painting on an inactive window, where macOS withdraws every other focus affordance. It now also requirescontrolActiveState == .key.resignFirstResponderis not sent on unparenting either: measured, afterremoveFromSuperviewthe window's first responder has moved away but no resign was delivered.viewDidMoveToWindownow re-reads focus from the window instead of waiting for a callback that never comes. This is the hazard class CLAUDE.md's "Appearance is not lifetime" invariant covers.The highlight now also steps aside for Reduce Transparency and Increase Contrast, which is what
SolidSurfacePreferencealready does for every other translucent surface in the app. Reduce Motion is deliberately not a gate: the gradient is static (stopsis written once and never mutated), and the only motion is the crossfade, which moved off a raw.animationonto the app's ownmotionAnimationgate.Folded in: the composer had no accessible name
setAccessibilityPlaceholderValuesat behindif textView.placeholder != placeholder, butmakeNSViewhad already stored the same string, so the comparison was never true again and the call has not run since #2097. The placeholder is painted indraw(_:)and never reaches the accessibility tree, so VoiceOver announced an unlabelled text area. Moving the call into the property'sdidSet(with its own equality guard, so it does not fire per keystroke) means no assignment path can leave the field nameless.Included because this change is about the composer's focus affordances, and shipping those while the field stays nameless to VoiceOver would be half a fix.
Verification
All through
.claude/skills/fix-issue/scripts/verify.sh.generatePASS,buildPASStestPASS: 36 executed, 36 passed, 0 failed overComposerHighlightPreferenceTests,ChatComposerScrollViewTests,ChatComposerTextViewAccessibilityTests,AISettingsTests,AppSettingsManagerMigrationTestslint0 violationsdocsPASSfocusRingTypeto always.noneand dropping the accessibility call each turn their suites red, so they assert rather than decorate.No UI automation. The composer only renders when an AI provider is configured (
AIChatPanelView.swift:54gates the whole input area onhasConfiguredProvider) and the UI-test sandbox has none, so there is no deterministic way to reach it fromTableProUITests.Tests ran in a separate worktree: a concurrent session had the Kafka plugin mid-refactor in the shared checkout, and
TableProTestscompiles plugin sources, so their in-progress edit broke the test target's build. Nothing in this branch touchesPlugins/.Screenshots are pending. The "before" state was captured and matches the reporter's screenshot exactly. The machine's screen locked partway through the session, so the "after" states (highlight off showing the system focus ring, and the context menu itself) could not be captured. They need a run on an unlocked screen before merge.
Why not remove the highlight outright
Recorded because the evidence points that way and the decision went the other way deliberately. Apple's Intelligence glow means an assistant is working, not this field is focused: Siri's glow appears "when Siri is active", and
nmoverXcode.app/Contents/PlugIns/IDEIntelligenceChat.frameworkputs every glow symbol onUVBanner, none on its prompt input. The HIG's Focus and selection page says to rely on system-provided focus effects and to use a focus ring for a text field. There is also no public glow API:grep -ci glowover the macOS 27 SDK's SwiftUI interface returns 0. Keeping the highlight as the default and making it optional was chosen over removing it.