[72837] Click position is lost when activating an inline edit field#22232
[72837] Click position is lost when activating an inline edit field#22232HDinger wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to preserve the user’s click position when activating an inline edit field that renders a CKEditor instance, so the caret is placed where the user clicked.
Changes:
- Persist the triggering click coordinates on
document.body.datasetwhen starting the Turbo-stream request for the inplace edit form. - On CKEditor wrapper connect, read those coordinates and attempt to set the CKEditor selection to the corresponding DOM/model range before focusing.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| frontend/src/stimulus/controllers/dynamic/inplace-edit.controller.ts | Stores (and clears) click coordinates on the body dataset when issuing the Turbo-stream fetch for inplace edit. |
| frontend/src/stimulus/controllers/ckeditor-focus.controller.ts | Reads stored click coordinates and tries to translate them into a CKEditor selection before focusing the editor. |
You can also share your feedback on Copilot code review. Take the survey.
HDinger
force-pushed
the
bug/72837-click-position-is-lost-when-activating-an-inline-edit-field
branch
from
087a884 to
d463e59
Compare
Dependency ReviewThe following issues were found:
|
HDinger
force-pushed
the
bug/72837-click-position-is-lost-when-activating-an-inline-edit-field
branch
from
d463e59 to
8b783cb
Compare
myabc
force-pushed
the
bug/72837-click-position-is-lost-when-activating-an-inline-edit-field
branch
from
8b783cb to
a175e93
Compare
| // Don't trigger edit mode if the user is selecting text or just finished a selection | ||
| if (window.getSelection()?.toString()) { | ||
| return; | ||
| } | ||
|
|
||
| // Don't trigger edit mode if clicking on a link | ||
| const target = e.target as HTMLElement; | ||
| if (target.tagName === 'a' || target.closest('a')) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
request() returns early (text selection or link click) before calling storeCursorPositionData(), so the document.body.dataset.inplaceEditClickX/Y and inplaceEditCharOffset values from a previous activation can remain and be reused unintentionally. Clear the stored dataset values on these early-return paths (or move storeCursorPositionData() before the guards and have it explicitly reset state when activation is aborted).
| private setCursorPosition(element:HTMLElement):void { | ||
| const offset = parseInt(document.body.dataset.inplaceEditCharOffset ?? '', 10); | ||
| delete document.body.dataset.inplaceEditCharOffset; | ||
| if (!isNaN(offset)) { | ||
| // requestAnimationFrame ensures autofocus has run and the element is focused. | ||
| // setSelectionRange is not supported on all input types (e.g. number, date) — | ||
| // those will silently keep the browser's default cursor placement. | ||
| requestAnimationFrame(() => { | ||
| try { | ||
| (element as HTMLInputElement).setSelectionRange(offset, offset); | ||
| } catch { |
There was a problem hiding this comment.
setCursorPosition is only called with HTMLInputElement | HTMLTextAreaElement, but the method signature accepts HTMLElement and then type-asserts to HTMLInputElement to call setSelectionRange. Tighten the parameter type to HTMLInputElement | HTMLTextAreaElement (and avoid the cast) so TypeScript can enforce correct usage and autocomplete the proper API surface.
| export function caretRangeFromPoint(x:number, y:number):Range|null { | ||
| if ('caretPositionFromPoint' in document) { | ||
| const pos = document.caretPositionFromPoint(x, y); | ||
| if (pos) { | ||
| const range = document.createRange(); | ||
| range.setStart(pos.offsetNode, pos.offset); | ||
| range.collapse(true); | ||
| return range; | ||
| } | ||
| return null; | ||
| } | ||
| return (document as Document).caretRangeFromPoint?.(x, y) ?? null; | ||
| } |
There was a problem hiding this comment.
document.caretPositionFromPoint and document.caretRangeFromPoint are not part of the standard DOM typings in many TypeScript setups. As written, this will typically fail to compile (Property 'caretPositionFromPoint' does not exist on type 'Document', same for caretRangeFromPoint). Use a typed cast (e.g., const doc = document as { caretPositionFromPoint?: ...; caretRangeFromPoint?: ... }) or add a local/global type augmentation so the helper remains type-safe without any errors.
myabc
left a comment
myabc
left a comment
There was a problem hiding this comment.
Hi @HDinger,
I'm not yet done with the review, but some initial findings:
Functionality
I tried the solution with Safari, Firefox and Chrome. The solution seems to work OK across browsers in larger viewports:
https://github.com/user-attachments/assets/0b3feeaa-35b2-41b5-9ecc-54e95f6c7938
https://github.com/user-attachments/assets/ee6e9074-5db6-47b1-bdd7-609d6deb34d2
However it inserts the caret in the wrong place on smaller viewports (in these examples, I tried using the DevTools responsive mode):
https://github.com/user-attachments/assets/18899573-5c12-4b21-a170-2bdc3371f6b9
https://github.com/user-attachments/assets/0e041691-27e3-46da-a894-5c59f6cf2e4d
I'm not sure why this is - perhaps related to display and input text size differing on smaller viewports (which in itself, is somewhat jarring)
I think always inserting the caret at the beginning of the text (i.e. the buggy behaviour) is probably preferable to a solution where insertion is erratic.
Architecture-wise
I really don't like setting data like cursor position coordinates on document.body itself. However, if we do need to use document.body.dataset as some sort of temporary store, then I'd prefer to see an abstraction around it.
I'm still trying to get my head around the controller and its lifecycle, so apologies if the following alternate suggestions are naive or unfeasible:
- Can we not scope cursor co-ordinates to individual inplace-edit controllers (or at least a parent context)? Stimulus values would provide an idiomatic way of handling this. However if the controller is not disconnected after a Turbo request, this could also just be an instance variable on the controller?
- Another option would be to send the click coordinates with the HTTP request. Initially I thought this was a bit outlandish, but since we are round-tripping to the server, passing request variables that then then set metadata on the rendered CKEditor might work.
|
Closing in favour of #23449 |
Ticket
https://community.openproject.org/wp/72837
What are you trying to accomplish?
Place cursor at click position when activating inplace edit fields. This works well for normal (text) inputs. Date and number inputs unfortunately don't support the
setSelectionRangemethod, so there was not much I could do for them.The CkEditor is a bit special. This PR attempts to also remember the click position for this field, but since the formatting between the two views differs so heavily it is basically impossible to set it to the exact same spot.
How it works
On click (before the edit field is fetched via Turbo Stream) the click position is captured in two fields on document.body.dataset:
inplaceEditClickX/Y) — for CKEditor, where the content layout differs between display and editorinplaceEditCharOffset) — computed viacaretRangeFromPointon the display field's text nodes, for plain text inputs where the content is identicalAfter the edit field renders: