patches: fix review B1/W1-5 — unwind protection, dealloc leak, DEFSYM nav, lineRange, buffer validation, select-window

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.
2026-02-27 16:38:12 +01:00
parent 936c251f11
commit 765725aaef
2 changed files with 97 additions and 55 deletions
--- 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++)
-+    {
-+      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')
+  /* 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)
 +    {
+      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);
+  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");
--- 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 <martin@sukany.cz>
-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.