Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle text replacements explicitly #3807

Merged
merged 1 commit into from
Jun 22, 2023
Merged

Handle text replacements explicitly #3807

merged 1 commit into from
Jun 22, 2023

Conversation

luin
Copy link
Member

@luin luin commented Jun 20, 2023

Fixes: #3692, #3375, #2407, #2096, #3692 , #3375 , #2407 , #2096 , #3267 , #2187 , #2285 , #2144 , #3522 , #2498 , #3450 , #3711

In the case of text replacements (see definition below), we currently rely on the browser's default behavior and then utilize MutationObserver to synchronize DOM changes with the model. Sometimes, this approach doesn't work well, as browsers may generate unexpected mutations that Quill is unable to process.

At Slab, we handle text replacements explicitly with Input Event (mostly Level 1 but some Level 2 features supported by all modern browsers) and it's been working reliably for months. This PR mostly moves the code to Quill but also handles IME as well.

Definition of Text Replacements

The most common scenario occurs when a range of text is selected, and the user presses any keystrokes.

Additionally, built-in spell checks and some platform-specific operations (e.g., emoji picker, text replacement settings on macOS) can also be considered as text replacements.

It's important to note that the selection does not necessarily need to encompass the text that is about to be replaced. For instance, on some platforms, there may be a hover toolbar that appears when the user moves the cursor over the misspelled word, allowing them to select a word to replace the current word.

Test Plan

Open http://localhost:9000/standalone/full.

Simple text replacement

Enter "abc" and make "b" red. Select "bc" and press "d". Make sure the editor ends with "ad".

Simple text replacement with IME

Enter "abc" and make "b" red. Select "bc". Switch to a Chinese/Japanese IME, press "d" and confirm the word. Make sure "bc" is replaced by the confirmed word.

Spell check

Enter "a makeing" and make "makeing" red. Right-click on "makeing" and select "making" in the spell check list:

CleanShot 2023-06-22 at 17 47 47@2x

Make sure "makeing" is converted to "making", and "making" is in red.

Others

Make sure the mentioned issues are fixed.


I verified all the mentioned issues except #2407 which I can't reproduce on my side and this PR fixes all of them.

I also tested on the following browsers and all seem to work correctly:

  1. Safari 15 on macOS 12
  2. Safari 16 on macOS 13
  3. Chrome on Android API 34
  4. Safari 16 on iOS 16.5.1
  5. Firefox: 89 on macOS 10.15
  6. Chrome 114 on macOS 13
  7. Chrome: 99 on macOS 10.15

TODO items

  • Support Android devices.
  • I believe we can extend the Input module to handle more input types while also transferring some logic from the Keyboard module to the Input module. For instance, we could move the handleBackspace function to the Input module by handling the deleteContentBackward input type. This improvement would increase compatibility with system features, such as iOS's dictation.

@luin luin force-pushed the zh-input-observer branch 15 times, most recently from faa93f4 to 44c9e01 Compare June 22, 2023 12:42
@luin luin marked this pull request as ready for review June 22, 2023 13:26
@luin luin requested a review from jhchen June 22, 2023 13:27
@fnlctrl
Copy link
Contributor

fnlctrl commented Jul 20, 2023

