SD-1695 - add flag for text selection in viewing mode#1891
SD-1695 - add flag for text selection in viewing mode#1891mattConnHarbour wants to merge 3 commits intomainfrom
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3b19fc0cd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (hasPermissionOverride) return false; | ||
| // Allow selection visuals when allowSelectionInViewMode is enabled | ||
| if (this.#options.allowSelectionInViewMode) return false; |
There was a problem hiding this comment.
Returning false here when allowSelectionInViewMode is set makes the rest of the presentation layer treat viewing mode as editable. In particular, #setupInputBridge() uses !this.#isViewLocked() to decide whether to forward key/composition/beforeinput events to the hidden editor, so selection-only mode now forwards IME/composition input and keyboard shortcuts even though Editor.setEditable(false) still marks the document read-only. The Editable extension only blocks beforeinput/keydown, not composition events, so IME input can still mutate the document in viewing mode. Consider keeping #isViewLocked() true for the input bridge (or adding a separate “selection-only” check) so selection visuals are allowed without re-enabling input forwarding.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
#isViewLocked() returning false now also affects HeaderFooterSessionManager's #validateEditPermission() this could allow entering header/footer edit mode even though typing is blocked.
worth verifying that doesn't cause weird UI states, or adding a separate documentMode check there?
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
nice one @mattConnHarbour!
wonder how we didn't catch this previously
ps. don't forget about allowing keyboard interactions as well
| 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) => { |
There was a problem hiding this comment.
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).
| handleKeyDown: (_view, event) => { | |
| handleKeyDown: (_view, event) => { | |
| if (!editor.options.editable) { | |
| if (editor.options.allowSelectionInViewMode) { | |
| // Allow navigation keys for selection | |
| const isNavigationKey = [ | |
| 'ArrowLeft', 'ArrowRight', 'ArrowUp', 'ArrowDown', | |
| 'Home', 'End', 'PageUp', 'PageDown' | |
| ].includes(event.key); | |
| // Allow copy and select all | |
| const isCopyOrSelectAll = (event.ctrlKey || event.metaKey) && | |
| ['c', 'a'].includes(event.key.toLowerCase()); | |
| if (isNavigationKey || isCopyOrSelectAll) return false; | |
| } | |
| return true; | |
| } | |
| return false; | |
| }, |
| if (hasPermissionOverride) return false; | ||
| // Allow selection visuals when allowSelectionInViewMode is enabled | ||
| if (this.#options.allowSelectionInViewMode) return false; |
There was a problem hiding this comment.
#isViewLocked() returning false now also affects HeaderFooterSessionManager's #validateEditPermission() this could allow entering header/footer edit mode even though typing is blocked.
worth verifying that doesn't cause weird UI states, or adding a separate documentMode check there?
| * - Click events are allowed for selection | ||
| * - But text input, keyboard shortcuts, paste, and drop remain blocked | ||
| */ | ||
| export const Editable = Extension.create({ |
There was a problem hiding this comment.
no tests for the new flag (editable.js didn't have tests previously) - which makes it even more dangerous not knowing if this change could impact something
test add at least a unit test for the Cmd+C allowlist to prevent regressions
| * Also returns false when allowSelectionInViewMode is enabled, allowing | ||
| * text selection while still blocking actual edits. | ||
| */ | ||
| #isViewLocked(): boolean { |
There was a problem hiding this comment.
nit: #isViewLocked() returning false while in viewing mode is a bit confusing - the name implies "is the view locked?" but it now means "should we hide selection?".
not worth changing now, but if this method grows more conditions might be worth splitting into #isInViewingMode() and #shouldBlockSelectionVisuals()
3 participants
No description provided.