feat(plugin-block): configurable block handle hover responsiveness - #2456
feat(plugin-block): configurable block handle hover responsiveness#2456johnfumaster wants to merge 9 commits into
Conversation
- Introduced `onImageLoadError` callback in the image block configuration and image input component. - Updated relevant components to utilize the new error handling feature for image loading failures. - Enhanced documentation to reflect the new callback functionality.
Adds mousemoveThrottle config for the Block Plugin. Defaulted throttle to 50ms as 200ms seems too slow. Heavy work runs inside a single requestAnimationFrame, and a new move cancels the previous RAF. So you never run the expensive path more than once per frame, and it’s aligned with the browser’s paint. That reduces layout thrash and avoids stacking work when many events fire in one frame.
Ensures the block currently under the pointer is determined synchronously when a mousedown event occurs. This prevents delays in block activation and ensures correct selection, especially if a hover update was pending.
|
|
@johnfumaster is attempting to deploy a commit to the Milkdown Team on Vercel. A member of the Team first needs to authorize it. |
@milkdown/components
@milkdown/core
@milkdown/crepe
@milkdown/ctx
@milkdown/exception
@milkdown/kit
@milkdown/prose
@milkdown/transformer
@milkdown/utils
@milkdown/react
@milkdown/vue
@milkdown/plugin-automd
@milkdown/plugin-block
@milkdown/plugin-clipboard
@milkdown/plugin-collab
@milkdown/plugin-cursor
@milkdown/plugin-diff
@milkdown/plugin-emoji
@milkdown/plugin-highlight
@milkdown/plugin-history
@milkdown/plugin-indent
@milkdown/plugin-listener
@milkdown/plugin-prism
@milkdown/plugin-slash
@milkdown/plugin-streaming
@milkdown/plugin-tooltip
@milkdown/plugin-trailing
@milkdown/plugin-upload
@milkdown/preset-commonmark
@milkdown/preset-gfm
@milkdown/theme-nord
commit: |
johnfumaster
changed the title
johnfumaster
changed the title
|
Hi! I'm I would like to apply some automated changes to this pull request, but it looks like I don't have the necessary permissions to do so. To get this pull request into a mergeable state, please do one of the following two things:
|
autofix.ci
7b8cb64 to
2839d0d
Compare
|
Could you please enable Allow edits by maintainers so I can fix some concerns? Or you want me to comment it here and let you fix it yourself? |
|
Sorry @Saul-Mirone for the late reply. My forked branch belongs to our org and I don't see the option to do that. Please comment on the PR and I'll update it. 🙏🏼 |
Saul-Mirone
left a comment
Saul-Mirone
left a comment
There was a problem hiding this comment.
Thanks for the PR. The direction is right, 200ms is too slow and moving the work into a RAF makes sense. A few things to fix first.
Two blocking ones, both left inline:
bind()readsblockConfigwithout a try/catch. That is the crash #1958 fixed, and here it fails silently instead of throwing, so the handle just stops working.- Making
mousemoveThrottlerequired is a breaking type change, and at runtime a missing value means no throttling at all.
Smaller things:
- The description says same-block updates are skipped, but I don't see that in the code.
#resolveHoveralways calls#show(), socomputePosition()now runs 4x more often than before. Either add it or drop it from the description. docs/api/crepe.mdshould document the new Crepe option.- No tests. The mousedown path and the 5px skip are both easy to cover.
block-drag.spec.tsalso mocksblockConfigwithoutmousemoveThrottle, which is why the throttle=0 problem doesn't show up in CI. - The option is called
mousemoveThrottlebut the plugin listens topointermove. The internal naming is already like that so I don't mind much, but this one is public.
Checked out locally: lint, tsc and the unit tests all pass.
| this.#mousemoveCallback.cancel() | ||
| this.#mousemoveCallback = throttle( | ||
| this.#onMousemove, | ||
| ctx.get(blockConfig.key).mousemoveThrottle |
There was a problem hiding this comment.
This needs the same try/catch as #filterNodes above. ctx.get() throws when the slice isn't registered yet, which is what #1958 was about.
It fails badly here. bind() is called from BlockProvider.#init(), and update() catches and ignores the error:
try {
this.#init()
this.#initialized = true
} catch {
// ignore
}So this.#service never gets assigned, addEvent() and draggable = true never run, and the handle is dead with nothing in the console. #ctx and #notify are already set by the time it throws, so the service is left half bound too.
Reading the throttle lazily on the first mousemove would also work.
| { filterNodes: defaultNodeFilter }, | ||
| /// - `mousemoveThrottle`: Throttle delay in ms for block hover detection (default 50). | ||
| export const blockConfig = $ctx< | ||
| { filterNodes: FilterNodes; mousemoveThrottle: number }, |
There was a problem hiding this comment.
Can this stay optional?
{ filterNodes: FilterNodes; mousemoveThrottle?: number }ctx.set replaces the whole object, so ctx.set(blockConfig.key, { filterNodes }) becomes a type error after this change, and that is a common thing for people to have.
The runtime side is the bigger problem. throttle(fn, undefined) does not throttle at all, lodash does toNumber(wait) || 0 so wait becomes 0:
wait=undefined -> invoked 100 /100
wait=50 -> invoked 1 /100
So for anyone who sets blockConfig without the new field, #onMousemove runs on every pointermove, including the getBoundingClientRect() in the 5px check which forces layout. The RAF only batches what comes after it.
Making the field optional and reading it with ?? 50 fixes both.
| this.#hide() | ||
| return | ||
| } | ||
| this.#show(result) |
There was a problem hiding this comment.
Point 3 in the description says this is skipped when the block hasn't changed, but it runs every time. #show goes to BlockProvider.show(), which calls floating-ui's computePosition() with flip(). At 50ms instead of 200ms that is 4x the calls.
An early return should be enough:
if (result.el === this.#active?.el && result.$pos.pos === this.#active.$pos.pos)
return| /// @internal | ||
| #handleMouseDown = () => { | ||
| const view = this.#view | ||
| if (view && this.#lastMouseY >= 0) { |
There was a problem hiding this comment.
#lastMouseY >= 0 only matters for the else branch below, the #active branch doesn't use it. Moving the check into the else would make that clearer.
| if (this.#rafId !== null) cancelAnimationFrame(this.#rafId) | ||
| this.#rafId = requestAnimationFrame(() => { | ||
| this.#rafId = null | ||
| this.#resolveHover(view, event.clientY) |
There was a problem hiding this comment.
this.#lastMouseY holds the same value here, and a later mousemove cancels this frame before it runs. Using the field instead would avoid keeping the event alive in the closure.
| blockConfig?: { | ||
| mousemoveThrottle?: number | ||
| } |
There was a problem hiding this comment.
Two things:
BlockEditFeatureConfig is DeepPartial<BlockEditConfig>, so the ? are redundant. Every other field in this interface is declared as required.
More importantly, blockConfig is the name of an internal ctx slice and I'd rather not expose it in the Crepe config. Either flatten it to mousemoveThrottle, or move it under the existing blockHandle. This is public API so it is awkward to change later.
|
|
||
| return true | ||
| }, | ||
| mousemoveThrottle: config?.blockConfig?.mousemoveThrottle ?? 50, |
There was a problem hiding this comment.
If the read side falls back to 50 (see my comment on block-config.ts), this can just pass the value through. Right now 50 is written in three places: here, the ctx default and the doc comment. A shared constant would be better.
|
Awesome review @Saul-Mirone. I'll update the PR as soon as I find some time to work on it. 🙏🏼 |
2 participants
Summary
Improves block handle hover detection an dmakes drag initiation more reliable when the user clicks the handle before the hover RAF has run.
Problem:
Block handle hover used a fixed 200ms throttle and did all hit-testing inside that callback. That made the handle feel laggy when moving vertically between blocks and mousedown on the handle could run before
#activewas set - so#createSelection()had nothing to select and drag could target the wrong block.Changes:
Configurable hover throttle - Add
mousemoveThrottleto blockConfig (default50ms, down from the previous hardcoded200ms). Expose it via CrepeBlockEditasblockConfig.mousemoveThrottle.RAF-batched hover updates - Move
elementFromPoint/selectRootNodeByDomwork intorequestAnimationFrame, coalescing rapid mousemove events and cancelling stale frames when the pointer moves again.Cheaper mousemove path - Skip work when Y movement is under 5px and the active block is unchanged; skip
#hide()/#show()when the resolved block is the same element and position.Immediate active block on mousedown - On handle mousedown, synchronously resolve the block under the last known pointer Y (
#ensureActiveForPointer) before creating the node selection, so click-to-drag works even if hover detection has not painted yet.How did you test this change?