[lexical-offset] Fix: @lexical/offset makes invalid selection

[wataru-chocola]

Description

Fixed anchor / focus offset of the selection created by OffsetView.createSelectionFromOffsets.

Closes #7580

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
lexical	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 30, 2025 1:31am
lexical-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 30, 2025 1:31am

[etrepum]

A test to show that this solves the issue would be great

[etrepum]

Suggested change

	startOffset = parent.getChildrenKeys().indexOf(startNode.getKey());
	startOffset = startNode.getIndexWithinParent();

[etrepum]

Suggested change

	endOffset = parent.getChildrenKeys().indexOf(endNode.getKey());
	endOffset = endNode.getIndexWithinParent();

[wataru-chocola]

@etrepum

Thank you for your review!

• Added a test case
• Updated code

[etrepum]

Can someone else take a close look at this one? I don't see any usage of @lexical/offset except in collab and I'm not sure what the existing test coverage from there looks like if there was really anything /cc @james-atticus @amanharwara

[james-atticus]

With respect to collab specifically, only $createChildrenArray is used which is a simple function that's not affected by this change. It's not doing anything specific to offsets and could probably live in core lexical.

Looking at the original PR though, I'm pretty sure these start/end values are intended to be absolute offsets rather than offsets relative to the parent -- state.offset (where start/end come from) is only ever increased by 1 or the length of the text. This is pretty similar to ProseMirror's Indexing, except that it doesn't count entering/leaving a block, which makes sense given that offsets were previously used for selection restoration in collab.

[etrepum]

After having a closer look myself, it looks like @james-atticus is right and that this probably isn't the correct fix. I think if we are going to fix anything in this module we should have a more comprehensive test suite that shows that it handles the diffing and offset clamping as expected in more than just this one case, and include the behavior when nodes are nested (e.g. an inline element node is not covered by this test case)

[wataru-chocola]

Thank you for reviewing, but I don't quite understand comments.

I don't think this PR changed anything related to absolute offsets.
OffsetView.createSelectionFromOffsets converts absolute start / end offsets to selection.
This PR fixed offset calculation of selection, which is inherently relative within anchor / focus node.

Do I misunderstand something?

[etrepum]

Possibly not, I did not give it a very thorough audit because I would have to carefully read all of the code in context to make sure it is correct. There just aren’t enough tests, and absolutely no usage in the playground or examples, to give me any confidence that this module does what it’s supposed to do before or after this change.

[james-atticus]

My mistake, @wataru-chocola is correct. I was assuming that the original behaviour (returning absolute offset) was intentional, but that's incorrect. For the text case:

    if (startNode.type === 'text') {
      startOffset = start - startNode.start;
      startType = 'text';

start and startNode.start are both absolute indexes, so startOffset ends up being relative to the text node. This PR is doing the same for inline nodes.

The part I'm still unsure on though is whether it's correct to drop the end > startOffsetNode.start check. Going back to the original PR, the check looks to be there to handle some behaviour change based on isCollapsed. Haven't got my head around exactly what it's meant to do though. As @etrepum says, hard to know what the original intent was without tests or playground usage.