Repository navigation
fix(aria/accordion): move listeners to trigger - #34053
Conversation
Currently the accordion listens to events on the entire accordion group. This is problematic, because it can end up picking up events from the accordion's content and we need to remember to filter them out on each usage. These changes try to address the root cause by listening for events only on the triggers. Fixes angular#34011.
| '[attr.aria-disabled]': '_pattern.disabled()', | ||
| '[attr.disabled]': '_pattern.hardDisabled() ? true : null', | ||
| '[attr.tabindex]': '_pattern.tabIndex()', | ||
| '(keydown)': '_pattern.onKeydown($event)', |
There was a problem hiding this comment.
Note: there are other ways to resolve the issue, but this one seemed like the most sustainable long-term. I might be missing some context on why we had the top-level listeners though.
There was a problem hiding this comment.
The reason was mostly for performance (one event listener handles all navigation items), but it's probably not a big deal for accordion as I can't imagine a use case there are hundreds of them in one group. ¯\_(ツ)_/¯
If we want to keep it single handler binding, then another possible solution might be passing down the event manually to the target/active child to separate the triggering logic (a similar way has been done in Grid https://github.com/angular/components/blob/main/src/aria/private/grid/grid.ts#L219), but I also doubt if it's sustainable for long-term.
There was a problem hiding this comment.
That's fair although my thinking is that there likely won't be that many accordions on a page for it to matter. I also suspect some of those gains might be offset by us trying to route the event to the right element.
Currently the accordion listens to events on the entire accordion group. This is problematic, because it can end up picking up events from the accordion's content and we need to remember to filter them out on each usage. These changes try to address the root cause by listening for events only on the triggers. Fixes #34011. (cherry picked from commit c665617)
Currently the accordion listens to events on the entire accordion group. This is problematic, because it can end up picking up events from the accordion's content and we need to remember to filter them out on each usage.
These changes try to address the root cause by listening for events only on the triggers.
Fixes #34011.