joinThoughts: Multiline not updated

[raineorshine]

Background

Single-line and multiline thoughts use different line-heights* for aesthetic reasons. Because there is no CSS-only method of applying a style to multiline thoughts only, we have to use Javascript. Detecting if a thought is multiline is not the hard part; it's knowing when to re-trigger the calculation. ResizeObservers are too expensive, so we manually define all the conditions in various useLayoutEffects. There are a lot of conditions to keep track of, and it's easy to miss new ones.

The first question for this issue is, why is multiline not updated correctly on join? A value change should trigger a recalculation. It may be a timing issue, or it may be that there is some other piece of state that needs to be included in the effect dependencies.

See: useMultiline

(*Completely different approaches such as using padding instead of line-height have been explored, and are currently untenable. See #2551. If you can think of a better alternative, please post it there.)

Steps to Reproduce

- a
  - This is some text that that fits on one line.
  - This is some more text that that fits on one line.
  - This is even more text that that fits on one line.
b

1. Set the browser width so each of the thoughts fit on a single line, but the combined text would take up more than one line.
2. Set the cursor on the first child of a.
3. Activate Join Thoughts

Current Behavior

Line height is incorrect because thought is not re-rendered with multiline: true

Expected Behavior

[snqb]

Hey, I really fear sounding stupid here, but isn't useLayoutEffect something that runs the effect BEFORE the dom recalculations? And using useEffect instead just solves the issue right away.

In updateMultiline we rely on dom calculations. But the function fires just before those. It's easy to confirm if we just log the sizings. On my machine the 3 lines combined beget a thought of 108px height. But the function doesn't know about it yet, it's still 36px. It also explains that it works eventually, after another re-render xd

  const updateMultiline = useCallback(() => {
    if (!contentRef.current) return

    const height = contentRef.current.getBoundingClientRect().height
    // must match line-height as defined in thought-container
    const singleLineHeight = fontSize * 2
    // .editable.multiline gets 5px of padding-top to offset the collapsed line-height
    // we need to account for padding-top, otherwise it can cause a false positive
    const style = window.getComputedStyle(contentRef.current)
    const paddingTop = parseInt(style.paddingTop)
    const paddingBottom = parseInt(style.paddingBottom)
    // 2x the single line height would indicate that the thought was multiline if it weren't for the change in line-height and padding.
    // 1.2x is used for a more forgiving condition.
    // 1.5x can cause multiline to alternate in Safari for some reason. There may be a mistake in the height calculation or the inclusion of padding that is causing this. Padding was added to the calculation in commit 113c692. Further investigation is needed.
    // See: https://github.com/cybersemics/em/issues/2778#issuecomment-2605083798
    setMultiline(height - paddingTop - paddingBottom > singleLineHeight * 1.2)
  }, [contentRef, fontSize])

anyways, I've attached a draft pr, you can see it works fine that way

[raineorshine]

useLayoutEffect is called after DOM calculations, just before paint. It's made specifically for situations where you need to measure the DOM and re-render before the first paint.

https://react.dev/reference/react/useLayoutEffect

If changing useLayoutEffect to useEffect fixes the issue, it's probably because the slight delay gives something else the chance to update. The downside is that in other situations it's going to paint once with the incorrect multiline value before re-painting with correct value from the new measurement.

This actually used to be useEffect and I worked hard to get it changed over to useLayoutEffect so that we can render it correctly on the first paint. It's unfortunate that this case seems to be an exception. I think it would be worth experimenting some more to see if you can identify what state changes that invalidates the useLayoutEffect calculation. If we can add that to the dependency list, maybe we can get it to do the correct calculation before the first paint.

[snqb]

yeah, I did sound stupid hahaha
I sincerely had a lapse of reason

OK, I'll see why it's having the delay. it's sure important to have it as a layout effect, yeah

[raineorshine]

I don't think it was stupid. useLayoutEffect is not so common, and there was some additional background on the issue you didn't have. I'm glad it makes sense now why it was done that way.

Thanks, let me know if you figure out anything!

[snqb]

I now think that problem is this:

    // While editing, watch the current Value and trigger the layout effect
  const editingValue = editingValueStore.useSelector(state => (isEditing ? state : null))

  // Recalculate multiline on mount, when the font size changes, edit, split view resize, value changes, and when the
  // cursor changes to or from the element.
  useLayoutEffect(updateMultiline, [
    contentRef, <----- this does not change when you do a join
    fontSize,
    isEditing,
    showSplitView,
    simplePath,
    splitPosition,
    editingValue, <---- this is the thing that is triggering on join
    updateMultiline,
  ])

 const updateMultiline = useCallback(() => {
    if (!contentRef.current) return
. . .. . . . . . .  . . .
    setMultiline(height - paddingTop - paddingBottom > singleLineHeight * 1.2)
  }, [contentRef, fontSize]) <---- depends on `contentRef`, but not on `editingValue`

the updateMultiline is rightly called, but it's stale, as a ref upstream did not change, thus the dimensions are old
and if we listen for editingValue here too, we are "synchronized" with the effect

[raineorshine]

Hi, thanks for the update. I don't see how updateMultipline can be stale, as it directly accesses the DOM. It doesn't use editingValue, it just measures the height after editingValue has changed.

[snqb]

but it is.
My understanding is that we pass a ref as a prop(object), and we rely on it in useCallback
so each the useLayoutEffect fires, the useCallback is invoked, but it contains the old(memoized) ref to the element, so it calculates from that.

editingValue changes -> useLayoutEffect fires -> calls memoized/stale useCallback
doesn't seem strange to me at all, and we can see that empirically.
if we'd remove useCallback part, just use a fn, it'd work fine.
As well as if we'd include editingValue in the deps.

[raineorshine]

if we'd remove useCallback part, just use a fn, it'd work fine.

Fair enough, that would demonstrate it! It's not clicking for me yet, but I look forward to seeing it in action.

I didn't think callbacks needed to update refs, since contentRef.current will always return the latest instance.

[snqb]

It's stale, it uses the ref from the previous render:
I've logged just one path, with their measured height.

or even simpler, logging the contentRef in the updateMultiline.

It's like we don't really wanna memoize the contentRef manipulations based on contentRef, which is a stable object.
we even have this thing that our useLayoutEffect depends on:

em/src/components/Editable/useMultiline.ts

Lines 21 to 22 in f87b263

	// While editing, watch the current Value and trigger the layout effect
	const editingValue = editingValueStore.useSelector(state => (isEditing ? state : null))

like if to think, what is the sense of memoizing a function that measures the element's rect? should it be memoized in the first place? why do we need a clojure with an arbitrary state of contentRef to measure the current state of the element?

so I'd suggest just dropping the memo part, or adding the same editingValue thing we use in the effect too, as that's what's driving the hooks on join.(both solve the issue)

[raineorshine]

Thank you for the additional explanation, that's helpful.

I'm now wondering if this behavior could be related to the React key. Due to the way that the join reducer is written, the cursor thought is edited to the joined value, and its siblings are deleted. This means that its id is unchanged, and thus its React key is unchanged, and React will reuse the same DOM element. Could this explain why the stale contentRef has an incorrect measurement?

We could this theory out be rewriting join to create a new thought rather than editing an existing thought. Though this may be unnecessary if we remove the memoization.

like if to think, what is the sense of memoizing a function that measures the element's rect? should it be memoized in the first place? why do we need a clojure with an arbitrary state of contentRef to measure the current state of the element?

so I'd suggest just dropping the memo part, or adding the same editingValue thing we use in the effect too, as that's what's driving the hooks on join.(both solve the issue)

That's a good point. I'm really not sure why its memoized, now that you point it out. Perhaps to avoid re-defining the function on every render, but that's insignificant. Memoizing probably uses up extra memory anyway.

---

Aside: The videos are super helpful, but it would be even more useful if they were uploaded as .mp4 or .mov so that the playback controls appear. Animated gifs aren't great for close review since I can't easily move forward or backward, or frame by frame. Something to consider. Thanks!

[snqb]

Yes, modifying the join indeed does work, as we get a new thought. But to alter that to change this, won't be wise imo xd

So let's stick with a function that is recreated?(attaching it as a pr)
Also, I think it's still okto just add the editingValue in the deps. Though, it's hacky, but we're already doing it anyways.

p.s.
should've been mp4 or something, yeah right. I'll use videos from now on, that's useful, thanks.

[raineorshine]

Okay! Removing the useCallback makes sense to me.

Great work, thanks.