fix(cdk-experimental/menu): simplify radio and checkbox item APIs (#2…

…4720) * fix(cdk-experimental/menu): simplify radio and checkbox item APIs * fix(cdk-experimental/menu): Add aria-controls attr to triggers * fixup! fix(cdk-experimental/menu): Add aria-controls attr to triggers * fixup! fix(cdk-experimental/menu): Add aria-controls attr to triggers
angular · Apr 4, 2022 · 26e6c1f · 26e6c1f
1 parent 3a94415
commit 26e6c1f
Show file tree

Hide file tree

Showing 21 changed files with 125 additions and 511 deletions.
diff --git a/src/cdk-experimental/menu/menu-bar.spec.ts b/src/cdk-experimental/menu/menu-bar.spec.ts
@@ -69,39 +69,6 @@ describe('MenuBar', () => {
     });
   });
 
-  describe('radiogroup change events', () => {
-    let fixture: ComponentFixture<MenuBarRadioGroup>;
-    let menuItems: CdkMenuItemRadio[];
-
-    beforeEach(waitForAsync(() => {
-      TestBed.configureTestingModule({
-        imports: [CdkMenuModule],
-        declarations: [MenuBarRadioGroup],
-      }).compileComponents();
-
-      fixture = TestBed.createComponent(MenuBarRadioGroup);
-
-      fixture.detectChanges();
-
-      menuItems = fixture.debugElement
-        .queryAll(By.directive(CdkMenuItemRadio))
-        .map(element => element.injector.get(CdkMenuItemRadio));
-    }));
-
-    it('should emit on click', () => {
-      const spy = jasmine.createSpy('cdkMenu change spy');
-      fixture.debugElement
-        .query(By.directive(CdkMenuBar))
-        .injector.get(CdkMenuBar)
-        .change.subscribe(spy);
-
-      menuItems[0].trigger();
-
-      expect(spy).toHaveBeenCalledTimes(1);
-      expect(spy).toHaveBeenCalledWith(menuItems[0]);
-    });
-  });
-
   describe('Keyboard handling', () => {
     describe('(with ltr layout)', () => {
       let fixture: ComponentFixture<MultiMenuWithSubmenu>;
@@ -1145,7 +1112,7 @@ describe('MenuBar', () => {
   template: `
     <ul cdkMenuBar>
       <li role="none">
-        <button checked="true" cdkMenuItemRadio>
+        <button cdkMenuItemChecked="true" cdkMenuItemRadio>
           first
         </button>
       </li>

diff --git a/src/cdk-experimental/menu/menu-base.ts b/src/cdk-experimental/menu/menu-base.ts
@@ -13,6 +13,7 @@ import {
   Directive,
   ElementRef,
   Inject,
+  Input,
   OnDestroy,
   Optional,
   QueryList,
@@ -26,9 +27,12 @@ import {MENU_STACK, MenuStack, MenuStackItem} from './menu-stack';
 import {Menu} from './menu-interface';
 import {PointerFocusTracker} from './pointer-focus-tracker';
 
+let nextId = 0;
+
 @Directive({
   host: {
     '[tabindex]': '_isInline ? (_hasFocus ? -1 : 0) : null',
+    '[id]': 'id',
     '[attr.aria-orientation]': 'orientation',
     '(focus)': 'focusFirstItem()',
     '(focusin)': 'menuStack.setHasFocus(true)',
@@ -39,6 +43,8 @@ export abstract class CdkMenuBase
   extends CdkMenuGroup
   implements Menu, AfterContentInit, OnDestroy
 {
+  @Input() id = `cdk-menu-${nextId++}`;
+
   /**
    * Sets the aria-orientation attribute and determines where menus will be opened.
    * Does not affect styling/layout.
@@ -73,16 +79,14 @@ export abstract class CdkMenuBase
     super();
   }
 
-  override ngAfterContentInit() {
-    super.ngAfterContentInit();
+  ngAfterContentInit() {
     this._setKeyManager();
     this._subscribeToHasFocus();
     this._subscribeToMenuOpen();
     this._subscribeToMenuStackClosed();
   }
 
-  override ngOnDestroy() {
-    super.ngOnDestroy();
+  ngOnDestroy() {
     this.destroyed.next();
     this.destroyed.complete();
   }

diff --git a/src/cdk-experimental/menu/menu-group.spec.ts b/src/cdk-experimental/menu/menu-group.spec.ts
@@ -2,7 +2,6 @@ import {Component, ViewChild} from '@angular/core';
 import {ComponentFixture, TestBed, waitForAsync} from '@angular/core/testing';
 import {By} from '@angular/platform-browser';
 import {CdkMenuModule} from './menu-module';
-import {CdkMenuGroup} from './menu-group';
 import {CdkMenuItemCheckbox} from './menu-item-checkbox';
 import {CdkMenuItemRadio} from './menu-item-radio';
 import {CdkMenuItem} from './menu-item';
@@ -58,14 +57,14 @@ describe('MenuGroup', () => {
     }));
 
     it('should change state of sibling menuitemradio in same group', () => {
-      menuItems[1].trigger();
+      menuItems[1].trigger({keepOpen: true});
 
       expect(menuItems[1].checked).toBeTrue();
       expect(menuItems[0].checked).toBeFalse();
     });
 
     it('should not change state of menuitemradio in sibling group', () => {
-      menuItems[3].trigger();
+      menuItems[3].trigger({keepOpen: true});
 
       expect(menuItems[3].checked).toBeTrue();
       expect(menuItems[0].checked).toBeTrue();
@@ -74,65 +73,12 @@ describe('MenuGroup', () => {
     it('should not change radiogroup state with disabled button', () => {
       menuItems[1].disabled = true;
 
-      menuItems[1].trigger();
+      menuItems[1].trigger({keepOpen: true});
 
       expect(menuItems[0].checked).toBeTrue();
       expect(menuItems[1].checked).toBeFalse();
     });
   });
-
-  describe('change events', () => {
-    let fixture: ComponentFixture<MenuWithMenuItemsAndRadioGroups>;
-    let menuItems: CdkMenuItemRadio[];
-
-    beforeEach(waitForAsync(() => {
-      TestBed.configureTestingModule({
-        imports: [CdkMenuModule],
-        declarations: [MenuWithMenuItemsAndRadioGroups],
-      }).compileComponents();
-
-      fixture = TestBed.createComponent(MenuWithMenuItemsAndRadioGroups);
-      fixture.detectChanges();
-
-      fixture.componentInstance.trigger.getMenuTrigger()?.toggle();
-      fixture.detectChanges();
-
-      menuItems = fixture.debugElement
-        .queryAll(By.directive(CdkMenuItemRadio))
-        .map(element => element.injector.get(CdkMenuItemRadio));
-    }));
-
-    it('should emit from enclosing radio group only', () => {
-      const spies: jasmine.Spy[] = [];
-
-      fixture.debugElement.queryAll(By.directive(CdkMenuGroup)).forEach((group, index) => {
-        const spy = jasmine.createSpy(`cdkMenuGroup ${index} change spy`);
-        spies.push(spy);
-        group.injector.get(CdkMenuGroup).change.subscribe(spy);
-      });
-
-      menuItems[0].trigger();
-
-      expect(spies[2]).toHaveBeenCalledTimes(1);
-      expect(spies[2]).toHaveBeenCalledWith(menuItems[0]);
-      expect(spies[3]).not.toHaveBeenCalled();
-      expect(spies[4]).not.toHaveBeenCalled();
-    });
-
-    it('should not emit with click on disabled button', () => {
-      const spy = jasmine.createSpy('cdkMenuGroup change spy');
-
-      fixture.debugElement
-        .queryAll(By.directive(CdkMenuGroup))[1]
-        .injector.get(CdkMenuGroup)
-        .change.subscribe(spy);
-      menuItems[0].disabled = true;
-
-      menuItems[0].trigger();
-
-      expect(spy).not.toHaveBeenCalled();
-    });
-  });
 });
 
 @Component({
@@ -145,7 +91,7 @@ describe('MenuGroup', () => {
         <li role="none">
           <ul cdkMenuGroup>
             <li #first role="none">
-              <button checked="true" cdkMenuItemCheckbox>
+              <button cdkMenuItemChecked="true" cdkMenuItemCheckbox>
                 one
               </button>
             </li>
@@ -174,7 +120,7 @@ class CheckboxMenu {
         <li role="none">
           <ul cdkMenuGroup>
             <li role="none">
-              <button checked="true" cdkMenuItemRadio>
+              <button cdkMenuItemChecked="true" cdkMenuItemRadio>
                 one
               </button>
             </li>
@@ -206,45 +152,3 @@ class CheckboxMenu {
 class MenuWithMultipleRadioGroups {
   @ViewChild(CdkMenuItem) readonly trigger: CdkMenuItem;
 }
-
-@Component({
-  template: `
-    <div cdkMenuBar>
-      <button cdkMenuItem [cdkMenuTriggerFor]="panel"></button>
-    </div>
-    <ng-template #panel>
-      <ul cdkMenu>
-        <li role="none">
-          <ul cdkMenuGroup>
-            <li role="none">
-              <button cdkMenuItemRadio>
-                one
-              </button>
-            </li>
-          </ul>
-        </li>
-        <li role="none">
-          <ul cdkMenuGroup>
-            <li role="none">
-              <button cdkMenuItemRadio>
-                two
-              </button>
-            </li>
-          </ul>
-        </li>
-        <li role="none">
-          <ul cdkMenuGroup>
-            <li role="none">
-              <button cdkMenuItem>
-                three
-              </button>
-            </li>
-          </ul>
-        </li>
-      </ul>
-    </ng-template>
-  `,
-})
-class MenuWithMenuItemsAndRadioGroups {
-  @ViewChild(CdkMenuItem) readonly trigger: CdkMenuItem;
-}
diff --git a/src/cdk-experimental/menu/menu-group.ts b/src/cdk-experimental/menu/menu-group.ts
@@ -6,19 +6,8 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {
-  AfterContentInit,
-  ContentChildren,
-  Directive,
-  EventEmitter,
-  OnDestroy,
-  Output,
-  QueryList,
-} from '@angular/core';
+import {Directive} from '@angular/core';
 import {UniqueSelectionDispatcher} from '@angular/cdk/collections';
-import {takeUntil} from 'rxjs/operators';
-import {CdkMenuItemSelectable} from './menu-item-selectable';
-import {CdkMenuItem} from './menu-item';
 
 /**
  * Directive which acts as a grouping container for `CdkMenuItem` instances with
@@ -33,43 +22,4 @@ import {CdkMenuItem} from './menu-item';
   },
   providers: [{provide: UniqueSelectionDispatcher, useClass: UniqueSelectionDispatcher}],
 })
-export class CdkMenuGroup implements AfterContentInit, OnDestroy {
-  /** Emits the element when checkbox or radiobutton state changed  */
-  @Output() readonly change: EventEmitter<CdkMenuItem> = new EventEmitter();
-
-  /** List of menuitemcheckbox or menuitemradio elements which reside in this group */
-  @ContentChildren(CdkMenuItemSelectable, {descendants: true})
-  private readonly _selectableItems: QueryList<CdkMenuItemSelectable>;
-
-  /** Emits when the _selectableItems QueryList triggers a change */
-  private readonly _selectableChanges: EventEmitter<void> = new EventEmitter();
-
-  ngAfterContentInit() {
-    this._registerMenuSelectionListeners();
-  }
-
-  ngOnDestroy() {
-    this._selectableChanges.next();
-    this._selectableChanges.complete();
-  }
-
-  /**
-   * Register the child selectable elements with the change emitter and ensure any new child
-   * elements do so as well.
-   */
-  private _registerMenuSelectionListeners() {
-    this._selectableItems.forEach(selectable => this._registerClickListener(selectable));
-
-    this._selectableItems.changes.subscribe((selectableItems: QueryList<CdkMenuItemSelectable>) => {
-      this._selectableChanges.next();
-      selectableItems.forEach(selectable => this._registerClickListener(selectable));
-    });
-  }
-
-  /** Register each selectable to emit on the change Emitter when clicked */
-  private _registerClickListener(selectable: CdkMenuItemSelectable) {
-    selectable.toggled
-      .pipe(takeUntil(this._selectableChanges))
-      .subscribe(() => this.change.next(selectable));
-  }
-}
+export class CdkMenuGroup {}
diff --git a/src/cdk-experimental/menu/menu-interface.ts b/src/cdk-experimental/menu/menu-interface.ts
@@ -15,6 +15,8 @@ export const CDK_MENU = new InjectionToken<Menu>('cdk-menu');
 
 /** Interface which specifies Menu operations and used to break circular dependency issues */
 export interface Menu extends MenuStackItem {
+  id: string;
+
   /** The element the Menu directive is placed on. */
   _elementRef: ElementRef<HTMLElement>;
 

diff --git a/src/cdk-experimental/menu/menu-item-checkbox.spec.ts b/src/cdk-experimental/menu/menu-item-checkbox.spec.ts
@@ -48,16 +48,12 @@ describe('MenuItemCheckbox', () => {
     expect(checkboxElement.getAttribute('aria-disabled')).toBe('true');
   });
 
-  it('should be a button type', () => {
-    expect(checkboxElement.getAttribute('type')).toBe('button');
-  });
-
   it('should not have a menu', () => {
     expect(checkbox.hasMenu()).toBeFalse();
   });
 
   it('should toggle the aria checked attribute', () => {
-    expect(checkboxElement.getAttribute('aria-checked')).toBeNull();
+    expect(checkboxElement.getAttribute('aria-checked')).toBe('false');
 
     checkboxElement.click();
     fixture.detectChanges();
@@ -84,7 +80,7 @@ describe('MenuItemCheckbox', () => {
 
   it('should emit on clicked emitter when triggered', () => {
     const spy = jasmine.createSpy('cdkMenuItemCheckbox clicked spy');
-    checkbox.toggled.subscribe(spy);
+    checkbox.triggered.subscribe(spy);
 
     checkbox.trigger();
 
@@ -93,7 +89,7 @@ describe('MenuItemCheckbox', () => {
 
   it('should not emit on clicked emitter when disabled', () => {
     const spy = jasmine.createSpy('cdkMenuItemCheckbox clicked spy');
-    checkbox.toggled.subscribe(spy);
+    checkbox.triggered.subscribe(spy);
     checkbox.disabled = true;
 
     checkbox.trigger();

diff --git a/src/cdk-experimental/menu/menu-item-checkbox.ts b/src/cdk-experimental/menu/menu-item-checkbox.ts
@@ -18,11 +18,7 @@ import {CdkMenuItem} from './menu-item';
   selector: '[cdkMenuItemCheckbox]',
   exportAs: 'cdkMenuItemCheckbox',
   host: {
-    '[tabindex]': '_tabindex',
-    'type': 'button',
     'role': 'menuitemcheckbox',
-    '[attr.aria-checked]': 'checked || null',
-    '[attr.aria-disabled]': 'disabled || null',
   },
   providers: [
     {provide: CdkMenuItemSelectable, useExisting: CdkMenuItemCheckbox},
@@ -31,8 +27,8 @@ import {CdkMenuItem} from './menu-item';
 })
 export class CdkMenuItemCheckbox extends CdkMenuItemSelectable {
   /** Toggle the checked state of the checkbox. */
-  override trigger() {
-    super.trigger();
+  override trigger(options?: {keepOpen: boolean}) {
+    super.trigger(options);
 
     if (!this.disabled) {
       this.checked = !this.checked;