[lexical-list] Bug Fix: stop throttling rapid mouse clicks on the same checklist checkbox - #9102
Merged
etrepum merged 4 commits intoAug 28, 2026
Merged
Conversation
…e checklist checkbox ## Description The checklist dedup mechanism (introduced in facebook#8390 to prevent mobile double-toggle from pointerup + synthesized click) was applied symmetrically: both `configHandleClick` and \configHandlePointerUp` checked the dedup window AND recorded a timestamp. As a side effect, rapid mouse clicks on the **same** desktop checkbox were throttled to one toggle per 500ms — the second click within the window was silently dropped. The dedup only needs to be **unidirectional**: a synthesized mobile `click` must be absorbed when it follows a touch `pointerup` for the same tap. Mouse clicks never have a preceding touch pointerup, so they should never be throttled. This change makes the dedup write-path touch-only: - `configHandlePointerUp` still checks + records (mobile throttle preserved, including spam-tap protection). - `configHandleClick` still checks (absorbs synthesized mobile clicks) but no longer records a timestamp, so rapid desktop clicks on the same checkbox all toggle. The internal field is renamed `__lexicalCheckListLastHandled` → `__lexicalCheckListLastTouchHandled` to reflect that it is now written exclusively by the touch path. Mobile behavior is unchanged: spam-tap on the same checkbox remains throttled at 500ms, and the pointerup + click double-toggle guard still works. ## Test plan ### Before ``` $ npx vitest run --project unit packages/lexical-list/src/__tests__/unit/checkList.test.tsx ``` No test covered rapid desktop clicks on the same item; the throttle was an undocumented side effect, so a second mouse click within 500ms was silently dropped (the checkbox stayed in its post-first-click state). ### After ``` $ npx vitest run --project unit packages/lexical-list/src/__tests__/unit/checkList.test.tsx ✓ unit packages/lexical-list/src/__tests__/unit/checkList.test.tsx (6 tests) 104ms Test Files 1 passed (1) Tests 6 passed (6) ``` Two new tests lock in the behavior: - "rapid mouse clicks on the same item all toggle (desktop not throttled)" — two mouse clicks within the dedup window produce false → true → false. - "rapid touch taps on the same item are throttled (mobile preserved)" — two touch pointerups within the dedup window produce a single toggle (false → true), confirming the mobile throttle is unchanged. Lint, tsc, and prettier are clean on the affected package.
noeschmidt
requested review from
acywatson,
etrepum,
fantactuka,
ivailop7,
potatowagon and
zurfyx
as code owners
August 27, 2026 12:20
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Hi @noeschmidt! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
Contributor
Author
|
Rapid mouse clicks on the same checklist checkbox were silently throttled to one toggle per 500ms, the dedup meant for mobile double-tap protection was also applied to desktop clicks, blocking spam-click toggling. Enregistrement.de.l.ecran.2026-08-27.a.14.12.11.mov |
meta-cla
Bot
added
the
CLA Signed
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
etrepum
reviewed
Aug 27, 2026
etrepum
left a comment
etrepum
left a comment
Collaborator
There was a problem hiding this comment.
I don't think it's useful to do so much renaming when the actual intent here is to delete one line of code
added 2 commits
August 28, 2026 07:35
…dback ## Description Review feedback on facebook#9102: the renames (`__lexicalCheckListLastHandled` → `__lexicalCheckListLastTouchHandled`, `recordHandled` → `recordTouchHandled`) added churn for what is really a one-line fix. This commit reverts all renaming and keeps only the essential change: `configHandleClick` no longer records a dedup timestamp, so rapid mouse clicks on the same desktop checkbox are not throttled. The comment above the dedup state is updated to explain why the click path reads but never writes the timestamp. ## Test plan ### Before ``` $ npx vitest run --project unit packages/lexical-list/src/__tests__/unit/checkList.test.tsx ``` 6/6 passing before this revision too; no behavior change, only a smaller diff. ### After ``` $ npx vitest run --project unit packages/lexical-list/src/__tests__/unit/checkList.test.tsx ✓ unit packages/lexical-list/src/__tests__/unit/checkList.test.tsx (6 tests) 107ms Test Files 1 passed (1) Tests 6 passed (6) ```
etrepum
approved these changes
Aug 28, 2026
unix-max
pushed a commit
to unix-max/lexical
that referenced
this pull request
Aug 28, 2026
…e checklist checkbox (facebook#9102) Co-authored-by: Noé SCHMIDT <noe.schmidt@infomaniak.com> Co-authored-by: Bob Ippolito <bob@redivi.com>
Merged
etrepum
pushed a commit
to etrepum/lexical
that referenced
this pull request
Sep 1, 2026
## Description Sync with main (24 commits, through 1723cd1). Conflict resolutions and semantic reconciliation with the checklist/semantic-nesting work: - ListItemNode.insertAfter split: keep `$copyListForSplit` (carries the semantic nesting mark) and adopt main's facebook#7032 numbering fix on top of it (`setStart($getNewListStart(...))` for numbered lists). - ListItemNode.remove / mergeLists: main added a same-listType guard before merging surrounding sublists (facebook#9050). `$collapseWrapperPair` now implements that policy for both call sites: differing-type boundary lists leave the two wrapper items as separate siblings (matching main's new tests) instead of collapsing them into one multi-list item. - ListImportExtension: adopt main's `listNode.createListItemNode()` subclass hook in `$normalizeListChildren`, threading it through `$normalizeSemanticChildren` as the wrapper-item factory; keep the shared `$isDomChecklist` helper over main's local copy. - checkList focus lifecycle (facebook#9088/facebook#9102): adopt main's press-tracking, returnFocusToRoot, CHECKBOX_KEYS keydown, and onUpdate-based arrow focus (replacing setTimeout) — reconciled with the native checkbox input mode: returnFocusToRoot resolves the focused input to its row via getActiveCheckListItem; the capture keydown defers ArrowRight on the input to KEY_ARROW_RIGHT_COMMAND (which needs the input still focused to fix the possibly-stale caret first); ArrowLeft on a focused native input stays a no-op (Right returns to the text, Escape exits) while the legacy li-focus mode keeps main's exit-to-root behavior; handleArrowUpOrDown keeps the wrapper-aware traversal, $selectCheckRowStart and plain-row focus-mode exit inside main's onUpdate structure. - $insertList: keep the three-argument $isSelectingEmptyListItem; adopt main's `$newListFrom` (copy-based retype) with `$copySemanticNestingMark` preserved at both call sites. - $removeList: keep the nested-list relocation and adopt main's format/indent/direction/style copy onto the replacement paragraph. - LexicalUtils: union — main's `removeEmptyDOMAttribute` plus the readonly `getCachedClassNameArray` docs. ## Test plan ### After ``` pnpm run test-unit — all packages pass pnpm vitest --project browser run packages/lexical-list Tests 25 passed (25) (4 consecutive runs) pnpm run ci-check — passes ``` Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Description
The checklist dedup mechanism (introduced in #8390 to prevent mobile double-toggle from
pointerup+ synthesizedclick) was applied symmetrically: bothconfigHandleClickandconfigHandlePointerUpchecked the dedup window and recorded a timestamp. As a side effect, rapid mouse clicks on the same desktop checkbox were throttled to one toggle per 500ms — the second click within the window was silently dropped.The dedup only needs to be unidirectional: a synthesized mobile
clickmust be absorbed when it follows a touchpointerupfor the same tap. Mouse clicks never have a preceding touchpointerup, so they should never be throttled.This change makes the dedup write-path touch-only:
configHandlePointerUpstill checks + records (mobile throttle preserved, including spam-tap protection).configHandleClickstill checks (absorbs synthesized mobile clicks) but no longer records a timestamp, so rapid desktop clicks on the same checkbox all toggle.The internal field is renamed
__lexicalCheckListLastHandled→__lexicalCheckListLastTouchHandledto reflect that it is now written exclusively by the touch path.Mobile behavior is unchanged: spam-tap on the same checkbox remains throttled at 500ms, and the
pointerup+clickdouble-toggle guard still works.Test plan
Before
No test covered rapid desktop clicks on the same item; the throttle was an undocumented side effect, so a second mouse click within 500ms was silently dropped (the checkbox stayed in its post-first-click state).
After
Two new tests lock in the behavior:
false → true → false.pointerups within the dedup window produce a single toggle (false → true), confirming the mobile throttle is unchanged.pnpm run lint,pnpm run tsc, andpnpm run prettierare clean on the affected package.