Skip to content
1 change: 1 addition & 0 deletions packages/crepe/src/feature/block-edit/handle/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ export function configureBlockHandle(

return true
},
mousemoveThrottle: config?.blockConfig?.mousemoveThrottle ?? 50,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

})
ctx.set(block.key, {
view: () => new BlockHandleView(ctx, config),
Expand Down
4 changes: 4 additions & 0 deletions packages/crepe/src/feature/block-edit/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ interface BlockEditConfig {
handleDragIcon: string
buildMenu: (builder: GroupBuilder<SlashMenuItem>) => void

blockConfig?: {
mousemoveThrottle?: number
}
Comment on lines +20 to +22

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


blockHandle: Pick<
BlockProviderOptions,
| 'shouldShow'
Expand Down
7 changes: 4 additions & 3 deletions packages/plugins/plugin-block/src/block-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,11 @@ export const defaultNodeFilter: FilterNodes = (pos) => {
/// A slice contains the block config.
/// Possible properties:
/// - `filterNodes`: A function to filter nodes that can be dragged.
export const blockConfig = $ctx<{ filterNodes: FilterNodes }, 'blockConfig'>(
{ filterNodes: defaultNodeFilter },
/// - `mousemoveThrottle`: Throttle delay in ms for block hover detection (default 50).
export const blockConfig = $ctx<
{ filterNodes: FilterNodes; mousemoveThrottle: number },

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

'blockConfig'
)
>({ filterNodes: defaultNodeFilter, mousemoveThrottle: 50 }, 'blockConfig')

withMeta(blockConfig, {
displayName: 'Ctx<blockConfig>',
Expand Down
79 changes: 68 additions & 11 deletions packages/plugins/plugin-block/src/block-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import type { EditorView } from '@milkdown/prose/view'
import { editorViewCtx } from '@milkdown/core'
import { browser } from '@milkdown/prose'
import { NodeSelection } from '@milkdown/prose/state'
import { throttle } from 'lodash-es'
import { throttle, type DebouncedFunc } from 'lodash-es'

import type { FilterNodes } from './block-config'
import type { ActiveNode } from './types'
Expand Down Expand Up @@ -79,6 +79,11 @@ export class BlockService {
/// @internal
#dragging = false

/// @internal
#lastMouseY = -1
/// @internal
#rafId: number | null = null

/// @internal
get #filterNodes(): FilterNodes | undefined {
try {
Expand Down Expand Up @@ -112,6 +117,11 @@ export class BlockService {
bind = (ctx: Ctx, notify: BlockServiceMessage) => {
this.#ctx = ctx
this.#notify = notify
this.#mousemoveCallback.cancel()
this.#mousemoveCallback = throttle(
this.#onMousemove,
ctx.get(blockConfig.key).mousemoveThrottle

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

)
}

/// Add mouse event to the dom.
Expand All @@ -132,11 +142,39 @@ export class BlockService {

/// Unbind the notify function.
unBind = () => {
if (this.#rafId !== null) {
cancelAnimationFrame(this.#rafId)
this.#rafId = null
}
this.#mousemoveCallback.cancel()
this.#notify = undefined
}

/// @internal
#handleMouseDown = () => {
const view = this.#view
if (view && this.#lastMouseY >= 0) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#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 = null
}
// Prefer the block already shown on the handle; only resolve from the
// pointer when hover has not established one yet.
if (this.#active) {
const filterNodes = this.#filterNodes
if (filterNodes) {
const rect = this.#active.el.getBoundingClientRect()
const result = selectRootNodeByDom(
view,
{ x: rect.left + rect.width / 2, y: rect.top + rect.height / 2 },
filterNodes
)
if (result?.el === this.#active.el) this.#active = result
}
} else {
this.#resolveHover(view, this.#lastMouseY)
}
}
this.#activeDOMRect = this.#active?.el.getBoundingClientRect()
this.#createSelection()
}
Expand Down Expand Up @@ -218,12 +256,10 @@ export class BlockService {
}

/// @internal
#mousemoveCallback = throttle((view: EditorView, event: MouseEvent) => {
if (!view.editable) return

#resolveHover = (view: EditorView, mouseY: number) => {
const rect = view.dom.getBoundingClientRect()
const x = rect.left + rect.width / 2
const dom = view.root.elementFromPoint(x, event.clientY)
const dom = view.root.elementFromPoint(x, mouseY)
if (!(dom instanceof Element)) {
this.#hide()
return
Expand All @@ -232,18 +268,39 @@ export class BlockService {
const filterNodes = this.#filterNodes
if (!filterNodes) return

const result = selectRootNodeByDom(
view,
{ x, y: event.clientY },
filterNodes
)
const result = selectRootNodeByDom(view, { x, y: mouseY }, filterNodes)

if (!result) {
this.#hide()
return
}
this.#show(result)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}, 200)
}

/// @internal
#onMousemove = (view: EditorView, event: MouseEvent) => {
if (!view.editable) return

// Skip tiny Y jitter while still inside the active block; leaving its
// vertical bounds always resolves so adjacent blocks are not missed.
if (this.#active && Math.abs(event.clientY - this.#lastMouseY) < 5) {
const activeRect = this.#active.el.getBoundingClientRect()
if (event.clientY >= activeRect.top && event.clientY <= activeRect.bottom)
return
}
this.#lastMouseY = event.clientY

if (this.#rafId !== null) cancelAnimationFrame(this.#rafId)
this.#rafId = requestAnimationFrame(() => {
this.#rafId = null
this.#resolveHover(view, event.clientY)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

})
}

/// @internal
#mousemoveCallback: DebouncedFunc<
(view: EditorView, event: MouseEvent) => void
> = throttle(() => {}, 50)

/// @internal
mousemoveCallback = (view: EditorView, event: MouseEvent) => {
Expand Down
2 changes: 1 addition & 1 deletion packages/plugins/plugin-block/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export * from './types'
/// @internal
export type BlockPlugin = [
$Ctx<PluginSpec<any>, 'blockSpec'>,
$Ctx<{ filterNodes: FilterNodes }, 'blockConfig'>,
$Ctx<{ filterNodes: FilterNodes; mousemoveThrottle: number }, 'blockConfig'>,
$Ctx<() => BlockService, 'blockService'>,
$Ctx<BlockService, 'blockServiceInstance'>,
$Prose,
Expand Down
Loading