Skip to content
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

propagate key down & press events upwards from NestedElementFieldView up to the top-level editor so key mappings work as expected #363

Merged
merged 4 commits into from
Mar 18, 2024

Conversation

twrichards
Copy link
Contributor

@twrichards twrichards commented Mar 11, 2024

https://trello.com/c/Z3K0WT7D/2160-fix-shortcuts-eg-cmd-z-not-working-in-key-takeaways-elements

It has been noticed that none of the key mappings (e.g. undo, redo etc.) have been working inside Key Takeaways. This makes sense since key-mappings are a plugin and we know from the exploration which led to #362 that events don't make it up to the top-level editor by default.

IMPORTANT: in #362 the click events were being forwarded up from ProseMirrorFieldView but doing the same for key events broke some tests for RTE, so moved the click event forwarding in #362 down to apply only to NestedElementFieldView

NOTE: the cypress test for undo/redo in the inner editors has been marked as 'skipped' since it wasn't working in CI (but was locally) and it has been manually tested - and we need to release this really.

What does this change?

This PR follows the pattern established in #362 to forward 'key down' events up to the top level editor so the key mapping plugin can do its thing. There is one notably exception the Enter key, as the line breaks were being misplaced.

How to test

Yalc it into composer local and add text in bursts, alternating between the outer editor and an inner editor (e.g. key takeaways) then Cmd-Z (to undo) a few times and see the undos applied in the correct order (unlike before).

How can we measure success?

Users experience is as before (muscle memory) is important here.

@twrichards twrichards force-pushed the fix-key-mappings-from-inner-editors branch from f066214 to 3300396 Compare March 11, 2024 15:25
@twrichards twrichards marked this pull request as ready for review March 11, 2024 15:27
@twrichards twrichards requested a review from a team as a code owner March 11, 2024 15:27
@twrichards
Copy link
Contributor Author

@jonathonherbert can you think of any issues with this approach? and do you think we'd be better allow list the keys allowed to travel up to the outer editor

@twrichards twrichards force-pushed the fix-key-mappings-from-inner-editors branch 3 times, most recently from 720de40 to f551953 Compare March 12, 2024 13:00
Copy link
Contributor

@rhystmills rhystmills left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working nicely in Composer when testing locally and not noticing anything misbehaving. Shortcut for other plugins (e.g. noting) are working, as are undo/redo and markup shortcuts (e.g. blockquote).

Seems to be an integration test failing but otherwise looks good.

@jonathonherbert
Copy link
Contributor

@jonathonherbert can you think of any issues with this approach? and do you think we'd be better allow list the keys allowed to travel up to the outer editor

If everything's working as expected, this seems reasonable to me! I can't think of a problem with propagating keydown generally – I'd be interested to know more about the journey these events take from creation to model update w/in the context of contenteditable.

(My understanding is that edits to contenteditable fields are generally 'allowed' by Prosemirror, with some exceptions for browser-specific behaviour (where PM corrects after the fact). It's been a while since I looked at the code though.)

@davidfurey
Copy link
Member

Are there any other events that we should forward while we are at it? https://github.com/ProseMirror/prosemirror-view/blob/5704aece3fd350f2ccfa04cfc7e861e9a38627c4/src/index.ts#L616

For example, do mobile keyboards send a keydown event or just a keypress event?

@twrichards twrichards force-pushed the fix-key-mappings-from-inner-editors branch from f551953 to da964cc Compare March 13, 2024 11:27
@twrichards twrichards mentioned this pull request Mar 13, 2024
@twrichards twrichards changed the base branch from fix-click-events-from-inner-editors to upgrade/cypress March 13, 2024 11:40
@twrichards twrichards changed the title propagate key down events upwards from inner editors (e.g. NestedElementFieldView) up to the top-level editor so key mappings work as expected propagate key down & press events upwards from inner editors (e.g. NestedElementFieldView) up to the top-level editor so key mappings work as expected Mar 13, 2024
@twrichards
Copy link
Contributor Author

twrichards commented Mar 13, 2024

Are there any other events that we should forward while we are at it? ProseMirror/prosemirror-view@5704aec/src/index.ts#L616

For example, do mobile keyboards send a keydown event or just a keypress event?

Good point @davidfurey - thank you. I've added in 95d0c05

@twrichards twrichards force-pushed the fix-key-mappings-from-inner-editors branch from 529cfbc to 95d0c05 Compare March 13, 2024 11:48
@twrichards twrichards force-pushed the fix-key-mappings-from-inner-editors branch from 95d0c05 to ceac245 Compare March 13, 2024 21:30
@twrichards twrichards changed the base branch from upgrade/cypress to fix-click-events-from-inner-editors March 14, 2024 15:02
@twrichards twrichards force-pushed the fix-key-mappings-from-inner-editors branch from ceac245 to fae7ab6 Compare March 14, 2024 15:02
Base automatically changed from fix-click-events-from-inner-editors to main March 14, 2024 15:12
… (e.g. `NestedElementFieldView`) up to the top-level editor so key mappings work (e.g. undo, redo)
…s (e.g. `NestedElementFieldView`) up to the top-level editor just like we do for key down
@twrichards twrichards force-pushed the fix-key-mappings-from-inner-editors branch 4 times, most recently from df183c9 to 0b276bc Compare March 14, 2024 15:56
@twrichards twrichards changed the title propagate key down & press events upwards from inner editors (e.g. NestedElementFieldView) up to the top-level editor so key mappings work as expected propagate key down & press events upwards from NestedElementFieldView up to the top-level editor so key mappings work as expected Mar 14, 2024
…iew` as it was breaking other inner editors such RTE used in captions
… CI and need to release it (it works when tested manually)
@twrichards twrichards force-pushed the fix-key-mappings-from-inner-editors branch from 0b276bc to 47229b0 Compare March 15, 2024 12:27
@twrichards twrichards requested review from rhystmills and a team March 15, 2024 12:40
Copy link
Contributor

@jonathonherbert jonathonherbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Untested, but code LGTM, approving to unblock.

@twrichards twrichards merged commit 9b7fee0 into main Mar 18, 2024
3 checks passed
@twrichards twrichards deleted the fix-key-mappings-from-inner-editors branch March 18, 2024 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants