-
Notifications
You must be signed in to change notification settings - Fork 64
SD-1695 - add flag for text selection in viewing mode #1891
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -6,11 +6,17 @@ import { Extension } from '../Extension.js'; | |||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||
| * When editable is false, all user interactions are blocked: | ||||||||||||||||||||||||||||||||||||||||||
| * - Text input via beforeinput events | ||||||||||||||||||||||||||||||||||||||||||
| * - Mouse interactions via mousedown | ||||||||||||||||||||||||||||||||||||||||||
| * - Focus via automatic blur | ||||||||||||||||||||||||||||||||||||||||||
| * - Click, double-click, and triple-click events | ||||||||||||||||||||||||||||||||||||||||||
| * - Mouse interactions via mousedown (unless allowSelectionInViewMode is true) | ||||||||||||||||||||||||||||||||||||||||||
| * - Focus via automatic blur (unless allowSelectionInViewMode is true) | ||||||||||||||||||||||||||||||||||||||||||
| * - Click, double-click, and triple-click events (unless allowSelectionInViewMode is true) | ||||||||||||||||||||||||||||||||||||||||||
| * - Keyboard shortcuts via handleKeyDown | ||||||||||||||||||||||||||||||||||||||||||
| * - Paste and drop events | ||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||
| * When allowSelectionInViewMode is true and editable is false: | ||||||||||||||||||||||||||||||||||||||||||
| * - Mouse interactions are allowed for text selection | ||||||||||||||||||||||||||||||||||||||||||
| * - Focus is allowed | ||||||||||||||||||||||||||||||||||||||||||
| * - Click events are allowed for selection | ||||||||||||||||||||||||||||||||||||||||||
| * - But text input, keyboard shortcuts, paste, and drop remain blocked | ||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||
| export const Editable = Extension.create({ | ||||||||||||||||||||||||||||||||||||||||||
| name: 'editable', | ||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -30,25 +36,39 @@ export const Editable = Extension.create({ | |||||||||||||||||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||
| mousedown: (_view, event) => { | ||||||||||||||||||||||||||||||||||||||||||
| if (!editor.options.editable) { | ||||||||||||||||||||||||||||||||||||||||||
| // Allow mousedown for selection when allowSelectionInViewMode is enabled | ||||||||||||||||||||||||||||||||||||||||||
| if (!editor.options.editable && !editor.options.allowSelectionInViewMode) { | ||||||||||||||||||||||||||||||||||||||||||
| event.preventDefault(); | ||||||||||||||||||||||||||||||||||||||||||
| return true; | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||
| focus: (view, event) => { | ||||||||||||||||||||||||||||||||||||||||||
| if (!editor.options.editable) { | ||||||||||||||||||||||||||||||||||||||||||
| // Allow focus when allowSelectionInViewMode is enabled | ||||||||||||||||||||||||||||||||||||||||||
| if (!editor.options.editable && !editor.options.allowSelectionInViewMode) { | ||||||||||||||||||||||||||||||||||||||||||
| event.preventDefault(); | ||||||||||||||||||||||||||||||||||||||||||
| view.dom.blur(); | ||||||||||||||||||||||||||||||||||||||||||
| return true; | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||
| handleClick: () => !editor.options.editable, | ||||||||||||||||||||||||||||||||||||||||||
| handleDoubleClick: () => !editor.options.editable, | ||||||||||||||||||||||||||||||||||||||||||
| handleTripleClick: () => !editor.options.editable, | ||||||||||||||||||||||||||||||||||||||||||
| handleKeyDown: () => !editor.options.editable, | ||||||||||||||||||||||||||||||||||||||||||
| // Allow click events for selection when allowSelectionInViewMode is enabled | ||||||||||||||||||||||||||||||||||||||||||
| handleClick: () => !editor.options.editable && !editor.options.allowSelectionInViewMode, | ||||||||||||||||||||||||||||||||||||||||||
| handleDoubleClick: () => !editor.options.editable && !editor.options.allowSelectionInViewMode, | ||||||||||||||||||||||||||||||||||||||||||
| handleTripleClick: () => !editor.options.editable && !editor.options.allowSelectionInViewMode, | ||||||||||||||||||||||||||||||||||||||||||
| // Always block keyboard input, paste, and drop when not editable | ||||||||||||||||||||||||||||||||||||||||||
| handleKeyDown: (_view, event) => { | ||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this only allows Cmd+C keyboard users or accessibility can't select (Shift+Arrow to extend, Cmd+A to select all) and/or navigate (arrows, Home/End).
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||
| if (!editor.options.editable) { | ||||||||||||||||||||||||||||||||||||||||||
| // Allow Ctrl+C / Cmd+C for copy when allowSelectionInViewMode is enabled | ||||||||||||||||||||||||||||||||||||||||||
| if (editor.options.allowSelectionInViewMode) { | ||||||||||||||||||||||||||||||||||||||||||
| const isCopy = (event.ctrlKey || event.metaKey) && event.key.toLowerCase() === 'c'; | ||||||||||||||||||||||||||||||||||||||||||
| if (isCopy) return false; | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| return true; | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||
| handlePaste: () => !editor.options.editable, | ||||||||||||||||||||||||||||||||||||||||||
| handleDrop: () => !editor.options.editable, | ||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1040,6 +1040,10 @@ export class PresentationEditor extends EventEmitter { | |
| #syncDocumentModeClass() { | ||
| if (!this.#visibleHost) return; | ||
| this.#visibleHost.classList.toggle('presentation-editor--viewing', this.#documentMode === 'viewing'); | ||
| this.#visibleHost.classList.toggle( | ||
| 'presentation-editor--allow-selection', | ||
| this.#documentMode === 'viewing' && !!this.#options.allowSelectionInViewMode, | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -4611,12 +4615,16 @@ export class PresentationEditor extends EventEmitter { | |
| * Determines whether the current viewing mode should block edits. | ||
| * When documentMode is viewing but the active editor has been toggled | ||
| * back to editable (e.g. permission ranges), we treat the view as editable. | ||
| * Also returns false when allowSelectionInViewMode is enabled, allowing | ||
| * text selection while still blocking actual edits. | ||
| */ | ||
| #isViewLocked(): boolean { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: not worth changing now, but if this method grows more conditions might be worth splitting into |
||
| if (this.#documentMode !== 'viewing') return false; | ||
| const hasPermissionOverride = !!(this.#editor as Editor & { storage?: Record<string, any> })?.storage | ||
| ?.permissionRanges?.hasAllowedRanges; | ||
| if (hasPermissionOverride) return false; | ||
| // Allow selection visuals when allowSelectionInViewMode is enabled | ||
| if (this.#options.allowSelectionInViewMode) return false; | ||
|
Comment on lines
4625
to
+4627
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Returning Useful? React with 👍 / 👎.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
worth verifying that doesn't cause weird UI states, or adding a separate documentMode check there? |
||
| return this.#documentMode === 'viewing'; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,3 @@ | ||
| { | ||
| "extends": "./tsconfig.json", | ||
| "compilerOptions": { | ||
| "customConditions": [] | ||
| } | ||
| "extends": "./tsconfig.json" | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no tests for the new flag (
editable.jsdidn't have tests previously) - which makes it even more dangerous not knowing if this change could impact somethingtest add at least a unit test for the Cmd+C allowlist to prevent regressions