Skip to content

[uiActions] Improve context menu keyboard support#70705

Merged
Dosant merged 6 commits intoelastic:masterfrom
Dosant:bugfix/ui-actions-menu-position
Jul 13, 2020
Merged

[uiActions] Improve context menu keyboard support#70705
Dosant merged 6 commits intoelastic:masterfrom
Dosant:bugfix/ui-actions-menu-position

Conversation

@Dosant
Copy link
Copy Markdown
Contributor

@Dosant Dosant commented Jul 3, 2020 •
edited
Loading

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
Copy Markdown
Contributor

elasticmachine commented Jul 3, 2020

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

@Dosant Dosant requested review from stacey-gammon and streamich July 3, 2020 13:38
@Dosant
Copy link
Copy Markdown
Contributor Author

Dosant commented Jul 7, 2020

@elasticmachine merge upstream

@streamich
Copy link
Copy Markdown
Contributor

streamich commented Jul 9, 2020

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
Copy Markdown
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
Copy Markdown
Contributor

kibanamachine commented Jul 13, 2020

💚 Build Succeeded

Build metrics

✅ unchanged

History

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

streamich
streamich approved these changes Jul 13, 2020
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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 Dosant mentioned this pull request Jul 13, 2020
Merged
Dosant added a commit to Dosant/kibana that referenced this pull request Jul 13, 2020
@Dosant @elasticmachine
[uiActions] Improve context menu keyboard support (elastic#70705)
3ddf4bf 
* 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
@Dosant @elasticmachine
[uiActions] Improve context menu keyboard support (#70705) (#71435)
7bcc84d 
* 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>
@darnautov darnautov mentioned this pull request Jul 15, 2020
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stacey-gammon stacey-gammon Awaiting requested review from stacey-gammon

1 more reviewer

@streamich streamich streamich approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

UI Actions popover not working properly with keyboard

4 participants

@Dosant @elasticmachine @streamich @kibanamachine