From 01c9316ef1234805e06caacbbde019cf8b4d50f6 Mon Sep 17 00:00:00 2001 From: jorg-vr Date: Wed, 15 Nov 2023 14:32:50 +0100 Subject: [PATCH 1/4] Disable selection hack while writing a comment --- app/assets/javascripts/code_listing.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/assets/javascripts/code_listing.ts b/app/assets/javascripts/code_listing.ts index 03f3cae785..d4c2a45bc3 100644 --- a/app/assets/javascripts/code_listing.ts +++ b/app/assets/javascripts/code_listing.ts @@ -55,6 +55,10 @@ function initAnnotateButtons(): void { return; // if there is no code selection, let the browser handle the copy event } + if (userAnnotationState.formShown) { + return; // if the annotation form is shown, let the browser handle the copy event + } + const selectedCode = submissionState.code.split("\n").slice(selection.row - 1, selection.row + selection.rows - 1); // on the first and last line, selection might only cover part of the line // only copy the selected columns/characters From 88820aa76674de9b503b63769dfde147f9b19f53 Mon Sep 17 00:00:00 2001 From: jorg-vr Date: Wed, 15 Nov 2023 17:20:17 +0100 Subject: [PATCH 2/4] Recalculate selection in copy trigger --- app/assets/javascripts/code_listing.ts | 17 +++++--- .../annotations/selectionHelpers.ts | 42 ++++++++++++------- 2 files changed, 38 insertions(+), 21 deletions(-) diff --git a/app/assets/javascripts/code_listing.ts b/app/assets/javascripts/code_listing.ts index d4c2a45bc3..297cca1c83 100644 --- a/app/assets/javascripts/code_listing.ts +++ b/app/assets/javascripts/code_listing.ts @@ -9,7 +9,11 @@ import "components/annotations/annotation_options"; import "components/annotations/annotations_count_badge"; import { annotationState } from "state/Annotations"; import { exerciseState } from "state/Exercises"; -import { triggerSelectionEnd } from "components/annotations/selectionHelpers"; +import { + anyRangeInAnnotation, + selectedRangeFromSelection, + triggerSelectionEnd +} from "components/annotations/selectionHelpers"; const MARKING_CLASS = "marked"; @@ -50,13 +54,14 @@ function initAnnotateButtons(): void { // copy only the selected code, this avoids copying the line numbers or extra whitespace from the complex html document.addEventListener("copy", event => { - const selection = userAnnotationState.selectedRange; - if (!selection) { - return; // if there is no code selection, let the browser handle the copy event + const browserSelection = window.getSelection(); + if (browserSelection.isCollapsed || anyRangeInAnnotation(browserSelection)) { + return; // selection is collapsed or contains an annotation, let the browser handle the copy event } - if (userAnnotationState.formShown) { - return; // if the annotation form is shown, let the browser handle the copy event + const selection = selectedRangeFromSelection(browserSelection, true); + if (!selection) { + return; // if there is no code selection, let the browser handle the copy event } const selectedCode = submissionState.code.split("\n").slice(selection.row - 1, selection.row + selection.rows - 1); diff --git a/app/assets/javascripts/components/annotations/selectionHelpers.ts b/app/assets/javascripts/components/annotations/selectionHelpers.ts index 604de9644d..9ceaf23580 100644 --- a/app/assets/javascripts/components/annotations/selectionHelpers.ts +++ b/app/assets/javascripts/components/annotations/selectionHelpers.ts @@ -5,12 +5,26 @@ import { submissionState } from "state/Submissions"; /** * @param node The node to get the offset for - * @param offset The offset within the current node + * @param childIndex The offset within the current node * * @returns The offset in number of characters from the start of the `closest` PRE element * If the element is not inside a PRE element, returns undefined */ -export function getOffset(node: Node, offset: number): number | undefined { +export function getOffset(node: Node, childIndex: number): number | undefined { + let offset = 0; + if (node.nodeType === Node.TEXT_NODE) { + offset += childIndex; + } else { + let precedingText = ""; + for (let i = 0; i < node.childNodes.length && i < childIndex; i++) { + const child = node.childNodes[i]; + if (child.nodeType !== Node.COMMENT_NODE) { + precedingText += child.textContent; + } + } + offset += precedingText.length; + } + if (node.nodeName === "PRE") { return offset; } @@ -19,31 +33,24 @@ export function getOffset(node: Node, offset: number): number | undefined { if (!parent) { return undefined; } + const indexInParent = Array.from(parent.childNodes).indexOf(node as ChildNode); - let precedingText = ""; - for (const child of parent.childNodes) { - if (child === node) { - break; - } - if (child.nodeType !== Node.COMMENT_NODE) { - precedingText += child.textContent; - } - } - return getOffset(parent, offset + precedingText.length); + return getOffset(parent, indexInParent) + offset; } /** * This function translates a selection into a range within the code listing. * If the selection is not inside a code listing row, returns undefined. * - * Multiline selections will always return the whole line for each line in the selection. + * If exact is false, the range will be expanded to include the entire row if the selection spans multiple rows. * In this case the selection param might be modified to match the returned range. * * @param selection The selection to get the range for + * @param exact If false, the range will be expanded to include the entire row if the selection spans multiple rows * @returns The range of the selection in the code listing * Unless both the start and end of the selection are inside a code listing row, returns undefined */ -export function selectedRangeFromSelection(selection: Selection): SelectedRange | undefined { +export function selectedRangeFromSelection(selection: Selection, exact = false): SelectedRange | undefined { // Selection.anchorNode does not behave as expected in firefox, see https://bugzilla.mozilla.org/show_bug.cgi?id=1420854 // So we use the startContainer of the range instead const anchorNode = selection.getRangeAt(0).startContainer; @@ -94,6 +101,11 @@ export function selectedRangeFromSelection(selection: Selection): SelectedRange columns: anchorColumn - focusColumn, }; } + + if ( exact ) { + return range; + } + const codeLines = submissionState.code.split("\n"); // If we have selected nothing on the last row, we don't want to include that row @@ -156,7 +168,7 @@ function rangeInAnnotation(range: Range): boolean { return annotation !== null; } -function anyRangeInAnnotation(selection: Selection): boolean { +export function anyRangeInAnnotation(selection: Selection): boolean { for (let i = 0; i < selection.rangeCount; i++) { if (rangeInAnnotation(selection.getRangeAt(i))) { return true; From 527d1e303b21c522d4dd1aac8ca8af670b6c5b77 Mon Sep 17 00:00:00 2001 From: jorg-vr Date: Thu, 16 Nov 2023 09:50:07 +0100 Subject: [PATCH 3/4] Rerturn undefined when expected --- .../javascripts/components/annotations/selectionHelpers.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/components/annotations/selectionHelpers.ts b/app/assets/javascripts/components/annotations/selectionHelpers.ts index 9ceaf23580..a31d463f6e 100644 --- a/app/assets/javascripts/components/annotations/selectionHelpers.ts +++ b/app/assets/javascripts/components/annotations/selectionHelpers.ts @@ -35,7 +35,8 @@ export function getOffset(node: Node, childIndex: number): number | undefined { } const indexInParent = Array.from(parent.childNodes).indexOf(node as ChildNode); - return getOffset(parent, indexInParent) + offset; + const offsetInParent = getOffset(parent, indexInParent); + return offsetInParent !== undefined ? offsetInParent + offset: undefined; } /** From 1d712f51e15345fc540ca3c53e99326112f5fefa Mon Sep 17 00:00:00 2001 From: jorg-vr Date: Thu, 16 Nov 2023 09:56:25 +0100 Subject: [PATCH 4/4] Fix tests --- .../components/annotations/select.test.ts | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/test/javascript/components/annotations/select.test.ts b/test/javascript/components/annotations/select.test.ts index 9a00199b2b..01c8eccd36 100644 --- a/test/javascript/components/annotations/select.test.ts +++ b/test/javascript/components/annotations/select.test.ts @@ -6,7 +6,9 @@ import { submissionState } from "state/Submissions"; describe("getOffsetTest", () => { it("should return the correct offset", async () => { const context = await fixture("
hello world
"); - const target = context.querySelector("#target"); + const target = context.querySelector("#target").childNodes[0]; + expect(target.textContent).toBe("or"); + expect(target.nodeType).toBe(Node.TEXT_NODE); const offset = getOffset(target, 1); expect(offset).toBe(8); @@ -14,12 +16,23 @@ describe("getOffsetTest", () => { it("should ignore anny offset outside the pre ellement", async () => { const context = await fixture("
123
hello world
"); - const target = context.querySelector("#target"); + const target = context.querySelector("#target").childNodes[0]; + expect(target.textContent).toBe("or"); + expect(target.nodeType).toBe(Node.TEXT_NODE); const offset = getOffset(target, 1); expect(offset).toBe(8); }); + it("should work on the pre element itself", async () => { + const context = await fixture("
hello world
"); + const target = context.querySelector("pre"); + + const offset = getOffset(target, 2); + // offset 2 is teh number of previous children, so `hello` and ` ` are the previous children + expect(offset).toBe(6); + }); + it("should return undefined if the node is not inside a pre element", async () => { const context = await fixture("
hello
 world
"); const target = context.querySelector("#target");