From 765725aaef192f8a4506ba3379c05c4943ca5318 Mon Sep 17 00:00:00 2001 From: Daneel Date: Fri, 27 Feb 2026 16:38:12 +0100 Subject: [PATCH] =?UTF-8?q?patches:=20fix=20review=20B1/W1-5=20=E2=80=94?= =?UTF-8?q?=20unwind=20protection,=20dealloc=20leak,=20DEFSYM=20nav,=20lin?= =?UTF-8?q?eRange,=20buffer=20validation,=20select-window?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit B1: setAccessibilitySelectedTextRange: — add record_unwind_protect_void(unblock_input) before block_input to prevent permanently blocked input if Fset_marker signals. W1: EmacsAccessibilityInteractiveSpan — add -dealloc releasing spanLabel/spanValue (MRC copy properties leaked on every span rebuild cycle). W2: ns_ax_event_is_line_nav_key — replace 8x intern_c_string with DEFSYM'd symbols (Qns_ax_next_line etc.) to avoid per-cursor-move obarray lookups. W3: accessibilityRangeForLine: — rewrite from O(n chars) characterAtIndex loop to O(lines) lineRangeForRange, matching accessibilityLineForIndex: pattern. W4: accessibilityChildrenInNavigationOrder — validate buffer before calling ns_ax_scan_interactive_spans to prevent Lisp signals in dispatch_sync context. W5: EmacsAccessibilityBuffer setAccessibilityFocused: — add Fselect_window so VoiceOver focus actually switches the Emacs selected window, with proper unwind protection for block_input. --- ...oundsForRange-for-macOS-Zoom-cursor-.patch | 124 ++++++++++++------ patches/README.txt | 28 ++-- 2 files changed, 97 insertions(+), 55 deletions(-) diff --git a/patches/0001-ns-implement-AXBoundsForRange-for-macOS-Zoom-cursor-.patch b/patches/0001-ns-implement-AXBoundsForRange-for-macOS-Zoom-cursor-.patch index 8dcf2c4..3f5c70c 100644 --- a/patches/0001-ns-implement-AXBoundsForRange-for-macOS-Zoom-cursor-.patch +++ b/patches/0001-ns-implement-AXBoundsForRange-for-macOS-Zoom-cursor-.patch @@ -88,7 +88,7 @@ Lisp calls, read only immutable NSString and scalar cache). etc/NEWS | 11 + src/nsterm.h | 108 ++ src/nsterm.m | 2870 +++++++++++++++++++++++++++++++++++++++++++++++--- - 3 files changed, 2922 insertions(+), 149 deletions(-) + 3 files changed, 2960 insertions(+), 149 deletions(-) diff --git a/etc/NEWS b/etc/NEWS index 7367e3cc..0e4480ad 100644 @@ -228,7 +228,7 @@ index 7c1ee4cf..542e7d59 100644 #endif BOOL font_panel_active; NSFont *font_panel_result; -@@ -528,6 +629,13 @@ enum ns_return_frame_mode +@@ -528,6 +626,13 @@ enum ns_return_frame_mode - (void)windowWillExitFullScreen; - (void)windowDidExitFullScreen; - (void)windowDidBecomeKey; @@ -309,7 +309,7 @@ index 932d209f..ea2de6f2 100644 ns_focus (f, NULL, 0); NSGraphicsContext *ctx = [NSGraphicsContext currentContext]; -@@ -6849,218 +6891,2471 @@ - (BOOL)fulfillService: (NSString *)name withArg: (NSString *)arg +@@ -6849,218 +6891,2498 @@ - (BOOL)fulfillService: (NSString *)name withArg: (NSString *)arg /* ========================================================================== @@ -788,24 +788,26 @@ index 932d209f..ea2de6f2 100644 - result = font_panel_result; - font_panel_result = nil; -+ /* 1. Check Vthis_command for known navigation command symbols. */ ++ /* 1. Check Vthis_command for known navigation command symbols. ++ All symbols are registered via DEFSYM in syms_of_nsterm to avoid ++ per-call obarray lookups in this hot path (runs every cursor move). */ + if (SYMBOLP (Vthis_command) && !NILP (Vthis_command)) + { + Lisp_Object cmd = Vthis_command; + /* Forward line commands. */ -+ if (EQ (cmd, intern_c_string ("next-line")) -+ || EQ (cmd, intern_c_string ("dired-next-line")) -+ || EQ (cmd, intern_c_string ("evil-next-line")) -+ || EQ (cmd, intern_c_string ("evil-next-visual-line"))) ++ if (EQ (cmd, Qns_ax_next_line) ++ || EQ (cmd, Qns_ax_dired_next_line) ++ || EQ (cmd, Qns_ax_evil_next_line) ++ || EQ (cmd, Qns_ax_evil_next_visual_line)) + { + if (which) *which = 1; + return true; + } + /* Backward line commands. */ -+ if (EQ (cmd, intern_c_string ("previous-line")) -+ || EQ (cmd, intern_c_string ("dired-previous-line")) -+ || EQ (cmd, intern_c_string ("evil-previous-line")) -+ || EQ (cmd, intern_c_string ("evil-previous-visual-line"))) ++ if (EQ (cmd, Qns_ax_previous_line) ++ || EQ (cmd, Qns_ax_dired_previous_line) ++ || EQ (cmd, Qns_ax_evil_previous_line) ++ || EQ (cmd, Qns_ax_evil_previous_visual_line)) + { + if (which) *which = -1; + return true; @@ -1129,6 +1131,13 @@ index 932d209f..ea2de6f2 100644 + +@implementation EmacsAccessibilityInteractiveSpan + ++- (void)dealloc ++{ ++ [spanLabel release]; ++ [spanValue release]; ++ [super dealloc]; ++} ++ +- (BOOL) isAccessibilityElement { return YES; } + +- (NSAccessibilityRole) accessibilityRole @@ -1225,6 +1234,18 @@ index 932d209f..ea2de6f2 100644 + } + + struct window *w = [self validWindow]; ++ if (!w) ++ return cachedInteractiveSpans ? cachedInteractiveSpans : @[]; ++ ++ /* Validate buffer before scanning. The Lisp calls inside ++ ns_ax_scan_interactive_spans (Ftext_properties_at, Fplist_get, ++ Fnext_single_property_change) do not signal on valid buffers ++ with valid positions. Verify those preconditions here so we ++ never enter the scan with invalid state, which could longjmp ++ out of a dispatch_sync block and deadlock the AX thread. */ ++ if (!BUFFERP (w->contents) || !XBUFFER (w->contents)) ++ return cachedInteractiveSpans ? cachedInteractiveSpans : @[]; ++ + NSArray *spans = ns_ax_scan_interactive_spans (w, self); + + if (!cachedInteractiveSpans) @@ -1744,6 +1765,10 @@ index 932d209f..ea2de6f2 100644 + + specpdl_ref count = SPECPDL_INDEX (); + record_unwind_current_buffer (); ++ /* Ensure block_input is always matched by unblock_input even if ++ Fset_marker or another Lisp call signals (longjmp). */ ++ record_unwind_protect_void (unblock_input); ++ block_input (); + + /* Convert accessibility index to buffer charpos via mapping. */ + ptrdiff_t charpos = [self charposForAccessibilityIndex:range.location]; @@ -1754,8 +1779,6 @@ index 932d209f..ea2de6f2 100644 + if (charpos > BUF_ZV (b)) + charpos = BUF_ZV (b); + -+ block_input (); -+ + /* Move point directly in the buffer. */ + if (b != current_buffer) + set_buffer_internal_1 (b); @@ -1778,8 +1801,6 @@ index 932d209f..ea2de6f2 100644 + + unbind_to (count, Qnil); + -+ unblock_input (); -+ + /* Update cached state so the next notification cycle doesn't + re-announce this movement. */ + self.cachedPoint = charpos; @@ -1809,14 +1830,22 @@ index 932d209f..ea2de6f2 100644 + if (!view || !view->emacsframe) + return; + ++ /* Use specpdl unwind protection for block_input safety. */ ++ specpdl_ref count = SPECPDL_INDEX (); ++ record_unwind_protect_void (unblock_input); + block_input (); + ++ /* Select the Emacs window so keyboard focus follows VoiceOver. */ ++ struct frame *f = view->emacsframe; ++ if (w != XWINDOW (f->selected_window)) ++ Fselect_window (self.lispWindow, Qnil); ++ + /* Raise the frame's NS window to ensure keyboard focus. */ + NSWindow *nswin = [view window]; + if (nswin && ![nswin isKeyWindow]) + [nswin makeKeyAndOrderFront:nil]; + -+ unblock_input (); ++ unbind_to (count, Qnil); + + /* Post SelectedTextChanged so VoiceOver reads the current line + upon entering text interaction mode. @@ -1945,32 +1974,30 @@ index 932d209f..ea2de6f2 100644 + return NSMakeRange (NSNotFound, 0); + + NSUInteger len = [cachedText length]; -+ NSInteger cur_line = 0; ++ if (len == 0) ++ return (line == 0) ? NSMakeRange (0, 0) ++ : NSMakeRange (NSNotFound, 0); + -+ for (NSUInteger i = 0; i <= len; i++) ++ /* Skip to the requested line using lineRangeForRange — O(lines) ++ not O(chars), consistent with accessibilityLineForIndex:. */ ++ NSInteger cur_line = 0; ++ NSUInteger scan = 0; ++ while (cur_line < line && scan < len) + { -+ if (cur_line == line) -+ { -+ /* Find end of this line. */ -+ NSUInteger line_end = i; -+ while (line_end < len -+ && [cachedText characterAtIndex:line_end] != '\n') -+ line_end++; -+ /* Include the trailing newline so empty lines have length 1. */ -+ if (line_end < len -+ && [cachedText characterAtIndex:line_end] == '\n') -+ line_end++; -+ return NSMakeRange (i, line_end - i); -+ } -+ if (i < len && [cachedText characterAtIndex:i] == '\n') -+ { -+ cur_line++; -+ } ++ NSRange lr = [cachedText lineRangeForRange:NSMakeRange (scan, 0)]; ++ NSUInteger next = NSMaxRange (lr); ++ if (next <= scan) break; /* safety */ ++ cur_line++; ++ scan = next; + } -+ /* Phantom final line after the last newline. */ -+ if (cur_line == line) -+ return NSMakeRange (len, 0); -+ return NSMakeRange (NSNotFound, 0); ++ if (cur_line != line) ++ return NSMakeRange (NSNotFound, 0); ++ ++ /* Return the range of the target line. */ ++ if (scan >= len) ++ return NSMakeRange (len, 0); /* phantom line after final newline */ ++ NSRange lr = [cachedText lineRangeForRange:NSMakeRange (scan, 0)]; ++ return lr; +} + +- (NSRange)accessibilityRangeForIndex:(NSInteger)index @@ -2929,7 +2956,7 @@ index 932d209f..ea2de6f2 100644 int code; unsigned fnKeysym = 0; static NSMutableArray *nsEvArray; -@@ -8237,6 +10532,31 @@ - (void)windowDidBecomeKey /* for direct calls */ +@@ -8237,6 +10559,31 @@ - (void)windowDidBecomeKey /* for direct calls */ XSETFRAME (event.frame_or_window, emacsframe); kbd_buffer_store_event (&event); ns_send_appdefined (-1); // Kick main loop @@ -2961,7 +2988,7 @@ index 932d209f..ea2de6f2 100644 } -@@ -9474,6 +11737,332 @@ - (int) fullscreenState +@@ -9474,6 +11821,332 @@ - (int) fullscreenState return fs_state; } @@ -3294,10 +3321,21 @@ index 932d209f..ea2de6f2 100644 @end /* EmacsView */ -@@ -11303,7 +13892,18 @@ Convert an X font name (XLFD) to an NS font name. +@@ -11303,7 +13976,29 @@ Convert an X font name (XLFD) to an NS font name. DEFSYM (Qns_drag_operation_generic, "ns-drag-operation-generic"); DEFSYM (Qns_handle_drag_motion, "ns-handle-drag-motion"); ++ /* Accessibility: line navigation command symbols for ++ ns_ax_event_is_line_nav_key (hot path, avoid intern per call). */ ++ DEFSYM (Qns_ax_next_line, "next-line"); ++ DEFSYM (Qns_ax_previous_line, "previous-line"); ++ DEFSYM (Qns_ax_dired_next_line, "dired-next-line"); ++ DEFSYM (Qns_ax_dired_previous_line, "dired-previous-line"); ++ DEFSYM (Qns_ax_evil_next_line, "evil-next-line"); ++ DEFSYM (Qns_ax_evil_previous_line, "evil-previous-line"); ++ DEFSYM (Qns_ax_evil_next_visual_line, "evil-next-visual-line"); ++ DEFSYM (Qns_ax_evil_previous_visual_line, "evil-previous-visual-line"); ++ + /* Accessibility span scanning symbols. */ + DEFSYM (Qns_ax_widget, "widget"); + DEFSYM (Qns_ax_button, "button"); diff --git a/patches/README.txt b/patches/README.txt index 549d1a3..cd8914f 100644 --- a/patches/README.txt +++ b/patches/README.txt @@ -2,8 +2,8 @@ EMACS NS VOICEOVER ACCESSIBILITY PATCH ======================================== patch: 0001-ns-implement-AXBoundsForRange-for-macOS-Zoom-cursor-.patch author: Martin Sukany -files: src/nsterm.h (+108 lines) - src/nsterm.m (+2638 ins, -140 del, +2498 net) +files: src/nsterm.h (+105 lines) + src/nsterm.m (+2844 ins, -149 del, +2695 net) OVERVIEW @@ -425,12 +425,15 @@ ZOOM INTEGRATION KEY DESIGN DECISIONS -------------------- - 1. DEFSYM instead of intern for property symbols. + 1. DEFSYM instead of intern for all frequently-used symbols. DEFSYM registers symbols at startup (syms_of_nsterm) and stores - them in C globals (e.g. Qcompletion__string). Using intern() at - every AX scan would perform an obarray lookup on each redisplay - cycle. DEFSYM symbols are also always reachable by the GC via - staticpro, eliminating any risk of premature collection. + them in C globals (e.g. Qns_ax_completion__string, Qns_ax_next_line). + This covers both property scanning symbols and line navigation + command symbols used in ns_ax_event_is_line_nav_key (hot path: + runs on every cursor movement). Using intern() would perform + obarray lookups on each redisplay cycle. DEFSYM symbols are + also always reachable by the GC via staticpro, eliminating any + risk of premature collection. 2. AnnouncementRequested for character moves, not SelectedTextChanged. VoiceOver derives the speech character from SelectedTextChanged by @@ -503,11 +506,12 @@ KNOWN LIMITATIONS covers the common case, but overlay-only changes with a stationary point would be missed. A future fix would compare overlay_modiff. - - Interactive span scan uses property-change jumps to skip - non-interactive regions, but still visits every property boundary. For - large visible buffers this scan runs on every redisplay cycle - whenever interactiveSpansDirty is set. An optimization would use - next_single_property_change to skip non-interactive regions in bulk. + - Interactive span scan uses Fnext_single_property_change across + multiple properties to skip non-interactive regions in bulk, but + still visits every property-change boundary. For buffers with + many overlapping text properties (e.g. heavily fontified source + code), the number of boundaries can be significant. The scan + runs on every redisplay cycle when interactiveSpansDirty is set. - Mode line text is extracted from CHAR_GLYPH rows only. Image glyphs, stretch glyphs, and composed glyphs are silently skipped.