cybersemics / em

A beautiful, minimalistic note-taking app for personal sensemaking.
Other
286 stars 119 forks source link

Black background color not stripped from ContentEditable #2548

Closed raineorshine closed 1 week ago

raineorshine commented 1 week ago

The background color is incorrectly set to black when the default text color it selected ~however this only manifests after another character is typed~.

This suggests to me that the thought value is being updated correctly, but the innerHTML of the ContentEditable component is not, so as soon as the user starts editing again the value is updated. Indeed, the ContentEditable has a mechanism to prevent the innerHTML from changing during editing. This may need to be bypassed or overridden in order to ensure that it aligns with the thought value after formatting. Untested.

Do not simply strip the background color in a followup edit. We want to fix this at the root of the problem rather than piling on good edits to fix bad edits.

Steps to Reproduce

Hello
  1. Set the caret at the end of the thought.
  2. Set the text color to blue.
  3. Set the text color to default (white).

Current Behavior

<font style="background-color: rgb(0, 0, 0);">Hello</font>

Expected Behavior

Hello

Testing

Add a puppeteer test that covers this case.

raineorshine commented 1 week ago

2546 partially fixed this, although I'm still seeing an incorrect innerHTML which I would like to fix.

Basically, the thought value is correct, but the innerHTML is not. Since this can be difficult to observe in Chrome Dev Tools without triggering a blur event, I recommend temporarily adding the following code to the editThoughtActionCreator:

    setTimeout(() => {
      console.log({
        innerHTML: document.querySelector('[data-editing] [contenteditable]')?.innerHTML,
        value: payload.newValue,
      })
    }, 400)

This will show you both the persisted thought value and the innerHTML of the contenteditable, with enough of a delay to ignore any intermediate changes.

Current Behavior

After following the steps to reproduce (above), the following is logged:

{
  innerHTML:  "<font style=\"background-color: rgb(0, 0, 0);\">Hello</font>"
  value: "Hello"
}

Expected

{
  innerHTML: "Hello"
  value: "Hello"
}
raineorshine commented 1 week ago

It appears I was on the wrong commit! Sorry about that.

However, there is the new problem that the value is set to an empty string. Unfortunately the timing of document.execCommand('delete') causes the empty string to trigger the onChange event, which calls editThought, which gets persisted to the database. While the DOM can sustain intermediate values, we don't want those persisted to the database, even temporarily. (Especially given the CRDT infrastructure where there is a permanent history of every edit.)

Current Behavior

{
  innerHTML:  "Hello"
  value: ""
}

Expected

{
  innerHTML: "Hello"
  value: "Hello"
}
raineorshine commented 1 week ago

I did some investigation and the problem may be further back:

https://github.com/cybersemics/em/blob/aa98cd95ba0242532f61c4a8cd956c6fa794594b/src/actions/formatSelection.ts#L33

If you use document.execCommand to set the background color to the default (black), then it probably triggers an onInput event that propagates the invalid value (background-color: rgb(0, 0, 0)) to editThought. The invalid value should never get passed to editThought.

document.execCommand is a new technique in em, so we have to think about how its particular behavior can be integrated into the ContentEditable component. Triggering onInput in this new way creates new challenges. Since document.execCommand is low level, we certainly want to address this at the lowest level possible (i.e. not in editThought).

Open to your suggestions.

raineorshine commented 1 week ago

I found a way to remove the delete and insertHTML while still preserving the caret offset. That should slightly simplify things. The invalid background color issue remains.

3444487357