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

[uiActions] Improve context menu keyboard support #70705

Merged
merged 6 commits into from
Jul 13, 2020

Conversation

Dosant
Copy link
Contributor

@Dosant Dosant commented Jul 3, 2020

Summary

Closes #67130

  • Improves position resolution logic by also tracking last clicked element.
  • Adds ownFocus prop, so can pick menu item with keyboard.
  • Also track if target element was removed from DOM. In that case tries to use previous element. won't work all the time, but works nicely in case context menu trigger by item in other context menu. (see gif below)

Opened menu with keyboard:

Screenshot 2020-07-03 at 13 58 01

If trigger element removed from DOM, reuses previous trigger if possible:

Jul-03-2020 15-30-28

Release notes

Multiple chart actions context menu positioning fixes.

Checklist

Delete any items that are not applicable to this PR.

@Dosant Dosant added bug Fixes for quality problems that affect the customer experience Project:Accessibility Team:AppArch v7.9.0 v8.0.0 Feature:UIActions UI actions. These are client side only, not related to the server side actions.. labels Jul 3, 2020
@Dosant Dosant marked this pull request as ready for review July 3, 2020 13:38
@Dosant Dosant requested a review from a team as a code owner July 3, 2020 13:38
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@Dosant
Copy link
Contributor Author

Dosant commented Jul 7, 2020

@elasticmachine merge upstream

@streamich
Copy link
Contributor

In mouse event list logic seems it is open the menu at a click one before the last on Lens pie chart visualization.

ui_actions_menu

In determining position of the context menu, I thought we wanted to emit the element and location of the click in the context object that trigger emits, curious why you opted for mouse detection logic instead?

@Dosant
Copy link
Contributor Author

Dosant commented Jul 13, 2020

@streamich, thanks for catching that 👍 should be fixed.

In determining position of the context menu, I thought we wanted to emit the element and location of the click in the context object that trigger emits, curious why you opted for mouse detection logic instead?

As noted in private discussion:

changing the api vs slightly enhancing current implementation internally.
until we not hit blocker with current implementation, don’t think we have to change the api.
Also didn’t look into how much work is that to pass dom event everywhere trigger.exec is used. I imagine, in consumer code they could be separate by multiple layers

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Contributor

@streamich streamich left a comment

Choose a reason for hiding this comment

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

LGTM, checked on Mac Chrome/Firefox.

@Dosant
Copy link
Contributor Author

Dosant commented Jul 13, 2020

While testing we found this bug in elastic-charts: elastic/elastic-charts#748

@Dosant Dosant merged commit f4b4dc5 into elastic:master Jul 13, 2020
@Dosant Dosant deleted the bugfix/ui-actions-menu-position branch July 13, 2020 12:55
Dosant added a commit to Dosant/kibana that referenced this pull request Jul 13, 2020
* Improves position resolution logic by also tracking last clicked element.
* Adds ownFocus prop, so can pick menu item with keyboard.
* Also track if target element was removed from DOM. In that case tries to use previous element. won't work all the time, but works nicely in case context menu trigger by item in other context menu.

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Dosant added a commit that referenced this pull request Jul 13, 2020
* Improves position resolution logic by also tracking last clicked element.
* Adds ownFocus prop, so can pick menu item with keyboard.
* Also track if target element was removed from DOM. In that case tries to use previous element. won't work all the time, but works nicely in case context menu trigger by item in other context menu.

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:UIActions UI actions. These are client side only, not related to the server side actions.. Project:Accessibility release_note:fix v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UI Actions popover not working properly with keyboard
4 participants