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

Bug Fix: Fix issue where triple-clicking a cell would dangerously select entire document #6542

Merged
merged 1 commit into from
Aug 22, 2024

Conversation

moughxyz
Copy link
Contributor

@moughxyz moughxyz commented Aug 22, 2024

Description

Triple-clicking may seem esoteric at first but it's a relatively common interaction for quickly selecting whole blocks. For example, triple clicking a paragraph selects the entire paragraph (at least on macOS).

We were seeing a strange issue for some of our users: their entire document would go missing. When we worked with some of these users, we found that previous revisions of their doc showed that it consisted of only a single large table.

After it happened to one of our team members, we found that it was very easy, particularly on mobile, to accidentally select the entire table while trying to tap a single cell to edit. It's my theory that when tapping very rapidly on mobile, a triple-click event gets processed.

Nonetheless, it's easy to replicate this issue on desktop in the playground:

  1. Create a table of any size
  2. Write some text in the last cell
  3. Triple click on the text in the last cell

Result: entire document get highlighted. One keystroke at this point is all it would take to wipe your entire document.

Expected result: only the text in the cell gets highlighted.

In my opinion the code responsible seems relatively dangerous: reaching out to the parent to grab some more nodes for selection. And in fact the issue that the original code was added to address does not seem to be affected by the removal of this code, as evidenced by the passing of the test added in that commit: 2b2a4f6

It's also my opinion that for large open source projects like this, tests should be the ultimate way of determining what stays and what goes. At some point there will no longer be anyone involved in the project who has all the historical context necessary to determine whether a change is harmful or beneficial, and its up to the tests to defend each change.

In this case, the removal of this code does not harm anyones tests, and improve the usecase mentioned above.

Test plan

A test was added that fails without the fix. Without the fix, the test would fail because the entire document would be highlighted.

Before

before.mov

After

after.mov

Copy link

vercel bot commented Aug 22, 2024

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 Aug 22, 2024 5:40pm
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 22, 2024 5:40pm

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 22, 2024
Copy link

size-limit report 📦

Path Size
lexical - cjs 29.38 KB (0%)
lexical - esm 29.24 KB (0%)
@lexical/rich-text - cjs 37.74 KB (0%)
@lexical/rich-text - esm 31.05 KB (0%)
@lexical/plain-text - cjs 36.41 KB (0%)
@lexical/plain-text - esm 28.42 KB (0%)
@lexical/react - cjs 39.64 KB (0%)
@lexical/react - esm 32.51 KB (0%)

@moughxyz moughxyz changed the title Bug Fix: Fix issue where triple-clicking a cell would dangerously sel… Bug Fix: Fix issue where triple-clicking a cell would dangerously select entire document Aug 22, 2024
Copy link
Member

@zurfyx zurfyx left a comment

Choose a reason for hiding this comment

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

Thank you, this logic was introduced just recently #5766, @ivailop7 @AlexanderReznik @Shubhankerism do you have some thoughts here?

@ivailop7
Copy link
Collaborator

Thank you, this logic was introduced just recently #5766, @ivailop7 @AlexanderReznik @Shubhankerism do you have some thoughts here?

I'm OK with this change

@ivailop7 ivailop7 added the extended-tests Run extended e2e tests on a PR label Aug 22, 2024
@ivailop7 ivailop7 added this pull request to the merge queue Aug 22, 2024
Merged via the queue into facebook:main with commit 4b646a1 Aug 22, 2024
37 of 46 checks passed
@AlexanderReznik
Copy link
Contributor

This has broken the selection when started inside of the table on the edge (1st or last row) that goes outside.
table-edge-selection-bug
You can see on the gif that the lexical selection is not matching the dom selection and not the whole table is selected.

@moughxyz
Copy link
Contributor Author

Honestly it's not unexpected that there wouldn't be some side-effects. This is a symptom of the problem of e2e tests not being required for every PR here.

I don't know whether I'll be able to look into this but I hope that whatever fix is implemented is grounded by a test.

@moughxyz moughxyz deleted the table-selection-fix branch August 29, 2024 17:07
@AlexanderReznik
Copy link
Contributor

Honestly it's not unexpected that there wouldn't be some side-effects. This is a symptom of the problem of e2e tests not being required for every PR here.

While it would be of course ideal to have 100% code coverage that tests every possible use case I'd say it's quite unrealistic to expect from an open source project of this scale. And given that there isn't a 100% test coverage just relying on the fact that the tests are passing is not enough.

Please next time you contribute spend some time to understand what the code actually does.

https://sproutsschools.com/chesterton-fence-dont-destroy-what-you-dont-understand/

@moughxyz
Copy link
Contributor Author

I'd say this is the likely fate of any large open source project: https://news.ycombinator.com/item?id=18442941

Ultimately it's up to the maintainers to accept/reject changes. For changes outside a particular reviewer's area of familiarity/expertise, I honestly can't imagine how they can do their job properly other than basing it off tests. The alternative is that PRs would never get merged from fear and you'd have a stagnate project. This codebase is well written but deals with a domain that is just too complex and vast for any one person to understand.

I've opened a PR that fixes the regression: #6579

@etrepum
Copy link
Collaborator

etrepum commented Aug 30, 2024

I think the problem here is more that there's a lack of specification. With or without tests, there's no clear description of what should be happening in a lot of these complicated cases where you have many different components interacting. The tests lock in a sort of specification but without a clear description of intent it could just be a "snapshot" test of some implementation quirk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. extended-tests Run extended e2e tests on a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants