slab / quill

Quill is a modern WYSIWYG editor built for compatibility and extensibility
https://quilljs.com
BSD 3-Clause "New" or "Revised" License
43.14k stars 3.35k forks source link

Format toggled off after entering a newline #4306

Open juanedi opened 2 months ago

juanedi commented 2 months ago

If I'm writing with formats like "bold" and "inline" enabled and press a line break with the enter key, the formats get toggled off (so if I keep typing on the next line my text won't have them).

Steps for Reproduction

  1. Visit quilljs.com
  2. Clear all text in the demo editor
  3. Enabled "bold" formatting and type a few words
  4. Press enter to enter a newline
  5. Type again

Expected behavior: Whatever you type in the second line preserves the formatting toggles you had before.

Actual behavior: Formats get cleared after the line break.

Platforms:

Tested this in macos using Chrome, Firefox and Safari.

Version:

2.0.2

This was working in my application on 1.3.7. Oddly, I also see the bug in https://v1.quilljs.com, so am not entirely sure if this was caused entirely by the version upgrade or if there is another factor at play here.

juanedi commented 2 months ago

If it helps, I'm experimenting with working around this by overriding the handler for the "enter" key using pretty much the same code as the default but with a few tweaks (see NOTE comments) below. I haven't tested this thoroughly yet, but thought it might help pinpoint the problem.

EDIT: BAD CODE! See the real workaround below
bindings: {
  handleEnter: {
    key: "Enter",
    handler: (range, context) => {
      const lineFormats = Object.keys(context.format).reduce(
        (formats, format) => {
          if (
            // NOTE: original implementation had Scope.BLOCK here. So it looks like it is
            // deliberately resetting inline styles on newlines, for some reason.
            this._quill.scroll.query(format, Scope.ANY) &&
              !Array.isArray(context.format[format])
          ) {
            formats[format] = context.format[format];
          }
          return formats;
        },
        {},
      );

      const delta = new Delta()
            .retain(range.index)
            .delete(range.length)
            .insert("\n", lineFormats);
      this._quill.updateContents(delta, Quill.sources.USER);
      this._quill.setSelection(range.index + 1, Quill.sources.SILENT);

      // NOTE: I think Quill will is trying to maintain styles by applying the formats to the newline
      // character (see Delta construction above), but it isn't working. This is the other bit I changed
      // from the built-in implementation, re-applying any formats we had on the previous line.
      Object.entries(lineFormats).forEach(([format, value]) => {
        this._quill.format(format, value, Quill.sources.USER);
      });
      // NOTE: Ensure toolbar reflects the styles after the newline. Otherwise the buttons won't reflect
      // the active formats until the user starts typing.
      // This is probably related to https://github.com/slab/quill/issues/4305.
      this._quill.getModule("toolbar").update(range);

      this._quill.focus();
    },
  },
}
juanedi commented 2 months ago

OK, the "workaround" I shared above doesn't actually make much sense after a deeper look. Looks like lineFormats was actually meant to determine the styles for the newline itself only (presumably just to handle bullet lists?).

I did some digging and eventually arrived at https://github.com/slab/quill/pull/3428, where the "preserve styles on newline" behavior was deliberately dropped with the intention to make the behavior more intuitive.

I wonder if there's a chance we could reconsider the default behavior? I think folks in that PR made some good points re. which is the most intuitive behavior, but even leaving that aside: this feels like a relatively important breaking change. Might at least be worth adding a note to the "keyboard" section of the migration notes?

For anyone interested, below is the overloaded handler that reinstates that behavior. I'll try later to see if there's a more elegant way to "decorate" the default handler rather than redefining it completely, but the essence of the fix is here for those who prefer the old behavior.

bindings: {
  handleEnter: {
    key: "Enter",
    handler: (range, context) => {
      const lineFormats = Object.keys(context.format).reduce(
        (formats, format) => {
          if (
            this._quill.scroll.query(format, Scope.BLOCK) &&
              !Array.isArray(context.format[format])
          ) {
            formats[format] = context.format[format];
          }
          return formats;
        },
        {},
      );

      const delta = new Delta()
            .retain(range.index)
            .delete(range.length)
            .insert("\n", lineFormats);
      this._quill.updateContents(delta, Quill.sources.USER);
      this._quill.setSelection(range.index + 1, Quill.sources.SILENT);

      // NOTE: Changed from default handler!
      // Applies previous formats on the new line. This was dropped in 
      // https://github.com/slab/quill/commit/ba5461634caa8e24641b687f2d1a8768abfec640
      Object.keys(context.format).forEach((name) => {
        if (lineFormats[name] != null) return;
        if (Array.isArray(context.format[name])) return;
        if (name === "code" || name === "link") return;
        this._quill.format(
          name,
          context.format[name],
          Quill.sources.USER,
        );
      });

      // NOTE: Changed from default handler!
      // Ensure toolbar reflects the styles after the newline. Otherwise the buttons won't reflect
      // the active formats until the user starts typing.
      // This is probably related to https://github.com/slab/quill/issues/4305.
      this._quill.getModule("toolbar").update(range);

      this._quill.focus();
    },
  },
}
jonathaneckmier commented 1 month ago

