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

Removing the first hidden toolbar button throws an exception #7655

Closed
mlewand opened this issue Jul 20, 2020 · 0 comments · Fixed by #7656
Closed

Removing the first hidden toolbar button throws an exception #7655

mlewand opened this issue Jul 20, 2020 · 0 comments · Fixed by #7656
Assignees
Labels
intro Good first ticket. package:ui type:bug This issue reports a buggy (incorrect) behavior.

Comments

@mlewand
Copy link
Contributor

mlewand commented Jul 20, 2020

📝 Provide detailed reproduction steps (if any)

  1. Go to https://ckeditor.com/docs/ckeditor5/20.0.0/examples/builds/document-editor.html
  2. Resize the window in a way that at least one button gets hidden.

  1. Use the following code to remove the first hidden button:
const toolbarView = editor.ui.view.toolbar;
const visibleButtonsCount = toolbarView._behavior.ungroupedItems.length;
const firstHiddenItem = toolbarView.items.get( visibleButtonsCount );
toolbarView.items.remove( firstHiddenItem );

✔️ Expected result

The first hidden toolbar button gets removed, and (if possible) one of hidden buttons gets "promoted" to the toolbar button.

❌ Actual result

An error is thrown:

Uncaught CKEditorError: collection-remove-404: Item not found. Read more: https://ckeditor.com/docs/ckeditor5/latest/framework/guides/support/error-codes.html#error-collection-remove-404

    at ViewCollection._remove (http://localhost:8125/ckeditor5/tests/manual/all-features.js:143318:10)
    at ViewCollection.remove (http://localhost:8125/ckeditor5/tests/manual/all-features.js:142909:32)
    at ViewCollection.<anonymous> (http://localhost:8125/ckeditor5/tests/manual/all-features.js:138818:25)
    at ViewCollection.fire (http://localhost:8125/ckeditor5/tests/manual/all-features.js:146827:30)
    at ViewCollection._remove (http://localhost:8125/ckeditor5/tests/manual/all-features.js:143328:8)
    at ViewCollection.remove (http://localhost:8125/ckeditor5/tests/manual/all-features.js:142909:32)
    at <anonymous>:1:19

📃 Other details

That's a typical OBOE originating in this line:

if ( index > this.ungroupedItems.length ) {

It's enough to change it to if ( index >= this.ungroupedItems.length ) {

There's a similar situation with adding buttons

if ( index > this.ungroupedItems.length ) {
but it doesn't throw an error, instead _updateGrouping() call that follows it cleans up the structure, and no problem is visible.


If you'd like to see this fixed sooner, add a 👍 reaction to this post.

@mlewand mlewand added type:bug This issue reports a buggy (incorrect) behavior. squad:red intro Good first ticket. labels Jul 20, 2020
@mlewand mlewand changed the title Removing last toolbar button throws an exception Removing the first hidden toolbar button throws an exception Jul 20, 2020
mlewand added a commit that referenced this issue Jul 21, 2020
mlewand added a commit that referenced this issue Jul 21, 2020
@mlewand mlewand self-assigned this Jul 21, 2020
@oleq oleq added this to the iteration 34 milestone Jul 22, 2020
oleq added a commit that referenced this issue Jul 22, 2020
Other (ui): Improved toolbar rendering time when multiple items are added or removed at once (e.g. during editor initialization). Closes #6194.

Fix (ui): Removing the first hidden (grouped) toolbar button should not throw an exception. Closes #7655.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
intro Good first ticket. package:ui type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants