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

Component: Editor - Upgrade Quill to 2.0 #14721

Closed
luin opened this issue Feb 6, 2024 · 30 comments · Fixed by #15764
Closed

Component: Editor - Upgrade Quill to 2.0 #14721

luin opened this issue Feb 6, 2024 · 30 comments · Fixed by #15764
Labels
dependencies Issue or pull request is related to a dependency package
Milestone

Comments

@luin
Copy link

luin commented Feb 6, 2024

Describe the bug

Hi 👋
I'm the maintainer of Quill and we are currently working actively for Quill 2.0. Given PrimeNG is still using Quill 1.3, I'm wondering if you'd like to take a PR of upgrading Quill to 2.0? If that's the case, I'd happy to create a PR.

Looking at the code, I think we can make the component support ^1.3 and ^2.0 at the same time so that PrimeNG can release it without a major version bump. There are some option changes introduced in 2.0 so we will also need to update the API documentation accordingly.

Environment

Quill 2.0

Reproducer

No response

Angular version

Not applied

PrimeNG version

17.5.0

Build / Runtime

Angular CLI App

Language

TypeScript

Node version (for AoT issues node --version)

Not applied

Browser(s)

No response

Steps to reproduce the behavior

No response

Expected behavior

No response

@luin luin added the Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible label Feb 6, 2024
@mehmetcetin01140 mehmetcetin01140 added Type: Enhancement Issue contains an enhancement related to a specific component. Additional functionality has been add and removed Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible labels Feb 8, 2024
@cetincakiroglu cetincakiroglu added this to the 17.Future milestone Feb 12, 2024
@psarno
Copy link

psarno commented Mar 10, 2024

Is there any timeframe for the update to Quill 2.0 for <p-editor>?

It would help clear up the console warnings. It's not causing any actual runtime issues at the moment, but it would be nice to get ahead of it.

[Violation] Listener added for a synchronous 'DOMNodeInserted' DOM Mutation Event. This event type is deprecated and work is underway to remove it from this browser. Usage of this event listener will cause performance issues today, and represents a risk of future incompatibility. Consider using MutationObserver instead.

image

@dgarciarubio
Copy link

Apparently they're deprecating mutation events this July.

Is Quill 2.0 in the roadmap? Or any workaround?

@ThoSap
Copy link
Contributor

ThoSap commented Apr 10, 2024

As you can see Quill is again under active development since September 2023.
The new v2.0 version, which will be released soon no longer has the mutation events which will be removed from Chrome/Chromium on July 2024 (Chromium 127).
slab/quill#3806 (comment)
https://github.com/quilljs/quill/releases

I do not think the community will switch to TinyMCE, because for open-source it is limited to 1,000 editor loads per month and the CKEditor has many features behind a paywall, for example paste from Microsoft Word, which works in Quill v2.0.

@pacozevallos
Copy link

Hello, I was testing quill@2.0.0-rc.5 with PrimeNg 17.13 in Angular 17, I am using a reactive form and it does not recognize the default value of a "formControlName". I tried downgrading the version to quill@1.3.7 and it works but I get the "DOMNodeInserted" warning

Any help is welcome.

@ThoSap
Copy link
Contributor

ThoSap commented Apr 14, 2024

In the transitioning phase until the PrimeNG Editor supports Quill v2.0.0 I'm using ngx-quill with the following styles.scss override, so the editor looks the same.

// Copied from PrimeNG _editor.scss and modified for ngx-quill
quill-editor {
  .ql-toolbar {
    background: $editorToolbarBg;
    border-top-right-radius: $borderRadius;
    border-top-left-radius: $borderRadius;

    &.ql-snow {
      border: $editorToolbarBorder;

      .ql-stroke {
        stroke: $editorToolbarIconColor;
      }

      .ql-fill {
        fill: $editorToolbarIconColor;
      }

      .ql-picker {
        // Custom Quill overrides start
        &:not(.ql-color-picker) {
          width: unset;
        }
        // Custom Quill overrides end

        .ql-picker-label {
          // Custom Quill overrides start
          display: flex;
          align-items: center;
          justify-content: space-between;
          // Custom Quill overrides end
          border: 0 none;
          color: $editorToolbarIconColor;

          // Custom Quill overrides start
          svg {
            position: relative;
            margin-top: unset;
            top: unset;
            right: unset;
          }
          // Custom Quill overrides end

          &:hover {
            color: $editorToolbarIconHoverColor;

            .ql-stroke {
              stroke: $editorToolbarIconHoverColor;
            }

            .ql-fill {
              fill: $editorToolbarIconHoverColor;
            }
          }
        }

        &.ql-expanded {
          .ql-picker-label {
            color: $editorToolbarIconHoverColor;

            .ql-stroke {
              stroke: $editorToolbarIconHoverColor;
            }

            .ql-fill {
              fill: $editorToolbarIconHoverColor;
            }
          }

          .ql-picker-options {
            background: $inputOverlayBg;
            border:$inputOverlayBorder;
            box-shadow:$inputOverlayShadow;
            border-radius: $borderRadius;
            padding: $inputListPadding;

            .ql-picker-item {
              color: $inputListItemTextColor;

              &:hover {
                color: $inputListItemTextHoverColor;
                background: $inputListItemHoverBg;
              }
            }
          }

          &:not(.ql-icon-picker) {
            .ql-picker-item {
              padding: $inputListItemPadding;
            }
          }
        }
      }
    }
  }

  .ql-container {
    border-bottom-right-radius: $borderRadius;
    border-bottom-left-radius: $borderRadius;

    &.ql-snow {
      border: $editorContentBorder;
    }

    .ql-editor {
      background: $inputBg;
      color: $inputTextColor;
      border-bottom-right-radius: $borderRadius;
      border-bottom-left-radius: $borderRadius;
    }
  }

  .ql-snow.ql-toolbar button:hover,
  .ql-snow.ql-toolbar button:focus {
    color: $editorToolbarIconHoverColor;

    .ql-stroke {
      stroke: $editorToolbarIconHoverColor;
    }

    .ql-fill {
      fill: $editorToolbarIconHoverColor;
    }
  }

  .ql-snow.ql-toolbar button.ql-active,
  .ql-snow.ql-toolbar .ql-picker-label.ql-active,
  .ql-snow.ql-toolbar .ql-picker-item.ql-selected {
    color: $editorIconActiveColor;

    .ql-stroke {
      stroke: $editorIconActiveColor;
    }

    .ql-fill {
      fill: $editorIconActiveColor;
    }

    .ql-picker-label {
      color: $editorIconActiveColor;
    }
  }
}

// For ngx-quill component quill-view-html
quill-view-html {
  .ql-container {
    .ql-editor {
      color: $inputTextColor;
    }
  }
}

Provider for Angular Standalone:

provideQuillConfig({
      modules: {
        // This enables syntax highlighting using highlight.js which then also requires us to install this dependency
        syntax: false,
        toolbar: [
          [
            {'font': []},
            {'header': 1},
            {'header': 2},
            {'size': ['small', false, 'large', 'huge']},
            'clean',
          ],
          [
            'bold',
            'italic',
            'underline',
            'strike',
            'code',
            {'script': 'sub'},
            {'script': 'super'},
          ],
          [
            {'color': []},
            {'background': []},
          ],
          [
            'blockquote',
            {'align': []},
            {'list': 'ordered'},
            {'list': 'bullet'},
            {'indent': '-1'},
            {'indent': '+1'},
          ],
          [
            'link',
            'image',
            'code-block',
          ],
        ],
      },
    }),

@aheritier
Copy link

quill 2.0 is out and reported as a security issue (disputed but still concerning):
CVE-2021-3163 / GHSA-4943-9vgg-gr5r

@luin reported the upgrade almost 3 months ago and also proposed to help to build a PR.

If we had the fact that quill 1.3 won't work in Chrome this summer after deprecating mutation events this July could it be possible for the community to look at this issue and collaborate with @luin to build a fix ?

Thanks a lot

Note: All users upgrading Quill to v2 will have now their editors broken.

FYI @stephanj

@luin
Copy link
Author

luin commented Apr 22, 2024

Looks like only the scrollingContainer option is affected: in v2, Quill auto detects the scrolling container, so the option would be ignored. We can add a notice in the documentation about the option is only needed for v1.

I am using a reactive form and it does not recognize the default value of a "formControlName"

Not familiar with Angular, looking at the code and don't see any usages that could break in v2. Not sure what caused it.

@aheritier
Copy link

I didn't dig a lot.
Doing only the upgrade makes the editor empty (no visible error in the console, neither in the build).
It's loaded but data are not injected.

Screenshot 2024-04-22 at 07 41 32

@luin
Copy link
Author

luin commented Apr 22, 2024

@aheritier Good catch! Actually 2.0 changed the signature of clipboard.convert() that it now accepts an object with two fields html/text. Also, for getting the html content, it's recommended to use getSemanticHTML() since innerHTML may contain elements that are only needed for editing (e.g. toolbars).

Here's the changes that make the demo work for me:

diff --git a/src/app/components/editor/editor.ts b/src/app/components/editor/editor.ts
index 9e578437f..ab7c767ad 100755
--- a/src/app/components/editor/editor.ts
+++ b/src/app/components/editor/editor.ts
@@ -223,7 +223,7 @@ export class Editor implements AfterContentInit, ControlValueAccessor {
         if (this.quill) {
             if (value) {
                 const command = (): void => {
-                    this.quill.setContents(this.quill.clipboard.convert(this.value));
+                    this.quill.setContents(this.quill.clipboard.convert({ html: this.value }));
                 };
 
                 if (this.isAttachedQuillEditorToDOM) {
@@ -296,12 +296,12 @@ export class Editor implements AfterContentInit, ControlValueAccessor {
         });
 
         if (this.value) {
-            this.quill.setContents(this.quill.clipboard.convert(this.value));
+            this.quill.setContents(this.quill.clipboard.convert({ html: this.value }));
         }
 
         this.quill.on('text-change', (delta: any, oldContents: any, source: any) => {
             if (source === 'user') {
-                let html = DomHandler.findSingle(editorElement, '.ql-editor').innerHTML;
+                let html = this.quill.getSemanticHTML();
                 let text = this.quill.getText().trim();
                 if (html === '<p><br></p>') {
                     html = null;

@luin
Copy link
Author

luin commented Apr 29, 2024

Note that we could make the change backward compatible by checking Quill.version and only make the changes when Quill.version is present and it doesn't start with 1.

@jcoppieters
Copy link

We're part of Visma and our source code is automatically scanned by Aikido...
quill is used in prime-ng's p-editor... which we use in our main application.

Aikido reports quill as a security risk and advises to upgrade 1.3.7 => 2.0.0...
We did, it looked OK, but on save of our document the content isn't saved.

So now we either face security-point penalties or switching to a different editor...

@jcoppieters
Copy link

Any idea when (or if) the upgrade to quill 2.0 is planned?

@ThoSap
Copy link
Contributor

ThoSap commented May 3, 2024

I previously used the following workaround with PrimeNG 17.12.0 and Quill 2.0.0.

<p-editor (onTextChange)="textChange()" [(ngModel)]="content" [style]="{height: '400px'}"/>
content?: {
    html: string | undefined,
    text: string | undefined,
};

// In any method or service
this.content = { html: c.content, text: undefined });

// When editing/"saving"
// You do not necessarily need this callback in the `<p-editor/>`, the content variable will always have the last value due to the Angular model/two-way binding
textChange(): void {
  if (!this.content) {
    // Your logic here
  }

  // Workaround for Quill v2.0.X until PrimeNG supports it officially
  const htmlContent = this.content as unknown as string;
}

@evertonrivas
Copy link

I didn't dig a lot. Doing only the upgrade makes the editor empty (no visible error in the console, neither in the build). It's loaded but data are not injected.

Screenshot 2024-04-22 at 07 41 32

I have the same problem! How can I solve it?

@roliveiradvt
Copy link

roliveiradvt commented May 6, 2024

After upgrading to PrimeNG 17.13.0 the quill component is still broken for me:
image

I already tried to update quill to 2.0.1 but didn't do anything.
To be more specific, this issue only happens when you try to use the Editor inside a dialog

@Rodesc
Copy link

Rodesc commented May 6, 2024

I didn't dig a lot. Doing only the upgrade makes the editor empty (no visible error in the console, neither in the build). It's loaded but data are not injected.

I have the same problem! How can I solve it?

Using quill version 2.0+ with reactive forms introduces the bug that the value is not displayed correctly.
Here's how I fixed it temporarily while waiting for p-editor to be updated:

  1. Change the quill version to 1.3.7 in package.json
  2. npm i --reinstall

@Rodesc
Copy link

Rodesc commented May 6, 2024

After upgrading to PrimeNG 17.13.0 the quill component is still broken for me: image

I already tried to update quill to 2.0.1 but didn't do anything. To be more specific, this issue only happens when you try to use the Editor inside a dialog

Looks like you didn't load the css files correctly. See the bottom of this page: https://www.primefaces.org/primeng-v14-lts/editor

@roliveiradvt
Copy link

After upgrading to PrimeNG 17.13.0 the quill component is still broken for me: image
I already tried to update quill to 2.0.1 but didn't do anything. To be more specific, this issue only happens when you try to use the Editor inside a dialog

Looks like you didn't load the css files correctly. See the bottom of this page: https://www.primefaces.org/primeng-v14-lts/editor

image

They are there, the editor doesn't work ONLY IF it is inside the dialog.
I have another instance of the editor working in my application, just the one inside the dialog doesn't work (tried adding to other dialogs just to make sure that was the issue)

@vigouredelaruse
Copy link

vigouredelaruse commented May 6, 2024

i'm working on an angular library dependent on primeng ^17.x.x unfortunately blocked due to

  • optimization bailouts due to quill 1.3.7 dependency
  • extra ##.js files alongside scripts.js that need to be loaded when affected modules are initialized

as per

the error
node_modules\primeng\fesm2022\primeng-editor.mjs depends on 'quill'. CommonJS or AMD dependencies can cause optimization bailouts. For more info see: https://angular.io/guide/build#configuring-commonjs-dependencies

the error artifact (partial scripts.js minification) is generated whether or not primeng::EditorModule is imported
image

package.json at quill 2.0.1

    "primeng": "^17.16.0",
    "quill": "^2.0.1",
    "rxjs": ">=7.5.4",
    "tslib": ">=2.3.0",

looking forward to the successful pr validation

@EdManukyan
Copy link

@luin I have a question.
so in the old version and even in rc2 valueGetter: (quillEditor: QuillType, editorElement: HTMLElement) => string | any; method looked like this, but in the newer version and in the latest one quill has this change valueGetter: import("@angular/core").InputSignal<(quillEditor: QuillType) => string | any>; which is great that quill uses signals. In my code the usage is like this.

  editor: Quill;
  const editorValue = this.quillEditorComponent.valueGetter();

    let noChanges = editorValue(this.editor) === this.formControl.value;
    if (noChanges) return;

    // use case falsy values (example, new/temp entity)
    noChanges = !editorValue(this.editor) && !this.formControl.value;

which seems to be solution for my problem , but the test fails
test:

    beforeEach(() => {
      component.formControl = new UntypedFormControl('', { updateOn });
      markAsDirtySpy = spyOn(component.formControl, 'markAsDirty');
      setValueSpy = spyOn(component.formControl, 'setValue');
      qecValidateSpy = spyOn(component.quillEditorComponent, 'validate').and.returnValue(null);
      qecValueGetterSpy = spyOn(component.quillEditorComponent, 'valueGetter');
    });

    it('should update value', () => {
      component.ngOnDestroy();
      expect(markAsDirtySpy).toHaveBeenCalled();
      expect(setValueSpy).toHaveBeenCalledWith(expectedUpdateValue);
    });

error:
TypeError: editorValue is not a function
i was not able to find proper way of valueGetter() for version 2.0

@luin
Copy link
Author

luin commented May 28, 2024

@MartinaAnt I'm not the maintainer of this repo but the patch I proposed should make this library work with Quill 2.0

@cetincakiroglu cetincakiroglu added Type: Bug Issue contains a bug related to a specific component. Something about the component is not working and removed Type: Enhancement Issue contains an enhancement related to a specific component. Additional functionality has been add labels Jun 13, 2024
@cetincakiroglu cetincakiroglu modified the milestones: 18.0.0-rc.1, 17.18.1 Jun 13, 2024
@cetincakiroglu
Copy link
Contributor

Hi @luin,

Thanks for the support! We've added it into 18.0.0-rc.1

@cetincakiroglu cetincakiroglu added Type: Enhancement Issue contains an enhancement related to a specific component. Additional functionality has been add dependencies Issue or pull request is related to a dependency package and removed Type: Bug Issue contains a bug related to a specific component. Something about the component is not working labels Jun 13, 2024
@ThoSap
Copy link
Contributor

ThoSap commented Jun 18, 2024

@cetincakiroglu can you please include the pull request #15764 already with the PrimeNG 17.18.2 release?

I don't think anything speaks against it as this PR both supports Quill 1.X.X and 2.X.X, then we could use Angular 17 + PrimeNG 17.18.2 and Quill after the 25th of July 2024.

And this should probably also be backported to older PrimeNG LTS versions.

@tuankhoiutc260
Copy link

I previously used the following workaround with PrimeNG 17.12.0 and Quill 2.0.0.

<p-editor (onTextChange)="textChange()" [(ngModel)]="content" [style]="{height: '400px'}"/>
content?: {
    html: string | undefined,
    text: string | undefined,
};

// In any method or service
this.content = { html: c.content, text: undefined });

// When editing/"saving"
// You do not necessarily need this callback in the `<p-editor/>`, the content variable will always have the last value due to the Angular model/two-way binding
textChange(): void {
  if (!this.content) {
    // Your logic here
  }

  // Workaround for Quill v2.0.X until PrimeNG supports it officially
  const htmlContent = this.content as unknown as string;
}

I'm a newbie in Angular, it works for me. Thanks very much!

@cetincakiroglu cetincakiroglu modified the milestones: 18.0.0-rc.1, 17.18.3 Jul 2, 2024
cetincakiroglu added a commit that referenced this issue Jul 3, 2024
Fixed Issue #14721 (Add support for Quill 2.0)
@cetincakiroglu cetincakiroglu removed the Type: Enhancement Issue contains an enhancement related to a specific component. Additional functionality has been add label Jul 3, 2024
@ThoSap
Copy link
Contributor

ThoSap commented Jul 4, 2024

🥳🥳🥳
https://github.com/primefaces/primeng/releases/tag/17.18.3

Thanks @fabiancabau @luin @cetincakiroglu

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Issue or pull request is related to a dependency package
Projects
None yet
Development

Successfully merging a pull request may close this issue.