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

Addressed issues related to closing dropdowns and "Show more items" menu in the toolbar #12319

Merged
merged 26 commits into from
Sep 1, 2022

Conversation

oleq
Copy link
Member

@oleq oleq commented Aug 23, 2022

Suggested merge commit message (convention)

Fix (ui): DropdownView should close when it gets blurred. Also, DropdownView should focus its #buttonView when it gets closed and the focus was inside its panelView. Closes #12178. Closes #12186. Closes #12005.

To be verified: Does it resolve #12005?

  • Confirmed

Additional information

This is a successor to #12239. It fixes both #12178 and #12186.

In this PR I did some considerable refactoring. I moved some logic from DropdownView to individual behaviors in dropdown utils. The assumption is that the bare DropdownView should be just a dumb button+panel that offers a lot of flexibility for developers who use it in different ways (e.g. see the formatting dropdown example in docs).

@Reinmar
Copy link
Member

Reinmar commented Aug 24, 2022

If I understand correctly: if the addDefaultBehavior() was used, by default we're still focusing the button. And we still expect this to be overridden in dropdown#execute event listener if needed?

While cleaning up the "where to put code that overrides the default behavior" problem in the previous iteration I thought that perhaps this could be approached differently (although, that would be a precedent) – e.g. via some #execute's event property like data.moveFocusTo. Do you think it could be a next step to clean up the way how dropdowns are used?

@oleq
Copy link
Member Author

oleq commented Aug 24, 2022

If I understand correctly: if the addDefaultBehavior() was used, by default we're still focusing the button. And we still expect this to be overridden in dropdown#execute event listener if needed?

While cleaning up the "where to put code that overrides the default behavior" problem in the previous iteration I thought that perhaps this could be approached differently (although, that would be a precedent) – e.g. via some #execute's event property like data.moveFocusTo. Do you think it could be a next step to clean up the way how dropdowns are used?

Let's have a meeting about this maybe?

@oleq
Copy link
Member Author

oleq commented Aug 24, 2022

Besides, I just noticed (CI) that there are some adjustments necessary in other packages.

@Dumluregn
Copy link
Contributor

I checked the PR functionally and everything works fine in all editor types. I confirm it also fixes #12005 as mentioned in the PR comment. As we discussed, I'm not diving into the code review until the QA team approves it too.

@oleq
Copy link
Member Author

oleq commented Aug 29, 2022

Can we have some feedback on this PR please @ckeditor/qa-team?

Scope:

  • In all editor types...
  • when drop-downs are either 
    • in the visible part of the toolbar,
    • under "Show more items" button
  • we need to test opening various dropdowns (media, insert table, insert image)...
  • and then closing them by either
    • executing the command (inserting media, inserting table, etc.)
    • clicking somewhere outside
    • using escape key
  • to make sure both the feature (media, table, etc.) dropdown gets closed and the "Show more items" dropdown/toolbar gets closed.

There's also #12005 on top of that.

Thanks 🙏

Copy link
Contributor

@Dumluregn Dumluregn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I left some discussion comments but they are not mandatory from my perspective. Still we're waiting for the QA check before merging the PR.

packages/ckeditor5-ui/tests/dropdown/utils.js Outdated Show resolved Hide resolved
packages/ckeditor5-ui/tests/dropdown/utils.js Show resolved Hide resolved
packages/ckeditor5-ui/src/dropdown/utils.js Outdated Show resolved Hide resolved
@dufipl
Copy link
Contributor

dufipl commented Aug 30, 2022

  • using escape key
  • to make sure both the feature (media, table, etc.) dropdown gets closed and the "Show more items" dropdown/toolbar gets closed.

@oleq, both using the feature and clicking somewhere else close both the feature (media, table, etc.) dropdown and the "Show more items" dropdown/toolbar. However, the escape key closes only the feature dropdown/toolbar returning focus back to the "Show more items" dropdown/toolbar. In the context of the above quote, should I consider it incorrect behaviour?

@Acrophost
Copy link

Acrophost commented Aug 30, 2022

Special characters dropdown closes when clicked between the character buttons. In case of balloon/inline editor, the toolbar closes as well.

Steps to make reproducing easier:

  1. Open special characters dropdown - editor and browser is meaningless
  2. Click in the middle of opened dropdown, somewhere between the buttons (or on dropdown footer/header outside of character categories select)
Screen.Recording.2022-08-30.at.16.31.07.mov

On master or release branches the dropdown doesn't close, so this part is a regression, but the editor is getting blurred as well (which doesn't happen when clicking similarly on other dropdowns).

@dufipl
Copy link
Contributor

dufipl commented Aug 31, 2022

With @Acrophost we have finished testing and we've not found any more issues with this PR beyond the ones above.

@oleq
Copy link
Member Author

oleq commented Aug 31, 2022

However, the escape key closes only the feature dropdown/toolbar returning focus back to the "Show more items" dropdown/toolbar. In the context of the above quote, should I consider it incorrect behaviour?

This is correct, sorry for not being specific. Esc should close UI elements "one by one" until you reach the editing root. Clicking the editing root should close everything at once.

@oleq
Copy link
Member Author

oleq commented Aug 31, 2022

Special characters dropdown closes when clicked between the character buttons. In case of balloon/inline editor, the toolbar closes as well.

Addressed in d3a4bb7.

@oleq
Copy link
Member Author

oleq commented Aug 31, 2022

Let's have another look at this PR @Dumluregn 🙂

@Dumluregn Dumluregn self-requested a review September 1, 2022 08:16
…y run the same in the windowed and window-less env.
@oleq oleq requested a review from Dumluregn September 1, 2022 13:40
Copy link
Contributor

@Dumluregn Dumluregn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I think the code of DropdownView class looks much cleaner now with all the logic moved to the utils.

@Dumluregn Dumluregn merged commit e8a33da into master Sep 1, 2022
@Dumluregn Dumluregn deleted the ck/12178-grouped-toolbar-focus-v2 branch September 1, 2022 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants