Fix automation picker overflow menu for keyboard #21048

karwosts · 2024-06-09T16:44:24Z

Proposed change

The current overflow menu in a lot of the new data tables cannot be used with the keyboard, as far as I can tell. Both enter or space close the menu without doing anything, neither of them trigger the click event. That seems like it is only triggered for <a> links, and only for Enter.

Just reading https://material-web.dev/components/menu/ it seems like maybe @close-event could be a reasonable event to use instead of @click so it can have keyboard support? The only drawback is it is also triggered by the escape key, which I've added a helper event to prevent actions on that.

I'm not clear if this is intentional behavior of md-menu-item, could be slightly related to issue 5577 from github.com/material-components/material-web, but I'm not sure if a fix there would necessarily fix this either.

There are many menus that are affected by this, but I'll just start with this one to discuss a good pattern for a solution.

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New feature (thank you!)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Example configuration

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue or discussion: Accessibility: unable to open/view automation details via the keyboard #21028
Link to documentation pull request:

Checklist

The code change is tested and works locally.
There is no commented out code in this PR.
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

Summary by CodeRabbit

New Features
- Added event listeners to handle close-menu events in the navigation menu.
- Introduced a clickAction property for custom actions in menu items.
Improvements
- Enhanced clarity and context maintenance in automation picker by replacing direct event handlers with arrow functions.

bramkragten · 2024-06-10T07:12:50Z

src/panels/config/automation/ha-automation-picker.ts

@@ -898,6 +906,12 @@ class HaAutomationPicker extends SubscribeMixin(LitElement) {
    `;
  }

+  private _handleMenuClose(ev) {
+    if (ev.detail.reason.key === "Escape") return;
+    const action = ev.currentTarget.action.bind(this);


Instead of using bind, can we make the action functions arrow functions?

I wonder if we should use a switch/case with a string action to call the right function instead of binding the function on the element.

Instead of using target or currentTarget, use initiator defined in the custom event detail. Then I wonder if the key/reason check is even necessary as hopefully that would be null or undefined unless the user actually took the action with space or enter?

The initiator is still ha-menu-item when canceling the menu with Escape key.

src/panels/config/automation/ha-automation-picker.ts

steverep

I think this proposed change is way too much of a hack. It also presumes that escape is the only way to close the menu, which may not be the case at all, and then lead to activation the user didn't take.

MDMenuItem extends MDListItem, and so my guess here would be that the simple fix should be to use the @request-activation event instead of @click. I'm dealing with some data loss of my docker volumes so I cannot test that ATM.

The same problem exists when listening to click instead of request-selected on mwc-list-item, so this is likely just a continuation of that with Material 3. I fixed many of those but many new ones using click have popped up or regressed. For the record, that's why I've taken a hiatus from a11y recently - I'm working on something big to hopefully prevent such regressions in the future.

karwosts · 2024-06-10T16:58:15Z

I might be missing something, but listening for request-activation on a ha-menu-item does not seem to ever fire (not with mouse, not with any combination of keyboard keystrokes I can find)

steverep

If request-activation doesn't work, then I guess we're stuck with this approach until material-components/material-web#5577 gets resolved. To make it less fluffy on each instance, my suggestions are to move the crux of the handling to the element classes, and just require the instances to set the action to take.

src/panels/config/automation/ha-automation-picker.ts

steverep · 2024-06-11T03:15:12Z

src/panels/config/automation/ha-automation-picker.ts

@@ -898,6 +906,12 @@ class HaAutomationPicker extends SubscribeMixin(LitElement) {
    `;
  }

