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

HtmlEmbed UI doesn't reflect isEnabled/isReadOnly state #10182

Closed
PhiR42 opened this issue Jul 20, 2021 · 5 comments · Fixed by #10682
Closed

HtmlEmbed UI doesn't reflect isEnabled/isReadOnly state #10182

PhiR42 opened this issue Jul 20, 2021 · 5 comments · Fixed by #10682
Assignees
Labels
package:html-embed squad:core Issue to be handled by the Core team. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@PhiR42
Copy link

PhiR42 commented Jul 20, 2021

📝 Provide detailed reproduction steps (if any)

  1. insert a HtmlEmbed block in the editor, type something and save
  2. switch the editor to isReadOnly mode
  3. It is now impossible to add new HtmlEmbed blocks to the content via the toolbar
  4. BUT the "pencil" button to edit existing blocks in the content still works and allows you to modify the html code inside the block.
  5. This is a UI bug rather that a functionnality bug since the "save" button on the htmlembed block is disabled, so it is indeed impossible to actually modify the content of the editor.

✔️ Expected result

The "pencil" button of the existing htmlembed blocks in the content should be in disabled state.

❌ Actual result

The "pencil" button of the existing htmlembed blocks is enabled and allows one to change the html code in the block (but not to save it)

@PhiR42 PhiR42 added the type:bug This issue reports a buggy (incorrect) behavior. label Jul 20, 2021
@FilipTokarski FilipTokarski added the squad:core Issue to be handled by the Core team. label Jul 21, 2021
@Reinmar
Copy link
Member

Reinmar commented Sep 8, 2021

We discussed this issue internally. Some (random) notes:

  • I want to prioritize it because command state is important for some features (comments only mode, e.g).
  • Assumption: It goes through commands. Is that correct?
  • It may not be a bug... people should perhaps have access to the source of the embed.
  • In this case, we should replace the icon, label and... well, review the UI. Big changes needed.
  • The source editing is disabled in read-only, though. In CKEditor 4 it is enabled but the source is read-only.
  • Plot twist: Link's data is not accessible. Hence, HTML embed's data may not be accessible too. Let's just disable the "Edit button" button (there's no command for that).
  • Same line as https://github.com/ckeditor/ckeditor5/blob/1fc2435/packages/ckeditor5-html-embed/src/htmlembedediting.js#L388-L389?

@dawidurbanski
Copy link
Contributor

Fixed in #10570

@Reinmar Reinmar modified the milestones: iteration 47, iteration 48 Sep 24, 2021
@mlewand mlewand changed the title HtmlEmbed UI isn't doesn't reflect isEnabled/isReadOnly state HtmlEmbed UI doesn't reflect isEnabled/isReadOnly state Sep 30, 2021
@oleq
Copy link
Member

oleq commented Oct 12, 2021

FYI: I prepared a PR with a ButtonView solution on top of a single unified HtmlEmbedCommand. Ready for review in #10682.

@oleq oleq self-assigned this Oct 12, 2021
@Reinmar
Copy link
Member

Reinmar commented Oct 13, 2021

Idea: Merge InsertHtmlEmbedCommand and UpdateHtmlEmbedCommand into one command, just like we have in the media embed feature. This will resolve the problem with the latter command (UpdateHtmlEmbedCommand) being disabled and thus disabling buttons.

But, this will mean that when the selection is e.g. in a table in which HTML embed is disallowed (by the schema) the "Edit" buttons on all widgets will be disabled.

Another idea: Have a separate command for updating a given widget (passed to execute()). It's an interesting option, in line with the broad problem that commands should be executable on passed selection (instead of being executable only on the document selection).

Let's ignore this problem, though, because the topic keeps growing beyond control.

Scope:

  • Merge the commands.
  • Use live instances of ButtonViews
  • Clean up detached instances of ButtonViews (ones used in removed widgets).

dawidurbanski added a commit that referenced this issue Oct 15, 2021
Fix (html-embed): Embed buttons should reflect the read-only state of the editor and the HTML embed command. Closes #10182.

MAJOR BREAKING CHANGE (html-embed): The InsertHtmlEmbedCommand and UpdateHtmlEmbedCommand have been replaced by HtmlEmbedCommand which is now responsible for both tasks. The command can be executed via editor.execute( 'htmlEmbed' ). See the API reference for more information.
@dawidurbanski
Copy link
Contributor

Closed in #10682

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment