diff --git a/engine/app/javascript/controllers/coplan/reference_preview_controller.js b/engine/app/javascript/controllers/coplan/reference_preview_controller.js index 463bbba..fd97746 100644 --- a/engine/app/javascript/controllers/coplan/reference_preview_controller.js +++ b/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) diff --git a/engine/app/models/coplan/comment_thread.rb b/engine/app/models/coplan/comment_thread.rb index 7bd1df4..7d28553 100644 --- a/engine/app/models/coplan/comment_thread.rb +++ b/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
tags, and the browser diff --git a/engine/app/services/coplan/plans/markdown_text_extractor.rb b/engine/app/services/coplan/plans/markdown_text_extractor.rb index dea4fc1..d621820 100644 --- a/engine/app/services/coplan/plans/markdown_text_extractor.rb +++ b/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 diff --git a/spec/models/comment_thread_anchor_spec.rb b/spec/models/comment_thread_anchor_spec.rb index 7dc910d..9165f04 100644 --- a/spec/models/comment_thread_anchor_spec.rb +++ b/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 diff --git a/spec/system/references_spec.rb b/spec/system/references_spec.rb index 1a90f8f..92544a9 100644 --- a/spec/system/references_spec.rb +++ b/spec/system/references_spec.rb @@ -83,6 +83,119 @@ def open_add_reference_modal p end + # The reference line, as the reader meets it: one paragraph carrying both + # a citation and a section link. Commenting on it means dragging across + # both, so these gestures have to be real — a scripted Range fires no + # mouseenter and synthesizes no click, which is exactly the machinery + # under test. + def line_with_references + find("#plan-content-body p", match: :first) + end + + def press_at_start_of(element) + page.driver.browser.action + .move_to(element.native, -(element.native.rect.width.to_i / 2) + 2, 0) + .click_and_hold + end + + # A held button outlives a failed example and would silence the preview + # for every later one, reporting one regression as a cascade. + after { page.driver.browser.action.release_actions } + + it "keeps the preview out of the way while a selection is dragged across the line" do + visit plan_page_path(referenced_plan) + + line = line_with_references + section_link = find('a.reference-anchor--section[href="#section-2-1"]') + + # Press on plain text and sweep onto the anchor, holding there. The + # press cannot start on the link — that begins Chrome's native link + # drag instead of a selection — and the held move has to land on the + # anchor, since one pointerMove only fires mouseenter for the element + # under its destination. + press_at_start_of(line).move_to(section_link.native).perform + + # Longer than HOVER_OPEN_DELAY, so an unguarded controller has time to + # open the card. It has to sit between two performs: a `pause` inside + # the chain blocks the renderer, and the timer would never run. + sleep 0.5 + + # Proves the sweep reached the anchor. Without it the assertions below + # would pass just as well for a gesture that missed. + expect(page).to have_css("a.reference-anchor--section:hover", visible: :visible) + expect(page).to have_no_css(".reference-preview", visible: :visible) + expect(section_link["aria-expanded"]).to eq("false") + + page.driver.browser.action.release.perform + + # The suppression is specific to the drag — an ordinary hover still + # previews. Moving off the anchor first is load bearing: the pointer is + # still inside it, and staying inside fires no second mouseenter. + find("#section-2-1").hover + section_link.hover + expect(page).to have_css(".reference-preview__title", text: "2.1 Rollout", visible: :visible) + end + + it "keeps the preview out of the way when the press lands on the reference itself" do + visit plan_page_path(referenced_plan) + + section_link = find('a.reference-anchor--section[href="#section-2-1"]') + + # Crossing the anchor arms the open timer, and the press then focuses + # it — which re-arms, and turns the card away because a press never + # matches :focus-visible. `duration: 0` is what makes this the gesture + # worth testing: Selenium's default 250ms move outlasts + # HOVER_OPEN_DELAY, so the card would already be open before the press + # landed and the example would be asking a different question. + page.driver.browser.action(duration: 0).move_to(section_link.native).click_and_hold.perform + sleep 0.5 + + expect(page).to have_no_css(".reference-preview", visible: :visible) + expect(section_link["aria-expanded"]).to eq("false") + + page.driver.browser.action.release.perform + end + + # The press above is turned away by asking :focus-visible, not by + # refusing focus outright — so the reader who never touches a mouse still + # gets the preview a hover would have given them. + it "still previews for a reader who reaches the reference with the keyboard" do + visit plan_page_path(referenced_plan) + + # Focus is moved directly rather than by tabbing: the citation sits + # ahead of the section link, and walking through it leaves a card + # opening and a close timer in flight that race the assertion. Scripted + # focus matches :focus-visible exactly as Tab does, which is the branch + # under test. + page.execute_script(%{document.querySelector('a.reference-anchor--section').focus()}) + + expect(page.evaluate_script("document.activeElement.className")) + .to include("reference-anchor--section") + expect(page).to have_css(".reference-preview__title", text: "2.1 Rollout", visible: :visible) + end + + # The tempting fix for the drag case is to have #follow bail whenever a + # selection is live. It must not: the browser dispatches a sweep's click + # on the paragraph rather than the anchor and will not start a selection + # from a press on a link, so a selection here is always older than the + # click and the click is always deliberate. Swallowing it strands the + # reader for good, because clicking a link never collapses a selection. + it "still follows a reference clicked while text is selected" do + visit plan_page_path(referenced_plan) + + line = line_with_references + press_at_start_of(line) + .move_to(line.native, (line.native.rect.width.to_i / 2) - 2, 0) + .release + .perform + + expect(page.evaluate_script("document.getSelection().toString()")) + .to include("This claim has evidence") + + find('a.reference-anchor--section[href="#section-2-1"]').click + expect(page.evaluate_script("window.location.hash")).to eq("#section-2-1") + end + it "previews references on hover and follows their links on click" do visit plan_page_path(referenced_plan) @@ -131,6 +244,33 @@ def open_add_reference_modal expect(page.evaluate_script("window.location.hash")).to eq("#section-2-1") expect(page).to have_no_css(".reference-preview__jump") end + + # The whole point of getting the preview out of the way: the reader is + # trying to comment. Everything else here stops at the card, which leaves + # the half that actually fails a reader untested — the selection has to + # survive the trip to the server and come back as a placed anchor. Only a + # real drag proves it: the citation reaches the server as the number the + # reader saw, not as the `[^label]` the source spells. + it "saves a comment selected across a citation" do + visit plan_page_path(referenced_plan) + + line = line_with_references + press_at_start_of(line).move_to(find('a.reference-anchor--section[href="#section-2-1"]').native).release.perform + + find(".comment-popover button").click + within("#new-comment-form") do + fill_in "comment_thread[body_markdown]", with: "Does this cover the rollout?" + click_button "Comment" + end + expect(page).to have_no_css("#new-comment-form", visible: true) + + thread = referenced_plan.comment_threads.reload.last + expect(thread).to be_present + expect(thread.anchor_text).to include("evidence.1") + expect(thread.anchor_start).to be_present + raw = referenced_plan.current_content[thread.anchor_start...thread.anchor_end] + expect(raw).to include("[^launch-data]") + end end describe "adding references via Turbo Stream" do