@juanedi This is great. Just curious about something though, I tried adding this to my code and I'm getting some confusing results...

  1. There's an error in my console on enter: TypeError: Cannot read properties of undefined (reading '_quill')
  2. Oddly enough, even with that error being thrown, the formatting is retained for new lines so it appears to be "working", but I'm wondering if I'm just lucky.

Is there something I'm missing – any trick to how you're setting things up where this._quill would be defined that's different? Or is there any update on how you've implemented things in your context?

Here's my config currently (note: element is just a dom node):

      const editor = new Quill(element, {
        modules: {
          keyboard: {
            bindings: {
              handleEnter: {
                key: "Enter",
                handler: (range, context) => {
                  const lineFormats = Object.keys(context.format).reduce(
                    (formats, format) => {
                      if (
                        this._quill.scroll.query(format, Scope.BLOCK) &&
                        !Array.isArray(context.format[format])
                      ) {
                        formats[format] = context.format[format];
                      }
                      return formats;
                    },
                    {},
                  );

                  const delta = new Delta()
                    .retain(range.index)
                    .delete(range.length)
                    .insert("\n", lineFormats);
                  this._quill.updateContents(delta, Quill.sources.USER);
                  this._quill.setSelection(range.index + 1, Quill.sources.SILENT);

                  // NOTE: Changed from default handler!
                  // Applies previous formats on the new line. This was dropped in
                  // https://github.com/slab/quill/commit/ba5461634caa8e24641b687f2d1a8768abfec640
                  Object.keys(context.format).forEach((name) => {
                    if (lineFormats[name] != null) return;
                    if (Array.isArray(context.format[name])) return;
                    if (name === "code" || name === "link") return;
                    this._quill.format(
                      name,
                      context.format[name],
                      Quill.sources.USER,
                    );
                  });
                },
              },
            },
          },
        },
      });

EDIT I was able to get rid of any errors by replacing this._quill with editor in my context. I also had to import Scope from parchment and Delta from quill/core to satisfy a couple other errors that came up. Things seem to be working well, but would still love any more insight on how your fix was implemented in context to make sure I'm not missing anything important.

juanedi commented 1 month ago

Hey @jonathaneckmier!

I was able to get rid of any errors by replacing this._quill with editor in my context

In the context where I took that code excerpt from, this._quill refers to the Quill instance (ie. the result of calling new Quill). How exactly you get a reference to that object doesn't matter much.

There shouldn't be anything wrong with the way you adapted the code for your use case.

would still love any more insight on how your fix was implemented in context to make sure I'm not missing anything important.

You're good! In my case, the editor is initialized within a custom element that has a _quill instance variable. Because the handler is defined as an arrow function expression, this refers to that same object (the custom element) within the handler.

A more context-agnostic way to define it would be to:

  1. Change the arrow function to a plain old function (so (range, context) => {...} becomes function(range, context) { ... }). This will make it so that this refers to whatever the keyboard module binds it to when calling the handler, which happens to be the keyboard module object itself: https://github.com/slab/quill/blob/cc898efe9f5662baeadf18dfd2ccca56a3284a7e/packages/quill/src/modules/keyboard.ts#L260

  2. Change this._quill to this.quill. This makes the handler use the reference to the Quill object that the keyboard module holds in the quill instance variable.

Oddly enough, even with that error being thrown, the formatting is retained for new lines so it appears to be "working", but I'm wondering if I'm just lucky.

I would 100% recommend you don't keep going after seeing an error like that 😅

Good luck!

jonathaneckmier commented 1 month ago

@juanedi Thanks so much for the reply, this is really helpful additional context, so thank you! Hopefully if others are also looking at this issue this thread will be helpful. I wish there was a configuration in Quill to retain formatting across line breaks.