You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Quickfix for #6515
As described in the issue currently changing the selection using the arrow keys can throw an error that crashes the editor.
(Error: Uncaught Error: Node at cords not TableCellNode.)
This quickfix works quite well for merged cells at the end of a row (most common cases imo)
For merged cells at the beginning or the middle of a row this simple approach creates some unexpected cursor jumps. But instead of crashing the user can be easily adapt the position and the editor won't break.
(For a full fix the positions would need to consider the colspan everywhere in the table plugin, that would be a bigger refactoring I guess).
I did check where this function is used and did some manual testing and did not encounter unwanted side effects, but this is also my first glance inside the lexical codebase :)
Fingers crossed.
@elbotho thanks for helping out on this. I think the last surface when it comes to table bugs is 'merged cells selection'. If you are motivated to help out clear those issues (all tagged with 'tables') nicely with a better refactor, it would be very welcome.
@elbotho thanks for helping out on this. I think the last surface when it comes to table bugs is 'merged cells selection'. If you are motivated to help out clear those issues (all tagged with 'tables') nicely with a better refactor, it would be very welcome.
Is this a no to the quickfix?
I see 12 issues with the table tag and around 4-5 seem to relate directly to merged cells and or selection. Are you refering to those?
@elbotho thanks for helping out on this. I think the last surface when it comes to table bugs is 'merged cells selection'. If you are motivated to help out clear those issues (all tagged with 'tables') nicely with a better refactor, it would be very welcome.
Is this a no to the quickfix?
I see 12 issues with the table tag and around 4-5 seem to relate directly to merged cells and or selection. Are you refering to those?
I'm OK to merge this PR, despite the lack of tests, since it doesn't seem to make things worse, but it would be nice to have a proper fix, even if it takes longer. Yes, I'm refering to those 4-5 issues that are all the same issue essentially.
CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.extended-testsRun extended e2e tests on a PR
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
facebook-github-bot
added
the
CLA Signed
elbotho
requested review from
zurfyx,
fantactuka,
acywatson,
Fetz,
ivailop7,
Sahejkm and
potatowagon
as code owners
Description
Quickfix for #6515
As described in the issue currently changing the selection using the arrow keys can throw an error that crashes the editor.
(Error:
Uncaught Error: Node at cords not TableCellNode.)Describe the changes in this pull request
This quickfix works quite well for merged cells at the end of a row (most common cases imo)
For merged cells at the beginning or the middle of a row this simple approach creates some unexpected cursor jumps. But instead of crashing the user can be easily adapt the position and the editor won't break.
(For a full fix the positions would need to consider the colspan everywhere in the table plugin, that would be a bigger refactoring I guess).
I did check where this function is used and did some manual testing and did not encounter unwanted side effects, but this is also my first glance inside the lexical codebase :)
Fingers crossed.
Test plan
Before
After