+  private _handleMenuClose(ev) {
+    if (ev.detail.reason.key === "Escape") return;
+    const action = ev.currentTarget.action.bind(this);


Instead of using target or currentTarget, use initiator defined in the custom event detail. Then I wonder if the key/reason check is even necessary as hopefully that would be null or undefined unless the user actually took the action with space or enter?

src/components/ha-menu.ts

steverep

Other than the few suggestions below, LGTM. If you can swing it quick, I'll approve so we can fix this for 2024.7.

steverep · 2024-07-02T16:02:20Z

src/components/ha-menu-item.ts


 @customElement("ha-menu-item")
 export class HaMenuItem extends MdMenuItem {
+  @property() closeAction?: (ev: CloseMenuEvent) => void;


A few suggestions to improve the API:

Turn off the attribute. I don't know why lit-analyzer doesn't flag that function props shouldn't be attributes.

Make the parameter the triggering item instead of the event, and make it optional as many handlers won't need it.

To make the purpose much clearer where used in instances, maybe rename to clickAction or just action. @bramkragten any strong opinion here?

Suggested change

@property() closeAction?: (ev: CloseMenuEvent) => void;

@property({ attribute: false }) clickAction?: (item?: HTMLElement) => void;

What should be passed as the item here, the ev.detail.initiator?

src/components/ha-menu.ts

steverep · 2024-07-02T16:22:30Z

src/panels/config/automation/ha-automation-picker.ts

@@ -1051,28 +1051,28 @@ class HaAutomationPicker extends SubscribeMixin(LitElement) {
    this._applyFilters();
  }

-  private _showInfo(ev) {
-    const automation = ev.currentTarget.parentElement.anchorElement.automation;
+  private _showInfo = (ev) => {


...then type the parameter as HaMenuItem (trying to cut down on the way too many instances of implicit any types we have).

I will say that typing this is getting kind of ugly. I have to cast the parent element, then cast the anchorElement back to any as the automation property does not actually exist on any type, it's just a custom added thing here.

const automation = ((item.parentElement as HaMenu)!.anchorElement as any)!.automation;

Is this going to be acceptable or need to clean it up somehow?

I think it's fine for now. The best goal would be to turn on noImplicitAny in tsconfig. Casting as any is never great, but that's a different rule for another day.

coderabbitai

Actionable comments posted: 6

Outside diff range comments (4)

src/panels/config/automation/ha-automation-picker.ts (4)
Line range hint 1078-1100: Avoid non-null assertions and any type.

Consider using optional chaining and proper type annotations for better type safety.
- const automation = ((item.parentElement as HaMenu)!.anchorElement as any)!.automation;
+ const automation = (item.parentElement as HaMenu)?.anchorElement?.automation as AutomationEntity;
Tools

Biome

[error] 1056-1056: Forbidden non-null assertion.

Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator

(lint/style/noNonNullAssertion)

[error] 1056-1056: Forbidden non-null assertion.

Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator

(lint/style/noNonNullAssertion)

[error] 1056-1056: Unexpected any. Specify a different type.

any disables many type checking rules. Its use should be avoided.

(lint/suspicious/noExplicitAny)

[error] 1062-1062: Forbidden non-null assertion.

Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator

(lint/style/noNonNullAssertion)

[error] 1062-1062: Forbidden non-null assertion.

Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator

(lint/style/noNonNullAssertion)

[error] 1062-1062: Unexpected any. Specify a different type.

any disables many type checking rules. Its use should be avoided.

(lint/suspicious/noExplicitAny)

[error] 1072-1072: Forbidden non-null assertion.

Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator

(lint/style/noNonNullAssertion)

[error] 1072-1072: Forbidden non-null assertion.

Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator

(lint/style/noNonNullAssertion)

[error] 1072-1072: Unexpected any. Specify a different type.

any disables many type checking rules. Its use should be avoided.

(lint/suspicious/noExplicitAny)

[error] 1079-1079: Forbidden non-null assertion.

Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator

(lint/style/noNonNullAssertion)

[error] 1079-1079: Forbidden non-null assertion.

Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator

(lint/style/noNonNullAssertion)

[error] 1079-1079: Unexpected any. Specify a different type.

any disables many type checking rules. Its use should be avoided.

(lint/suspicious/noExplicitAny)

Line range hint 1102-1117: Avoid non-null assertions and any type.

Consider using optional chaining and proper type annotations for better type safety.
- const automation = ((item.parentElement as HaMenu)!.anchorElement as any)!.automation;
+ const automation = (item.parentElement as HaMenu)?.anchorElement?.automation as AutomationEntity;
Line range hint 1129-1146: Avoid non-null assertions and any type.

Consider using optional chaining and proper type annotations for better type safety.
- const automation = ((item.parentElement as HaMenu)!.anchorElement as any)!.automation;
+ const automation = (item.parentElement as HaMenu)?.anchorElement?.automation as AutomationEntity;
Line range hint 1166-1192: Avoid non-null assertions and any type.

Consider using optional chaining and proper type annotations for better type safety.
- const automation = ((item.parentElement as HaMenu)!.anchorElement as any)!.automation;
+ const automation = (item.parentElement as HaMenu)?.anchorElement?.automation as AutomationEntity;

src/components/ha-menu.ts

src/panels/config/automation/ha-automation-picker.ts

Fix automation picker overflow menu for keyboard

9f4ece6

home-assistant bot added the cla-signed label Jun 9, 2024

karwosts added the Accessibility Related to accessibility for people with disabilities label Jun 9, 2024

This comment was marked as off-topic.

Sign in to view

bramkragten reviewed Jun 10, 2024

View reviewed changes

src/panels/config/automation/ha-automation-picker.ts Outdated Show resolved Hide resolved

bramkragten reviewed Jun 10, 2024

View reviewed changes

src/panels/config/automation/ha-automation-picker.ts Outdated Show resolved Hide resolved

steverep reviewed Jun 10, 2024

View reviewed changes

steverep reviewed Jun 11, 2024

View reviewed changes

some updates from review

2b8eb87

This comment was marked as off-topic.

Sign in to view

steverep reviewed Jun 13, 2024

View reviewed changes

src/components/ha-menu.ts Outdated Show resolved Hide resolved

src/components/ha-menu.ts Show resolved Hide resolved

src/components/ha-menu.ts Outdated Show resolved Hide resolved

src/components/ha-menu.ts Outdated Show resolved Hide resolved

typing

a41ad9c

This comment was marked as off-topic.

Sign in to view

no removeEventListener

705fbea

karwosts mentioned this pull request Jun 24, 2024

fix(data-tables): accessibility for row enter #21159

Closed

6 tasks

steverep reviewed Jul 2, 2024

View reviewed changes

updates from review

3f37eff

coderabbitai bot reviewed Jul 3, 2024

View reviewed changes

steverep approved these changes Jul 3, 2024

View reviewed changes

steverep merged commit e332364 into home-assistant:dev Jul 3, 2024
13 checks passed

steverep added this to the 2024.7 milestone Jul 3, 2024

karwosts deleted the auto-picker-menu-fix-keyboard branch July 3, 2024 16:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix automation picker overflow menu for keyboard #21048

Fix automation picker overflow menu for keyboard #21048

karwosts commented Jun 9, 2024 •

edited by coderabbitai bot

Loading

This comment was marked as off-topic.

This comment was marked as off-topic.

bramkragten Jun 10, 2024 •

edited

Loading

piitaya Jun 10, 2024

steverep Jun 11, 2024

karwosts Jun 11, 2024

steverep left a comment

karwosts commented Jun 10, 2024

steverep left a comment

steverep Jun 11, 2024

This comment was marked as off-topic.

This comment was marked as off-topic.

steverep left a comment

steverep Jul 2, 2024

karwosts Jul 3, 2024

steverep Jul 3, 2024

steverep Jul 2, 2024

karwosts Jul 3, 2024

steverep Jul 3, 2024

coderabbitai bot left a comment

	@property() closeAction?: (ev: CloseMenuEvent) => void;
	@property({ attribute: false }) clickAction?: (item?: HTMLElement) => void;

Fix automation picker overflow menu for keyboard #21048

Fix automation picker overflow menu for keyboard #21048

Conversation

karwosts commented Jun 9, 2024 • edited by coderabbitai bot Loading

Proposed change

Type of change

Example configuration

Additional information

Checklist

Summary by CodeRabbit

This comment was marked as off-topic.

This comment was marked as off-topic.

bramkragten Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steverep left a comment

Choose a reason for hiding this comment

karwosts commented Jun 10, 2024

steverep left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

This comment was marked as off-topic.

steverep left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

karwosts commented Jun 9, 2024 •

edited by coderabbitai bot

Loading

bramkragten Jun 10, 2024 •

edited

Loading