(Sorry if this is out of scope of this PR, I can create a separate issue if that's the case)

In #2096 which is closed by this PR, a comment #2096 (comment) mentioned the handling of <font> elements as a workaround.

It seems that there's another case where <font> is inserted when typing with Chinese IME: typing on an empty line that has an empty inline element with <br> that holds the styling. This is native contenteditable behavior and is observed on both Chrome and Safari, and there seems to be no way to prevent it. In Quill, it causes content to be removed unless a <font> blot is registered.

Reproduction:

  1. create an html file with the following content, or open this fiddle
<!DOCTYPE html>
<html>
  <body>
    <div class="ql-editor" contenteditable="true" spellcheck="true">
      <p style="font-size: 18px">
        <strong style="color: rgb(0, 128, 0); font-size: 18px"
          >Styled paragraph (empty line below)</strong
        >
      </p>
      <p style="font-size: 18px">
        <strong style="color: rgb(0, 128, 0); font-size: 18px"><br /></strong>
      </p>
      <h1>List item</h1>
      <ol>
        <li>
          <span class="ql-ui" contenteditable="false"></span>
          <span style="color: rgb(102, 204, 132); font-size: 36px"
            >List with inline style (empty line below)
          </span>
        </li>
        <li>
          <span class="ql-ui" contenteditable="false"></span>
          <span style="color: rgb(102, 204, 132); font-size: 36px">
            <br />
          </span>
        </li>
      </ol>
    </div>
  </body>
</html>
  1. Select the empty <br> in browser inspect menu, type "a" "f" "c" on the empty line

  2. Observe that a <font> element is inserted

Gif recording:
Chrome:
chrome

Safari:
safari

@luin
Copy link
Member Author

luin commented Jul 21, 2023

I don't feel it's possible to have br wrapped with strong. Is there a way to reproduce that programmatically?

@fnlctrl
Copy link
Contributor

fnlctrl commented Jul 24, 2023

@luin Yes, though it's not the default behavior of quill (but it's a hack to restore the native contenteditable behavior)

Demo: https://stackblitz.com/edit/vitejs-vite-bcjsis?file=main.js&terminal=dev
Run npm start in the terminal

Long story short, one tiny but critical missing behavior of quill (or any other editors like tiptap/prosemirror etc.) that makes it inferior to the experience of Google Docs, is inheriting inline styles from previous lines when hitting enter at the end. Quill's cursor implementaion can only retain formats for the current line. The hack in the demo tries to mimic Google Docs' behavior

Google Docs behavior:

Vanilla quill behavior:

Hack in the demo

This hack is an approximation because it doesn't really persist the formats on empty lines. They get filtered out by quill.getContents(), but they survive reloads because inheriting styles from previous blocks is deterministic. It all works intuitively - until IME comes into play that breaks it apart.

I do consider the behavior of using <br> as placeholder to be correct, because it's exactly how native contenteditable retains formats for empty lines: It copies the styling, and adds a <br> to the empty <span>

If we can fix the <font> issue, then final piece of the puzzle would be actually persisting the empty line formats to Delta. It could work by allowing line breaks (blocks) to use inline-level attributes, e.g.

{ 
  insert: '\n', 
  attributes:  {
    color: '#569cd6',
    size: '18px',
    bold: true,
    underline: true,
    italic: true
  }
} 

This would represent the empty line that holds the default inline formats.

@luin
Copy link
Member Author

luin commented Jul 25, 2023

Okay I think that makes sense. Can you create a new ticket for that? I think a way to fix that is to listen to compositionstart when caret is at the beginning of a formatted empty line, and reset the br styles. Also listen to compositionend and restore the styles. Haven't thought about it thoroughly though.

@fnlctrl
Copy link
Contributor

fnlctrl commented Aug 9, 2023

We discovered that <font> insertion can be avoided if the empty line is held up by an empty space instead of a <br>. We managed to fix this by replacing the empty <br> with a space on compositionstart, and removing the space by manipulating DOM directly on compositionend. This seems to work well with undo/redo.

@luin
Copy link
Member Author

luin commented Nov 27, 2023

@fnlctrl Have you tried utilizing beforeinput event? By capturing beforeinput, you can take over the text insertion and apply inline attributes there:

CleanShot.2023-11-27.at.15.23.56.mp4

Code: https://github.com/quilljs/quill/compare/zh-emptyline-formats?expand=1

There are still some TODOs left though:

  1. Toolbar button status
  2. IME support. Safari and Firefox support insertFromComposition, which should make IME support easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

addRange(): The given range isn't in document. related to custom color/styling
3 participants