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

Have plugin handle scrollToSelection events when they occur within elements #397

Merged
merged 1 commit into from
May 9, 2024

Conversation

jonathonherbert
Copy link
Contributor

@jonathonherbert jonathonherbert commented May 8, 2024

co-authored-by: @rhystmills - who is responsible for anything untrue in the following PR description:

What does this change?

Currently, some inner editors (e.g. one of our text fields), hand responsibility for certain commands to the outer editor - notably the nestedElementField, which passes events to the outer editor so that it can handle undo, formatting, and other commands. As part of these commands, the outer editor calls scrollIntoView - which has led to a bug.

This behaviour hasn't caused problems until now - the only pre-existing fields that have formatting options have their own menu plugins (e.g. the caption in image elements), so don't need to hand control to the outer editor. There was a PR to specifically handle this problem for history, which did have a similar problem. The problem is also much more obvious in our list elements because they can potentially be very long, so skipping to the 'top' of the element is very jarring for users.

This PR marks scrollIntoView as being handled by the prosemirror-elements plugin. We're not actually handling those events - we're just telling the outer editor that they're handled, then discarding them, so that the outer editor doesn't attempt to do so.

We're keen to get this mitigation out quickly because the bug is degrading the experience significantly in our list formats.

It would be good, in the near future, to:

  1. Get tests out soon after this PR is merged, to cover this behaviour.
  2. Handle the scrollToSelection behaviour properly, i.e. making sure the specific field within the nestedElement is scrolled to when a user makes changes - this will make it clearer what changes are being made when, e.g, a user uses undo or redo within prosemirror-elements. State changes to nodes are the only thing the inner editors see (i.e. they don't see transactions). We may want to set special markup on nodes that the inner editor can react to - we do something similar for selections at the moment.

How to test

Use in Composer locally via yalc and test usual behaviour.

@rhystmills rhystmills marked this pull request as ready for review May 9, 2024 08:28
@rhystmills rhystmills requested a review from a team as a code owner May 9, 2024 08:28
Copy link
Contributor

@simonbyford simonbyford left a comment

Choose a reason for hiding this comment

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

Have tested this locally with composer and everything behaves as stated in the PR, great work @rhystmills and @jonathonherbert!

@jonathonherbert
Copy link
Contributor Author

Brilliant description @rhystmills

@rhystmills rhystmills merged commit adffca1 into main May 9, 2024
3 checks passed
@rhystmills rhystmills deleted the rm-js/handle-scroll-to-selection branch May 9, 2024 09:56
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

3 participants