Skip to content

feat(cdk-experimental/toolbar): toolbar and widget #31610

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

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

demanr
Copy link

@demanr demanr commented Jul 29, 2025

Draft PR for first revision of CdkToolbar and CdkToolbarWidget.

@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label Jul 29, 2025
@ok7sai ok7sai added dev-app preview When applied, previews of the dev-app are deployed to Firebase and removed dev-app preview When applied, previews of the dev-app are deployed to Firebase labels Jul 31, 2025
@ok7sai ok7sai requested review from wagnermaciel and ok7sai August 6, 2025 19:49
@ok7sai ok7sai added the dev-app preview When applied, previews of the dev-app are deployed to Firebase label Aug 6, 2025
selector: '[cdkToolbarWidget]',
exportAs: 'cdkToolbarWidget',
host: {
'role': 'button',
Copy link
Contributor

Choose a reason for hiding this comment

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

widgets might not always be buttons, we should leave the role assignment to developers. https://www.w3.org/WAI/ARIA/apg/patterns/toolbar/examples/toolbar/ at least there are links and spinbuttons.

Copy link
Author

Choose a reason for hiding this comment

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

Removing the 'role' attribute causes some issues with the _getItem function. This function is called when a pointer event occurs on a widget, and the current solution involves searching for the closest target with role button or role radio. By removing this role keyboard navigation is not updated in position when a widget is clicked. The _getItem function is likely to change in the future- do you have a recommended way of altering it now to prevent this issue while removing the role?

/** Finds the Toolbar Widget associated with a pointer event target. */
  private _getItem(e: PointerEvent): RadioButtonPattern<V> | ToolbarWidgetPattern | undefined {
    if (!(e.target instanceof HTMLElement)) {
      return undefined;
    }

    // Assumes the target or its ancestor has role="radio" or role="button"
    const element = e.target.closest('[role="button"], [role="radio"]');
    return this.inputs.items().find(i => i.element() === element);
  }

Copy link
Contributor

Choose a reason for hiding this comment

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

This needs a larger discussion but we might want to lean on data- attributes to resolve this

...this,
id: this.id,
element: this.element,
disabled: computed(() => this._cdkToolbar.disabled() || this.disabled() || false),
Copy link
Contributor

Choose a reason for hiding this comment

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

The list behavior handles the parent/child disabled logic internally, so passing only the current disabled state for the widget should be sufficient.

Copy link
Author

Choose a reason for hiding this comment

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

Due to the fact that widgets do not have selection logic directly applied, the list behavior isn't fully in charge of whether or not a widget can be interacted with. Without having the widget disabled state mimic that of the parent toolbar the native buttons can still be interacted with when the toolbar is disabled. Since interaction begins at the inner most level the Toolbar cannot prevent the click event from propagating to the button. The extra || false is not needed though. I think it's worth discussing having a cleaner way of preventing interaction within a toolbar though.

} else {
/** Item is a Toolbar Widget, manually select it */
if (activeItem && activeItem.element() && !activeItem.disabled())
activeItem.element().click();
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's investigate whether we should allow event manager to propagate some events. I imagine there are also cases like developers adding a wrapper on top of a button widget to trigger side-effects (e.g. tracking) or something, and this solution will only trigger the target button.

};

/** Represents the required inputs for a toolbar widget in a toolbar. */
export interface ToolbarWidgetInputs extends Omit<ListItem<any>, 'searchTerm' | 'value' | 'index'> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use generics instead of any.

Copy link
Author

Choose a reason for hiding this comment

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

It is presently set as any due to the fact this typing is solely required for selection and as such is never set or used in the widget. Should it still be changed to a generic despite this?

/** Represents an item in the list. */
export type ListItem<V> = ListTypeaheadItem &
  ListNavigationItem &
  ListSelectionItem<V> &
  ListFocusItem;

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if there's some way to clean up the List behavior to make this part of the authoring experience nicer. It's very common for UI Patterns to rely on 3 of the 4 sub-behaviors, and each time it is annoying to need to get the type definitions right

We might benefit from authoring more precise types within the List that UI Patterns could pick from:

export type ListInputs<T extends ListItem<V>, V> = ListFocusInputs<T> &
  ListNavigationInputs<T> &
  ListSelectionInputs<T, V> &
  ListTypeaheadInputs<T>;

export type ListInputsWithoutTypeahead<T extends ListItem<V>, V> = Omit<
  ListInputs<T, V>,
  keyof ListTypeaheadInputs<T>
>;

export type ListInputsWithoutSelection<T extends ListItem<V>, V> = Omit<
  ListInputs<T, V>,
  keyof ListSelectionInputs<T, V>
>;

export type ListInputsWithoutSelectionOrTypeahead<T extends ListItem<V>, V> = Omit<
  ListInputs<T, V>,
  keyof ListSelectionInputs<T, V> | keyof ListTypeaheadInputs<T>
>;

Copy link
Author

Choose a reason for hiding this comment

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

I think this could be a good way forward- especially considering how many patterns only need a subset of what List provides.

@ok7sai ok7sai added dev-app preview When applied, previews of the dev-app are deployed to Firebase and removed dev-app preview When applied, previews of the dev-app are deployed to Firebase labels Aug 6, 2025
Copy link

github-actions bot commented Aug 6, 2025

Deployed dev-app for 664a27b to: https://ng-dev-previews-comp--pr-angular-components-31610-dev-t4erc2pi.web.app

Note: As new commits are pushed to this pull request, this link is updated after the preview is rebuilt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: docs Related to the documentation area: material/button-toggle area: material/chips area: material/stepper detected: feature PR contains a feature commit dev-app preview When applied, previews of the dev-app are deployed to Firebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants