search

facebook / lexical

Lexical is an extensible text editor framework that provides excellent reliability, accessibility and performance.
https://lexical.dev
MIT License
17.5k stars 1.45k forks source link

Improve table selection and navigation #5767

Closed serey-roth closed 1 month ago

serey-roth commented 1 month ago

This PR addresses several issues related to table selection and navigation within the editor. The following changes have been made:

https://www.loom.com/share/f16a323757294da7b767ba870fa18421?sid=e7ee42d6-2e59-4ffb-87db-00d8fed838d8

https://www.loom.com/share/b461840c78c443d4816cbb94cad938f7?sid=aa3406b5-b50b-4071-b1ef-b235e57e9154

https://www.loom.com/share/107d1bcac82f497085231858235fe3f3?sid=8dfc4d27-68af-465c-87e6-1e274e48e2bb

https://www.loom.com/share/454f7f63c2b34a3a838c3b0e6ccc6ca4?sid=0b00f765-360c-438e-a8b0-0b47f78ce4d9

https://www.loom.com/share/fed0547e1e894953811a70648c0c3751?sid=b75d789b-db7d-4ddf-a62f-331488bcdb0c

Updated 03/27/2024:

https://www.loom.com/share/9eedbba40dae4dc4b57090bf9404d0c9?sid=784bd357-fdc9-4206-895e-365714c65680

https://www.loom.com/share/e179a5c87235409fa8d1945636e5ba03?sid=009ab1ed-70b2-47d0-8cf4-891ccf26e62e

vercel[bot] commented 1 month ago

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 Mar 31, 2024 8:04pm
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 31, 2024 8:04pm
ivailop7 commented 1 month ago

This looks VERY VERY good, there's still a small bug related to what you are fixing, if you put your cursor on the line after the table, when you press Enter, it goes to the next row correctly now, but if you type 'abc' it doesn't go to the line below, but to the last cell in the table and puts the 'abc' in there.

serey-roth commented 1 month ago

This looks VERY VERY good, there's still a small bug related to what you are fixing, if you put your cursor on the line after the table, when you press Enter, it goes to the next row correctly now, but if you type 'abc' it doesn't go to the line below, but to the last cell in the table and puts the 'abc' in there.

This happens because the selection is still in the last cell of the table and the table is the last node. In this scenario, should we create a new paragraph and insert the text there? I think this is the most sensible solution since the cursor is technically outside. @ivailop7 What do you think?

ivailop7 commented 1 month ago

This looks VERY VERY good, there's still a small bug related to what you are fixing, if you put your cursor on the line after the table, when you press Enter, it goes to the next row correctly now, but if you type 'abc' it doesn't go to the line below, but to the last cell in the table and puts the 'abc' in there.

This happens because the selection is still in the last cell of the table and the table is the last node. In this scenario, should we create a new paragraph and insert the text there? I think this is the most sensible solution since the cursor is technically outside. @ivailop7 What do you think?

Yes, I think this is the most sensible approach. Looking at how other editors behave for this, they don't even allow the cursor to be on the same line as the table, but move it to a paragraph below the table directly when you click after the row after the table. So let's go with the same. Add a paragraph below and type in there.

serey-roth commented 1 month ago

This looks VERY VERY good, there's still a small bug related to what you are fixing, if you put your cursor on the line after the table, when you press Enter, it goes to the next row correctly now, but if you type 'abc' it doesn't go to the line below, but to the last cell in the table and puts the 'abc' in there.

This happens because the selection is still in the last cell of the table and the table is the last node. In this scenario, should we create a new paragraph and insert the text there? I think this is the most sensible solution since the cursor is technically outside. @ivailop7 What do you think?

Yes, I think this is the most sensible approach. Looking at how other editors behave for this, they don't even allow the cursor to be on the same line as the table, but move it to a paragraph below the table directly when you click after the row after the table. So let's go with the same. Add a paragraph below and type in there.

@ivailop7 Currently, the cursor will be on the same line as the table if you click on the table's right edge. Do we want to replace this as well? In that case, clicking outside the table, regardless of the click position, will move the selection to the next line.

ivailop7 commented 1 month ago

Let's keep it for now next to the table and upon typing to append the paragraph below and have the text there. If we need to change it later, should be easy.

serey-roth commented 1 month ago

Let's keep it for now next to the table and upon typing to append the paragraph below and have the text there. If we need to change it later, should be easy.

@ivailop7 I've added this, but it only works when the table doesn't have a nested table as the last child in the last cell. I've noticed it's because of the selection; the cursor will still be on the last cell of the nested table when it appears to be on the same line as the outer table (see video below). The same thing happens when pressing the "Enter" key.

https://www.loom.com/share/3c655aead08e4286a7d3f6646a3c2498?sid=aadcfb4e-897f-4c8c-9d83-0d4c5e6276df

ivailop7 commented 1 month ago

Let's keep it for now next to the table and upon typing to append the paragraph below and have the text there. If we need to change it later, should be easy.

@ivailop7 I've added this, but it only works when the table doesn't have a nested table as the last child in the last cell. I've noticed it's because of the selection; the cursor will still be on the last cell of the nested table when it appears to be on the same line as the outer table (see video below). The same thing happens when pressing the "Enter" key.

