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

Feature: Proper Handling of Deletions Across Adjacent Unmergeable Text Spans in Grid Containers #7301

Open
birtles opened this issue Mar 6, 2025 · 7 comments · May be fixed by #7317
Open
Assignees
Labels
enhancement Improvement over existing feature

Comments

@birtles
Copy link
Contributor

birtles commented Mar 6, 2025

Description

When deleting across adjacent unmergeable text spans inside a display: inline-grid (or presumably display: grid) container, Lexical does not properly extend the selection into the next span, causing deletion to get stuck.

Previously, it was possible to work around this issue by manually extending the selection when deleting across boundaries (see span-deletion.ts), but this workaround no longer functions in Lexical 0.27.0+ due to a change in selection syncing (related change).

The expected behavior is for RangeSelection.modify to correctly move from the edge of one text node to the next before applying the native selection change, similar to $modifySelectionAroundDecoratorsAndBlocks.

Steps to Reproduce

A minimal reproduction can be found here: StackBlitz.

  1. Position the caret between "A" and "B".
  2. Press Del to delete "B".
  3. Press Del again.

Expected Results

  • Both "B" and "C" should be deleted sequentially.

Actual Results

  • Deletion gets stuck either at "B" or at "C" (it depends on if the caret initially lies at the start of the "B" span or the end of the "A" span).

This only occurs when using display: inline-grid. Disabling inline-grid allows deletion to function normally.

Use cases

  • Any applications requiring adjacent text nodes inside a container like display: grid (ruby text editing in our case)

Alternative Solutions Considered

  • Manually adjusting the selection before deletion (which worked in 0.26 but no longer works in 0.27 due to the updated selection syncing behavior).
  • Finding an alternative layout approach that avoids using grid etc. (In our specific case, browser support for ruby layout has come a long way since we started but still only Firefox supports the necessary HTML tags to render mono ruby without interleaving the transcription nodes.)

Impact

  • This affects developers working with custom nodes that put multiple adjacent (unmergeable) text nodes inside certain containers (at least display: inline-grid but possibly others like display: flex too).
  • The issue prevents deletion across text spans, breaking expected editor behavior.
  • Fixing this would improve selection handling and deletion reliability in specialized text-editing scenarios.

(As discussed with @etrepum on Discord)

@birtles birtles added the enhancement Improvement over existing feature label Mar 6, 2025
@etrepum etrepum self-assigned this Mar 11, 2025
@etrepum
Copy link
Collaborator

etrepum commented Mar 12, 2025

@birtles if you take a look at #7317 I think all of these issues are sorted in a way where you can delete your workarounds for delete and arrow keys, but I need you to verify that it does work as expected for your use case before I go through the trouble of writing e2e tests, debugging the existing ones, and getting it reviewed/merged with this new behavior.

@birtles
Copy link
Contributor Author

birtles commented Mar 13, 2025

@etrepum That looks awesome. Thank you so much. I tried it out now and confirmed that I can drop both the deletion and navigation workarounds in at least Firefox and Chrome.

However, it's not working for me in Webkit. Webkit often throws the selection outside the container with content like this so we have workarounds to detect that and restore it.

Also, I've got about a dozen newly failing E2E tests with this change which I need to dig into. It's possible they're all just cases of code that needs to be updated (ideally deleted) to reflect the new behavior but I need to go through each one to just ensure they're not unintended changes in behavior.

@etrepum
Copy link
Collaborator

etrepum commented Mar 13, 2025

Great, I’ll look a bit deeper. Didn’t do any WebKit testing or look at any of the e2e failures, but since you confirmed that this approach sorts out your navigation issues I’ll spend some more time with it

@birtles
Copy link
Contributor Author

birtles commented Mar 15, 2025

@etrepum I dug into some of the new failures I was encountering with #7317 and at least some of them seem to be coming from the changes to range-selection.ts.

We embed inline contenteditable=false nodes in our content to cause the caret to stop twice in some places so that the user can position the cursor inside or outside a container (think inline code snippets in Slack, for example).

This video explains it better: https://x.com/brianskold/status/1649336444467699712

A very simplified version of the structure is something like this:

<div contenteditable>
  abc<span contenteditable=false>&#x200B;</span>def
</div>

(The zero-width space is needed for Chrome but Firefox and Webkit work fine without it.)

However, it seems like with this change, if we are extending a selection (i.e. non-collapsed selection where we've moving the focus) we return true from $shouldOverrideDefaultCharacterSelection where we previously returned false and end up putting the focus on the contenteditable=false node when we previously didn't.

The actual structure we use is more complex than that (we have several nested containers) so the markup above might not be enough to reproduce the issue but it's going to take more time to produce a minimal reproduction.

@etrepum
Copy link
Collaborator

etrepum commented Mar 15, 2025

I'd need to know exactly what the corresponding lexical document looks like, the HTML is only half the story.

With regard to controlling the cursor location a better solution than zero width nodes would probably be to manually control the selection in the areas where you know that needs to happen (by moving to an element point from a text point or vice versa at those specific boundaries)

@birtles
Copy link
Contributor Author

birtles commented Mar 17, 2025

I'd need to know exactly what the corresponding lexical document looks like, the HTML is only half the story.

Yes, let me see what I can do.

With regard to controlling the cursor location a better solution than zero width nodes would probably be to manually control the selection in the areas where you know that needs to happen (by moving to an element point from a text point or vice versa at those specific boundaries)

Ok, fair enough. I was hoping to get away without writing more JS for this since all browser's seem to provide the desired behavior natively, but if doing that isn't going to play well with Lexical I guess we should use JS.

@etrepum
Copy link
Collaborator

etrepum commented Mar 17, 2025

You're writing JS one way or the other, whether it's to create and manage artificial DOM and/or lexical nodes that don't have any semantic meaning or to explicitly handle the selection. The latter makes more sense in the general case because it behaves the same way on all platforms and it's usually less code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement over existing feature
Projects
None yet
2 participants