Repository navigation
feat: Placeholder DOM elements - #3205
matthewlipski wants to merge 3 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| // positions are hardcoded for now | ||
| if (state.doc.content.size === 6) { | ||
| decs.push( | ||
| Decoration.node(2, 4, { |
There was a problem hiding this comment.
I think (had a quick look), you should be able to keep the old code, but change Decoration.node into Decoration.widget (in the right position)?
Your performance regression is likely that the new version loops the document (doc.descendents). From a quick look, I don't understand why all the code needed to change and not just the decoration type
# Conflicts: # packages/core/src/extensions/Placeholder/Placeholder.ts
Summary
Currently, DOM elements are rendered as CSS pseudo elements. This is a problem for a11y accessibility testing as we cannot use an axe-core scan to check its contrast. This is because axe only checks contrast for DOM elements. Therefore, this PR changes the implementation to use real DOM elements for placeholders.
This comes with a performance overhead. A script has been added in this PR which benchmarks the render time between the old & new implementations, using a document of 1000 empty paragraphs and 1000 empty list items. It measures the initial render times, as well as re-renders on selection and content changes. The table below shows the performance overhead tested locally.
The alternative solution to this problem is to have additional contrast check tests just for placeholders. To be honest, both that solution and the one in this PR don't seem the cleanest, so I would appreciate feedback r.e. this.
This PR is currently more of an experimental change as I'm unsure whether we want this.
Fixes #3245. Replacing CSS pseudo-elements with DOM placeholders also resolves the stale "List" placeholder in Firefox after collaborative edits. Manually verified with Chromium as the writer and Firefox as the observer: the placeholder disappears when remote text arrives, including across repeated Enter-and-type cycles.
Rationale
TODO
Changes
TODO
Impact
TODO
Testing
TODO
Screenshots/Video
TODO
Checklist
Additional Notes
TODO