[lexical-list] Bug Fix: Toggle checklist items on mobile tap#8390
Conversation
The CheckList plugin's touchstart listener calls event.preventDefault() to keep the caret away from the marker. On iOS Safari and Android Chrome that suppression also cancels the synthesized click, so handleClick never runs and the checkbox cannot be toggled by tap. Reproducible in the official playground. Add a pointerup listener that runs the same toggle logic when event.pointerType === 'touch'. Deduplicate against any click that does fire (browsers where preventDefault didn't suppress it) via a 500ms window so we never double-toggle. Adds packages/lexical-list/src/__tests__/unit/checkList.test.tsx covering: touch pointerup over the marker toggles, mouse pointerup is ignored (click stays the desktop path), and pointerup followed by a synthesized click does not double-toggle.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
etrepum
left a comment
There was a problem hiding this comment.
This is directionally good, I think the tests could be cleaned up and maybe the tests should explicitly test clicking on multiple checkboxes in a short amount of time because I think this global state would make that not work very well.
| // tap. We additionally listen for pointerup with pointerType === 'touch' | ||
| // and run the same toggle logic, deduplicating against any click that | ||
| // does fire on browsers where preventDefault doesn't suppress it. | ||
| let lastHandledTimeStamp = 0; |
There was a problem hiding this comment.
Would it make more sense to store this on the element itself so you're not prevented from tapping or clicking on several checkboxes really fast?
There was a problem hiding this comment.
Done in 13e7f91 : switched to WeakMap<EventTarget, number> keyed by event.target. Added a regression test that taps two <li> 100ms apart and asserts both toggle (would fail under the previous global timestamp).
There was a problem hiding this comment.
I would just write it directly to the element. You can use a symbol if you want to hide it away but lexical already sets several properties on HTMLElements it manages
There was a problem hiding this comment.
Done in e3ad7fb : switched to __lexicalCheckListLastHandled set directly on the event target (with // @ts-ignore internal field ).
| // | ||
| // Dedup state is per-target: a global window would block tapping a second | ||
| // checkbox within 500ms of toggling the first. | ||
| const lastHandledByTarget = new WeakMap<EventTarget, number>(); |
There was a problem hiding this comment.
I think this would be a bit cleaner and maybe slightly more debuggable if the property was stored directly on the HTMLElement, either with a symbol or a prefixed property. Plenty of other examples of doing this to elements that the editor manages in the codebase, __lexicalEditor on root elements, __lexicalListType on all checklists, __lexicalTableSelection on selected tables, etc.
Description
You can't tap to toggle a checklist item on iOS Safari or Android Chrome. The bug is reproducible in the Lexical playground and in Chrome DevTools mobile emulation.
The cause: the
touchstartlistener registered by the CheckList plugin callsevent.preventDefault()to keep the caret away from the marker, which on mobile also cancels the synthesizedclick, and the click is whathandleClickis wired to. So the toggle path is never reached. PR #7655 widened the touch hit-test, but the click that the hit-test was meant to handle was already gone by then, so taps still did nothing.The fix is to also listen for
pointerupand run the toggle from there whenevent.pointerType === 'touch'. To stay safe on browsers where the click does eventually fire, a 500 ms dedup window swallows the second event so we never toggle twice. Desktop click is left alone.While I was in there, I widened the handler signatures from
MouseEvent | TouchEventtoPointerEvent | MouseEvent | TouchEvent(they were already accepting pointer events at runtime, this just makes it explicit) and replaced the(event as PointerEvent).pointerTypecast insidehandleCheckItemEventwith a'pointerType' in eventguard.Closes #7651
Test plan
Before
Tap a checklist marker in the playground on iOS Safari / Android Chrome / Chrome DevTools mobile emulation : nothing happens.
The new
packages/lexical-list/src/__tests__/unit/checkList.test.tsxfile fails onmain:🔴 touch pointerup over the marker area toggles the item
🔴 : pointerup followed by a synthesized click does not double-toggle
🟢 mouse pointerup is ignored (click stays the desktop path)
After
Same tap toggles the checkbox.
The three new tests pass, and the rest of the
lexical-listsuite still does (86/86).pnpm tscandpnpm lintare clean.