Repository navigation
Conversation
5ec0d41 to
3a0c212
Compare
|
Updated the regression test for Firefox 115, where formatting an entire text node may not emit selectionchange. The test now explicitly triggers that event after applying/removing bold. |
|
hi~@neSpecc, I hope your week is going well! I know you're probably swamped, so no urgency, but I wanted to gently follow up on this PR and see if you might have some bandwidth to review it soon. Happy to make any adjustments needed! |
There was a problem hiding this comment.
🔵 Needs a closer look
Interactions between retained UI, asynchronous tool rendering, and cleanup warrant final human review.
No open issues 🎉
What changed in this PR
Addresses #2822 by preserving the inline-toolbar container during selection refreshes, avoiding flicker while still recreating tools and buttons.
Changes:
- Updates popover contents and positioning without remounting the container.
- Guards pending interactions and stale asynchronous renders.
- Adds regression coverage for refresh behavior and cleanup.
| File | Description |
|---|---|
| test/cypress/tests/modules/InlineToolbarRefresh.cy.ts | Tests container preservation, positioning, links, and asynchronous cleanup. |
| src/components/utils/popover/popover-inline.ts | Adds in-place item replacement and interaction controls. |
| src/components/modules/toolbar/inline.ts | Preserves the popover and guards asynchronous refresh completion. |
Review effort: Balanced
There was a problem hiding this comment.
What bothers me most is that the popover implementation diverges from the one we extracted into the UI Kit (https://github.com/editor-js/utils/tree/main/packages/ui-kit). I’d recommend first replacing the current Popover implementation in the editor with the ui-kit package in a separate PR, and only then adding new methods to Popover if needed. By the way, an addItem method was recently added there, which adds items without reinitialization.
There was a problem hiding this comment.
Hi @neSpecc, I propose a separate PR to migrate the inline toolbar, toolbox, and block settings to @editorjs/ui-kit@2.0.1. I’d remove duplicated code/styles, preserve existing public type exports, and keep compatibility changes minimal.
I’d verify keyboard, focus, selection/link, and mobile behavior, then adapt #3027 on top. One packaging detail to confirm: UI Kit lacks license metadata/files, which may conflict with Editor’s license check.
Does this scope look right to you?
There was a problem hiding this comment.
Related to #2822.
This fix keeps the open inline-toolbar container mounted during selection refreshes, preventing flicker and repeated entrance animations. Tools and buttons are still recreated, preserving existing cleanup, including link handling. It also handles positioning, pending interactions, stale renders, and cleanup after render failures.
Validation
13/13 new and 54/54 existing related Cypress cases passed, plus 13/13 checks each in Chromium, Firefox, and WebKit. ESLint reports no errors; TypeScript has the same 719 baseline diagnostics. Tests loaded source through Vite; no distribution build or full local CI run was performed.
Flipper
The unchanged keyboard-navigation test failed in earlier runs on both unmodified v2.31.7 and the earlier prototype. Its 100ms wait can race the 180ms selection debounce, leaving currentBlock undefined; two retries did not consistently resolve it. Flipper remains untouched and was excluded from the final selected run.
More
Retaining tool/button instances could be a separate follow-up.