Let a reader comment on a line that carries a reference

engine/app/javascript/controllers/coplan/reference_preview_controller.js

@@ -20,13 +20,30 @@ export default class extends Controller {
     this.cancelClose()
   }
 
+  // A held button means the reader is sweeping a selection across the line,
+  // not resting on a reference — a card that opens mid-drag covers the very
+  // text they are trying to select and comment on. Each entry point reads
+  // that for itself, so there is no flag to leave stranded when a gesture
+  // ends without a mouseup (a link drag, a release outside the window):
+  // mouseenter carries the button state, and the focus Chromium fires on
+  // mousedown answers with :focus-visible, which a press never matches but
+  // keyboard and assistive-technology focus always do.
   enter(event) {
     const anchor = event.currentTarget
     if (!this.targetFor(anchor)) return
+    if (event.type === "mouseenter" && event.buttons !== 0) return
 
+    const focused = event.type === "focus"
     this.cancelOpen()
     this.cancelClose()
-    this.openTimer = setTimeout(() => this.show(anchor, "hover"), HOVER_OPEN_DELAY)
+    this.openTimer = setTimeout(() => {
+      // Asked as the card is about to open, not while the focus event is
+      // still dispatching: Chrome settles :focus-visible afterwards, so
+      // reading it inside the handler races and intermittently turns a
+      // keyboard reader away.
+      if (focused && !anchor.matches(":focus-visible")) return
+      this.show(anchor, "hover")
+    }, HOVER_OPEN_DELAY)
   }
 
   leave() {
@@ -62,6 +79,13 @@ export default class extends Controller {
   // Preserve ordinary hash-link behavior even while dismissing the preview.
   // Turbo does not consistently navigate same-document fragments after the
   // hover card changes state during the click, so perform the jump directly.
+  // A live selection deliberately does NOT veto the jump. It cannot mean
+  // "this click is the tail of a sweep": the browser dispatches a sweep's
+  // click on the common ancestor paragraph rather than the anchor, and
+  // refuses to start a selection from a press on a link at all. So a
+  // selection here always predates the press, the click is always
+  // deliberate, and swallowing it strands the reader — clicking a link
+  // never collapses a selection, so every retry would be swallowed too.
   follow(event) {
     const href = event.currentTarget.getAttribute("href")
     const target = this.targetFor(event.currentTarget)

engine/app/models/coplan/comment_thread.rb

@@ -160,15 +160,17 @@ def anchor_occurrence_index
 
       # When anchor_start is known, count occurrences before it.
       if anchor_start.present?
-        stripped, pos_map = plan.stripped_content
+        # Folded the same way resolution folds it — the two have to agree on
+        # what the text looks like or they disagree about which occurrence
+        # this is, and the highlight lands on the wrong copy of the phrase.
+        stripped, pos_map = self.class.fold_whitespace(*plan.stripped_content)
         # Map raw anchor_start to its position in the stripped string.
         # Use >= to find the closest valid position if anchor_start falls
         # on a stripped formatting character.
         stripped_start = pos_map.index { |raw_idx| raw_idx >= anchor_start }
         return nil if stripped_start.nil?
 
-        normalized_anchor = anchor_text.gsub("\t", " ")
-        ranges = find_all_occurrences(stripped, normalized_anchor)
+        ranges = find_all_occurrences(stripped, self.class.fold_anchor(anchor_text))
         return ranges.index { |s, _| s >= stripped_start } || 0
       end
 
@@ -196,6 +198,38 @@ def self.strip_markdown(content)
       Plans::MarkdownTextExtractor.call(content)
     end
 
+    # A browser hands back a selection with every run of whitespace collapsed
+    # to a single space: a hard-wrapped paragraph is one flowing line on
+    # screen, and the reader swept across it as one. The extractor keeps the
+    # source's own newlines, so without this a selection that crosses a
+    # wrapped line — reference or not — never resolves.
+    def self.fold_anchor(text)
+      text.gsub(/\s+/, " ")
+    end
+
+    # The same fold over the stripped text, carrying the position map. A run
+    # collapses onto its first character, so ranges still land on real source
+    # positions.
+    def self.fold_whitespace(text, pos_map)
+      folded = +""
+      map = []
+      in_run = false
+
+      text.each_char.with_index do |char, i|
+        if char.match?(/\s/)
+          next if in_run
+          in_run = true
+          folded << " "
+        else
+          in_run = false
+          folded << char
+        end
+        map << pos_map[i]
+      end
+
+      [ folded, map ]
+    end
+
     private
 
     def mark_notifications_read_if_closed
@@ -227,10 +261,10 @@ def resolve_anchor_position
       # cell text without pipe delimiters). Parse the markdown AST to
       # extract plain text with source position mapping.
       if ranges.empty?
-        stripped, pos_map = self.class.strip_markdown(content)
-        # Normalize tabs to spaces — browser selections across table cells
-        # produce tab-separated text, but the stripped markdown uses spaces.
-        normalized_anchor = anchor_text.gsub("\t", " ")
+        stripped, pos_map = self.class.fold_whitespace(*self.class.strip_markdown(content))
+        # Fold the anchor to match: tabs between table cells, and the newlines
+        # a hard-wrapped paragraph carries, both reach us as plain spaces.
+        normalized_anchor = self.class.fold_anchor(anchor_text)
         stripped_ranges = find_all_occurrences(stripped, normalized_anchor)
 
         # Mermaid labels line-break on literal <br/> tags, and the browser

engine/app/services/coplan/plans/markdown_text_extractor.rb

@@ -21,11 +21,16 @@ def initialize(content)
       end
 
       def call
-        doc = Commonmarker.parse(@content)
+        # Parsed with the renderer's own extensions. Reading the page with a
+        # different grammar than we wrote it with is how `[^note]` ended up
+        # here verbatim while the reader saw a superscript number — and a
+        # comment selected across that citation could never be placed.
+        doc = Commonmarker.parse(@content, options: { extension: MarkdownHelper::EXTENSION_OPTIONS })
         byte_to_char = build_byte_to_char_map
         line_byte_offsets = build_line_byte_offsets
         stripped = +""
         pos_map = []
+        @footnote_numbers = {}
 
         extract_text_nodes(doc, line_byte_offsets, byte_to_char, stripped, pos_map)
 
@@ -120,6 +125,14 @@ def extract_text_nodes(node, line_byte_offsets, byte_to_char, stripped, pos_map)
                 pos_map << (char_idx + i)
               end
             end
+          when :footnote_reference
+            # The reader sees the number the renderer assigned, so that is
+            # what a selection sweeping across the citation carries. Its
+            # digits are sentinels: no character of "[^note]" spells "1", and
+            # a selection that merely ends on the marker should resolve back
+            # to the prose before it rather than to half a citation.
+            marker = footnote_number(child, line_byte_offsets, byte_to_char).to_s
+            marker.each_char { |char| append_separator(stripped, pos_map, char) }
           when :softbreak, :linebreak
             pos = child.source_position
             start_byte = line_byte_offsets[pos[:start_line]] + pos[:start_column] - 1
@@ -137,6 +150,20 @@ def append_separator(stripped, pos_map, char)
         stripped << char
         pos_map << -1
       end
+
+      # The number the renderer prints for this citation. Commonmarker counts
+      # by first reference rather than by definition order, and a label cited
+      # twice keeps its original number, so the count is keyed on the label —
+      # which the node itself won't hand over, but its source span will.
+      def footnote_number(node, line_byte_offsets, byte_to_char)
+        pos = node.source_position
+        start_char = byte_to_char[line_byte_offsets[pos[:start_line]] + pos[:start_column] - 1]
+        end_char = byte_to_char[line_byte_offsets[pos[:end_line]] + pos[:end_column] - 1]
+        return "" unless start_char && end_char
+
+        label = @content[start_char..end_char].to_s.delete_prefix("[^").delete_suffix("]")
+        @footnote_numbers[label] ||= @footnote_numbers.size + 1
+      end
     end
   end
 end

spec/models/comment_thread_anchor_spec.rb

@@ -67,34 +67,34 @@
       expect(content[thread.anchor_start...thread.anchor_end]).to eq("unit tests")
     end
 
-    context "when anchor text spans markdown formatting" do
-      # Helper: create a plan with the given markdown, then create a thread
-      # anchored to `dom_text` (what the browser selection returns). Asserts
-      # that the resolved raw range matches `expected_raw`.
-      def assert_anchor_resolves(markdown, dom_text, expected_raw, occurrence: nil)
-        p = CoPlan::Plan.create!(title: "Test", created_by_user: user)
-        v = CoPlan::PlanVersion.create!(
-          plan: p, revision: 1,
-          content_markdown: markdown, actor_type: "human", actor_id: user.id
-        )
-        p.update!(current_plan_version: v, current_revision: 1)
-
-        attrs = {
-          plan_version: p.current_plan_version,
-          created_by_user: user,
-          anchor_text: dom_text
-        }
-        attrs[:anchor_occurrence] = occurrence if occurrence
-
-        thread = p.comment_threads.create!(**attrs)
-        expect(thread.anchor_start).to be_present, "anchor_start should be set for #{dom_text.inspect}"
-        expect(thread.anchor_end).to be_present
-        matched = markdown[thread.anchor_start...thread.anchor_end]
-        expect(matched).to eq(expected_raw),
-          "Expected raw range to be #{expected_raw.inspect}, got #{matched.inspect}"
-        thread
-      end
+    # Helper: create a plan with the given markdown, then create a thread
+    # anchored to `dom_text` (what the browser selection returns). Asserts
+    # that the resolved raw range matches `expected_raw`.
+    def assert_anchor_resolves(markdown, dom_text, expected_raw, occurrence: nil)
+      p = CoPlan::Plan.create!(title: "Test", created_by_user: user)
+      v = CoPlan::PlanVersion.create!(
+        plan: p, revision: 1,
+        content_markdown: markdown, actor_type: "human", actor_id: user.id
+      )
+      p.update!(current_plan_version: v, current_revision: 1)
 
+      attrs = {
+        plan_version: p.current_plan_version,
+        created_by_user: user,
+        anchor_text: dom_text
+      }
+      attrs[:anchor_occurrence] = occurrence if occurrence
+
+      thread = p.comment_threads.create!(**attrs)
+      expect(thread.anchor_start).to be_present, "anchor_start should be set for #{dom_text.inspect}"
+      expect(thread.anchor_end).to be_present
+      matched = markdown[thread.anchor_start...thread.anchor_end]
+      expect(matched).to eq(expected_raw),
+        "Expected raw range to be #{expected_raw.inspect}, got #{matched.inspect}"
+      thread
+    end
+
+    context "when anchor text spans markdown formatting" do
       it "inline code (backticks)" do
         assert_anchor_resolves(
           "Hello `me` you should read this.",
@@ -265,6 +265,69 @@ def assert_anchor_resolves(markdown, dom_text, expected_raw, occurrence: nil)
       end
     end
 
+    # Commenting on a line that carries a citation. The reader sees a
+    # superscript number where the source says `[^label]`, and sweeping a
+    # selection across the line hands back exactly what they saw — so the
+    # citation is where the anchor has to survive a change of alphabet.
+    context "when anchor text crosses a footnote citation" do
+      it "resolves a selection swept through the citation" do
+        assert_anchor_resolves(
+          "The claim holds.[^data] The rest follows.\n\n[^data]: Evidence.",
+          "The claim holds.1 The rest follows.",
+          "The claim holds.[^data] The rest follows."
+        )
+      end
+
+      # A selection that stops on the marker was a selection of the prose;
+      # the number is punctuation the reader swept up on the way past. Half
+      # a citation is not a range anyone can act on.
+      it "leaves the citation out when the selection merely ends on it" do
+        assert_anchor_resolves(
+          "The claim holds.[^data] The rest follows.\n\n[^data]: Evidence.",
+          "The claim holds.1",
+          "The claim holds."
+        )
+      end
+
+      it "numbers by first reference, not by definition order" do
+        assert_anchor_resolves(
+          "See this[^second] and that[^first].\n\n[^first]: One.\n[^second]: Two.",
+          "See this1 and that2.",
+          "See this[^second] and that[^first]."
+        )
+      end
+
+      it "gives a twice-cited label the same number both times" do
+        assert_anchor_resolves(
+          "Here[^a] and later[^b] and again[^a].\n\n[^a]: One.\n[^b]: Two.",
+          "and again1.",
+          "and again[^a]."
+        )
+      end
+    end
+
+    # A paragraph hard-wrapped in the source is one flowing line on screen.
+    # The browser collapses the wrap to a space; the source still has a
+    # newline. Every selection that crossed a wrapped line used to be refused
+    # — citations had nothing to do with it.
+    context "when anchor text crosses a hard-wrapped line" do
+      it "resolves plain prose spanning a source line break" do
+        assert_anchor_resolves(
+          "Watch the error rate for a full\nweek before widening.",
+          "error rate for a full week before widening",
+          "error rate for a full\nweek before widening"
+        )
+      end
+
+      it "resolves a wrapped line that also carries a link" do
+        assert_anchor_resolves(
+          "The numbers live in\n[§2.1](#section-2-1), so read on.",
+          "numbers live in §2.1, so read on",
+          "numbers live in\n[§2.1](#section-2-1), so read on"
+        )
+      end
+    end
+
     it "handles missing anchor_text gracefully" do
       thread = plan.comment_threads.create!(
         plan_version: plan.current_plan_version,
@@ -421,6 +484,30 @@ def assert_anchor_resolves(markdown, dom_text, expected_raw, occurrence: nil)
 
       expect(thread.anchor_occurrence_index).to eq(0)
     end
+
+    # Resolution and counting read the same text or they disagree about which
+    # copy of a repeated phrase this is, and the highlight lands on the wrong
+    # one. Only a phrase that both repeats and crosses a wrapped line can tell
+    # the two apart: unfolded, the count finds no match at all and quietly
+    # answers "the first one".
+    it "counts a repeated phrase that spans a line break" do
+      md = "Alpha the rate is fine\nhere. Beta.\n\nGamma the rate is fine\nhere. Delta."
+      p = CoPlan::Plan.create!(title: "Wrapped Occ", created_by_user: user)
+      v = CoPlan::PlanVersion.create!(
+        plan: p, revision: 1,
+        content_markdown: md, actor_type: "human", actor_id: user.id
+      )
+      p.update!(current_plan_version: v, current_revision: 1)
+
+      thread = p.comment_threads.create!(
+        plan_version: p.current_plan_version,
+        created_by_user: user,
+        anchor_text: "the rate is fine here",
+        anchor_occurrence: 2
+      )
+
+      expect(thread.anchor_occurrence_index).to eq(1)
+    end
   end
 
   describe ".strip_markdown (delegates to Plans::MarkdownTextExtractor)" do
@@ -528,6 +615,34 @@ def assert_anchor_resolves(markdown, dom_text, expected_raw, occurrence: nil)
       stripped, _ = CoPlan::CommentThread.strip_markdown("Line one\nline two")
       expect(stripped).to include("Line one\nline two")
     end
+
+    it "substitutes a footnote citation with the number the reader sees" do
+      stripped, _ = CoPlan::CommentThread.strip_markdown("A claim.[^src] Next.\n\n[^src]: Source.")
+      expect(stripped).to include("A claim.1 Next.")
+    end
+
+    # The digits spell nothing that exists in the source, so they carry the
+    # -1 sentinel every other synthetic character does. The position-map
+    # integrity example above is what would catch a real index here.
+    it "maps citation digits to sentinel positions" do
+      md = "A claim.[^src] Next.\n\n[^src]: Source."
+      stripped, pos_map = CoPlan::CommentThread.strip_markdown(md)
+      expect(pos_map[stripped.index("1")]).to eq(-1)
+    end
+  end
+
+  describe ".fold_whitespace" do
+    it "collapses a run onto its first source position" do
+      folded, map = CoPlan::CommentThread.fold_whitespace("a \n b", [ 0, 1, 2, 3, 4 ])
+      expect(folded).to eq("a b")
+      expect(map).to eq([ 0, 1, 4 ])
+    end
+
+    it "leaves text without runs untouched" do
+      folded, map = CoPlan::CommentThread.fold_whitespace("a b", [ 0, 1, 2 ])
+      expect(folded).to eq("a b")
+      expect(map).to eq([ 0, 1, 2 ])
+    end
   end
 
   describe "#anchor_context_with_highlight" do