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

Toolbar 'Show more items' button is focused after clicking a button from it #12178

Closed
Dumluregn opened this issue Jul 28, 2022 · 19 comments · Fixed by #12319
Closed

Toolbar 'Show more items' button is focused after clicking a button from it #12178

Dumluregn opened this issue Jul 28, 2022 · 19 comments · Fixed by #12319
Assignees
Labels
domain:accessibility This issue reports an accessibility problem. package:ui squad:features Issue to be handled by the Features team. type:bug This issue reports a buggy (incorrect) behavior. type:regression This issue reports a bug that was not present in the previous releases.

Comments

@Dumluregn
Copy link
Contributor

📝 Provide detailed reproduction steps (if any)

  1. Open full features demo http://localhost:8125/ckeditor5/tests/manual/all-features.html
  2. Reduce the width of the editor so that some buttons are grouped.
  3. Click a button that should move the focus to the editable (e.g. choose language) from the collapsed part of toolbar.

✔️ Expected result

Focus is moved to the editable.

❌ Actual result

Focus is moved to the "Show more items" toolbar button.

❓ Possible solution

It is a regression introduced in #12162. Due to moving the focus when any dropdown is closed, it also affects the toolbar dropdown which is closed after the button's command is executed, so it focuses the toolbar back.

The problem occurs on every type of button - generic one, dropdown and splitbutton.

📃 Other details


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

@Dumluregn Dumluregn added type:bug This issue reports a buggy (incorrect) behavior. package:ui domain:accessibility This issue reports an accessibility problem. type:regression This issue reports a bug that was not present in the previous releases. labels Jul 28, 2022
@mlewand mlewand added the squad:features Issue to be handled by the Features team. label Jul 29, 2022
@CKEditorBot CKEditorBot added the status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. label Jul 29, 2022
@Dumluregn
Copy link
Contributor Author

A cheap and dirty idea could be checking the document.activeElement when dropdown is being closed and don’t focus the button if editable is focused, but I’m not sure if it covers all the cases.

@Reinmar
Copy link
Member

Reinmar commented Jul 29, 2022

Could you make a draft PR with this?

@Dumluregn
Copy link
Contributor Author

Yes, I'm on it.

@Dumluregn
Copy link
Contributor Author

PoC: ck/12178-grouped-toolbar-focus. cc @Reinmar @mlewand

@Reinmar
Copy link
Member

Reinmar commented Jul 29, 2022

cc @ckeditor/qa-team – could you verify this idea?

@Mgsy
Copy link
Member

Mgsy commented Aug 3, 2022

@FilipTokarski @Acrophost could you please take a look at it ☝️? Tests should cover:

  • Desktop browsers,
  • Mobile devices,
  • All editor types,
  • Different toolbars (the main toolbar, the floating toolbar, the contextual toolbar)

@mlewand
Copy link
Contributor

mlewand commented Aug 3, 2022

@Mgsy @FilipTokarski @Acrophost please check whether this issue also solves #12186 - because there's a good chance it does. (in this case please update PR so that it closes both issues)

@FilipTokarski

This comment was marked as resolved.

@FilipTokarski

This comment was marked as duplicate.

@Acrophost

This comment was marked as duplicate.

@Acrophost

This comment was marked as duplicate.

@Dumluregn
Copy link
Contributor Author

Most of the issues above seem to be the same as #12207, only the reproduction steps are a bit different (using toolbar buttons) - @FilipTokarski @Acrophost please verify it and hide the proper comments if that's the case.

@FilipTokarski
Copy link
Member

Thanks, you're right 👍 I'll leave the one with inserting image, as it looks a bit different because the focus is still in the toolbar.

@Dumluregn
Copy link
Contributor Author

Yes, the image button doesn't focus the editable - https://github.com/ckeditor/ckeditor5/blob/master/packages/ckeditor5-image/src/imageupload/imageuploadui.js#L57-L63. I think it will be worth an actual issue.

@Acrophost
Copy link

In addition to the image button issue - in case where we are using a balloon toolbar, the toolbar hides after inserting an image, but the image is not getting selected (we're losing selection overall)

no.toolbar.no.selection.2022-08-4.o.16.46.45.mov

@mlewand

This comment was marked as resolved.

@Acrophost
Copy link

I've tested the fix on different browsers, devices and editor types - for now I don't see any more issues with focus aside from ones that are already reported.

@Dumluregn
Copy link
Contributor Author

Thanks QA, I'll create a PR with the fix.

@Dumluregn Dumluregn self-assigned this Aug 8, 2022
@CKEditorBot CKEditorBot added status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. and removed status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. labels Aug 8, 2022
@mlewand
Copy link
Contributor

mlewand commented Aug 9, 2022

The #12178 (comment) should be fixed by #12229 (on the latest master).

@oleq oleq self-assigned this Aug 29, 2022
@Dumluregn Dumluregn removed their assignment Aug 30, 2022
Dumluregn added a commit that referenced this issue Sep 1, 2022
…s-v2

Fix (ui): DropdownView should close when it gets blurred. Also, DropdownView should focus its #buttonView only when it gets closed and the focus was inside its panelView. Closes #12178. Closes #12005.
Fix (media-embed): The focus will move to the editor after inserting a media. Closes #12186.
@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Sep 1, 2022
@CKEditorBot CKEditorBot added this to the iteration 57 milestone Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:accessibility This issue reports an accessibility problem. package:ui squad:features Issue to be handled by the Features team. type:bug This issue reports a buggy (incorrect) behavior. type:regression This issue reports a bug that was not present in the previous releases.
Projects
None yet
8 participants