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

WidgetTypeAround should use insertContent in order to integrate with track changes #7229

Closed
Reinmar opened this issue May 19, 2020 · 9 comments · Fixed by #7237
Closed

WidgetTypeAround should use insertContent in order to integrate with track changes #7229

Reinmar opened this issue May 19, 2020 · 9 comments · Fixed by #7237
Assignees
Labels
package:widget squad:magic type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@Reinmar
Copy link
Member

Reinmar commented May 19, 2020

📝 Provide a description of the improvement

It uses writer.insert() while it should use model.insertContent().

In addition, it should probably copy the implementation of the current <kbd>Enter</kbd> handler that we have for widgets. AFAICS it does some more checks.


If you'd like to see this improvement implemented, add a 👍 reaction to this post.

@Reinmar Reinmar added the type:improvement This issue reports a possible enhancement of an existing feature. label May 19, 2020
@Reinmar
Copy link
Member Author

Reinmar commented May 19, 2020

PS. I'm not entirely sure whether it's possible to insert a new empty paragraph with insertContent(). It may get merged automatically to the previous block. Let me know if that happens.

@Reinmar
Copy link
Member Author

Reinmar commented May 19, 2020

PPS. The Enter/Shift+Enter support uses writer.insert(), however it works with track changes because there's a custom integration for these two handlers. If we'll use writer.insertContent() for WidgetTypeAround (if it can be used), then track changes will integrate well automatically.

@Reinmar Reinmar added this to the iteration 32 milestone May 19, 2020
@Reinmar
Copy link
Member Author

Reinmar commented May 19, 2020

I asked @ckeditor/qa-team to help us verify that it will resolve the issue on the CF side. Here, let's focus on whether we can use insertContent() as I'm not sure about that unfortunately.

@Reinmar
Copy link
Member Author

Reinmar commented May 19, 2020

I talked with @Mgsy and it turned out that Enter isn't handled by track changes (so, there's no custom handler apparently) and that using insertContent() does not solve this issue.

Let's wait for @scofalik.

@scofalik
Copy link
Contributor

scofalik commented May 19, 2020

There is integration for enter command and there is integration for insertContent(). See integrations/entercommand.js and trackchangesediting.js.

However, there's some kind of an error if you simply add a single paragraph 🤔

editor.model.change( writer => {
    const p = writer.createElement( 'paragraph' );
    editor.model.insertContent( p, writer.createPositionAt( editor.model.document.getRoot(), 0 ) );
} );

Most probably, the range is set in an incorrect way. I can't guarantee that we will be able to fix that in this iteration.

EDIT: It works if you add paragraph with some text.

@Mgsy
Copy link
Member

Mgsy commented May 19, 2020

There is integration for enter command and there is integration for insertContent(). See integrations/entercommand.js and trackchangesediting.js.

Note to clarify: Enter command is handled properly, however, not in a context of widgets - after pressing Enter/Shift+Enter, a new paragraph is inserted, instead of a suggestion.

@scofalik
Copy link
Contributor

I see @oleq that you created InsertParagraphCommand. I assume you plan to use it also for enter/shift+enter behaviour when selection is on widget?

This can work. When you are done, could you please add an issue in collaboration features repository so we can work on it and have it done hopefully this iteration?

@oleq
Copy link
Member

oleq commented May 20, 2020

In a F2F talk with @scofalik we decided:

  • to go with the implementation as in #6775: Integrated widget type around feature with read–only, restricted editing and track changes #7237,
  • because the Track Changes integration with the insertParagraph command will take some time and it is not clear whether it is possible in this iteration, I will create a quick fix that hides the type around UI in TC mode:
    • WidgetTypeAround plugin will listen to InsertParagraphCommand#isEnabled (which is false in TC mode) and add a CSS class to the root that will make the type around buttons disappear
    • we will revert the CSS class approach in the next iteration when a proper integration arrives

@oleq
Copy link
Member

oleq commented May 20, 2020

FYI: The quick fix landed in 005a5d2.

niegowski added a commit that referenced this issue May 21, 2020
Internal (widget): Integrated the `WidgetTypeAround` plugin with the read–only mode. Closes #6775.

Internal (widget): Integrated the `WidgetTypeAround` plugin with the `RestrictedEditingMode` plugin (see #6775).

Feature (paragraph): Implemented the `InsertParagraphCommand` registered as `'insertParagraph'` in the editor. Closes #6823. Closes #7229.

Docs (engine): Improved `Model#insertContent` API documentation (see #6775).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:widget squad:magic type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
4 participants