https://www.loom.com/share/3c655aead08e4286a7d3f6646a3c2498?sid=aadcfb4e-897f-4c8c-9d83-0d4c5e6276df

The nested tables strike again... to be honest I'm fine with this limitation, since it's still miles better than what we had before. Random speculation, but maybe a check if the parent node is TableCellNode could be a way to become aware of the nested tables and behave accordingly. Either way, happy with the direction!

ivailop7 commented 1 month ago

Once you are happy with it, would you mind adding tests :)

serey-roth commented 1 month ago

Once you are happy with it, would you mind adding tests :)

Will do! As you noticed, I didn't include the ability to insert a new paragraph by clicking under the last row of the table. I'll tackle this on later, but I'm thinking it should only work for the outermost table? @ivailop7 What do you think?

ivailop7 commented 1 month ago

Doing this after this PR should be fine! 👍🏻

ivailop7 commented 1 month ago

@serey-roth one of your own tests is failing, once ready, will get the others to have a look at the PR too.

serey-roth commented 1 month ago

@serey-roth one of your own tests is failing, once ready, will get the others to have a look at the PR too.

The tests are failing in collab mode. I've noticed that no paragraphs are inserted along with the table, which ends up changing the selection path. @ivailop7 Is this the correct behavior?

ivailop7 commented 1 month ago

@serey-roth one of your own tests is failing, once ready, will get the others to have a look at the PR too.

The tests are failing in collab mode. I've noticed that no paragraphs are inserted along with the table, which ends up changing the selection path. @ivailop7 Is this the correct behavior?

Not sure I get this one. Is this an issue that happens only when in collab, but fine in standalone? When you say no paragraph are inserted with the table, do you mean, a paragraph after the table upon 'table insert' or something else?

serey-roth commented 1 month ago

@serey-roth one of your own tests is failing, once ready, will get the others to have a look at the PR too.

The tests are failing in collab mode. I've noticed that no paragraphs are inserted along with the table, which ends up changing the selection path. @ivailop7 Is this the correct behavior?

Not sure I get this one. Is this an issue that happens only when in collab, but fine in standalone? When you say no paragraph are inserted with the table, do you mean, a paragraph after the table upon 'table insert' or something else?

@ivailop7 It only happens in collab, and only when you insert the table via the dropdown after the initial load. Currently, it looks like we don't automatically insert a paragraph after editor loads. This causes the selection to be null. (See videos below)

https://www.loom.com/share/6990dd65bb4c4f83876444ebf23e7f8e

https://www.loom.com/share/bf61832280834c8799e63887c8231573?sid=f7ffca54-d2da-4b29-afa2-ec5a3d55b404

ivailop7 commented 1 month ago

@serey-roth one of your own tests is failing, once ready, will get the others to have a look at the PR too.

The tests are failing in collab mode. I've noticed that no paragraphs are inserted along with the table, which ends up changing the selection path. @ivailop7 Is this the correct behavior?

Not sure I get this one. Is this an issue that happens only when in collab, but fine in standalone? When you say no paragraph are inserted with the table, do you mean, a paragraph after the table upon 'table insert' or something else?

@ivailop7 It only happens in collab, and only when you insert the table via the dropdown after the initial load. Currently, it looks like we don't automatically insert a paragraph after editor loads. This causes the selection to be null. (See videos below)

https://www.loom.com/share/6990dd65bb4c4f83876444ebf23e7f8e

https://www.loom.com/share/bf61832280834c8799e63887c8231573?sid=f7ffca54-d2da-4b29-afa2-ec5a3d55b404

This is interesting, didn't realize the difference in behaviour that the default paragraph is not inserted when in collab, which makes sense. I do still find the failures strange, since other test would be affected. In this case, try just insert a paragraph after focusing the editor so that you never have an editor without a paragraph node, if you don't get them to work do a test.skip(isPlainText || isCollab); as a last resort. Nonetheless, great work here!

ivailop7 commented 1 month ago

Also, don't forget to sort the imports for integrity to pass as well

ivailop7 commented 1 month ago

@serey-roth I pushed a commit to your branch to help with the tests, let me know when you want to do more changes or you are done with this PR, so I can give this a proper read.

serey-roth commented 1 month ago

@serey-roth I pushed a commit to your branch to help with the tests, let me know when you want to do more changes or you are done with this PR, so I can give this a proper read.

@ivailop7 Thanks for taking care of the failing checks! I rewrote all my tests last night, but it looks like they weren't exactly correct. I'm happy with the current changes so please review them and let me know if there are anything I need to adjust.

Out of curiosity, how did you handle the missing paragraph in the collab editor?

ivailop7 commented 1 month ago

@serey-roth I pushed a commit to your branch to help with the tests, let me know when you want to do more changes or you are done with this PR, so I can give this a proper read.

@ivailop7 Thanks for taking care of the failing checks! I rewrote all my tests last night, but it looks like they weren't exactly correct. I'm happy with the current changes so please review them and let me know if there are anything I need to adjust.

Out of curiosity, how did you handle the missing paragraph in the collab editor?

Nothing really, removed all the extra code, I found it suspicious in a first place, since all other table tests should fail by the same logic and they didn't. I only added 'undefined' in the assertHTML methods, to not compare the left editor state with the right editor state, as it was done for the other table tests and things worked. I'll start reviewing now.