Skip to content

Commit 05419c5

Browse files
authored
fix(aria/menu): use computed for menu item patterns, with trigger on visible (#33118)
1 parent 4d48ccf commit 05419c5

1 file changed

Lines changed: 16 additions & 9 deletions

File tree

src/aria/menu/menu.ts

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -119,13 +119,21 @@ export class Menu<V> {
119119
readonly _pattern: MenuPattern<V>;
120120

121121
/**
122-
* The menu items as a writable signal.
122+
* The menu item patterns for the menu items that are direct children of this menu, passed
123+
* to the menu pattern.
123124
*
124-
* TODO(wagnermaciel): This would normally be a computed, but using a computed causes a bug where
125-
* sometimes the items array is empty. The bug can be reproduced by switching this to use a
126-
* computed and then quickly opening and closing menus in the dev app.
125+
* Note: contentChildren has an issue where it will return a successively smaller list
126+
* each time that the menu is open and closed, eventually resulting in an empty list.
127+
* The workaround is to trigger a recomputation of this signal whenever the menu is opened
128+
* or closed, by calling this._pattern.visible() in the signal body. Otherwise, computed could
129+
* not be used and would have to rebuild the list each time this method is called.
127130
*/
128-
private readonly _itemPatterns = () => this._items().map(i => i._pattern);
131+
private readonly _itemPatterns = computed(() => {
132+
// Only needed to force a recompute.
133+
this._pattern.visible();
134+
135+
return this._items().map(i => i._pattern);
136+
});
129137

130138
/** Whether the menu is visible. */
131139
readonly visible = computed(() => this._pattern.visible());
@@ -165,10 +173,9 @@ export class Menu<V> {
165173
}
166174
});
167175

168-
// TODO(wagnermaciel): This is a redundancy needed for if the user uses display: none to hide
169-
// submenus. In those cases, the ui pattern is calling focus() before the ui has a chance to
170-
// update the display property. The result is focus() being called on an element that is not
171-
// focusable. This simply retries focusing the element after render.
176+
// Focuses an active menu item when the menu becomes visible. This is needed to
177+
// properly restore focus to the active item when returning to a menu, and to
178+
// focus the first item when navigating into a submenu with hover.
172179
afterRenderEffect(() => {
173180
if (this._pattern.visible()) {
174181
const activeItem = untracked(() => this._pattern.inputs.activeItem());

0 commit comments

Comments
 (0)