fix(dropdown): don't fail on keypress if there are no ngbDropdownItems

Fixes #3117
ng-bootstrap · Apr 25, 2019 · b4b0222 · b4b0222
1 parent fdf3985
commit b4b0222
Show file tree

Hide file tree

Showing 6 changed files with 170 additions and 117 deletions.
diff --git a/e2e-app/src/app/dropdown/focus/dropdown-focus.component.html b/e2e-app/src/app/dropdown/focus/dropdown-focus.component.html
@@ -6,23 +6,32 @@ <h3>Dropdown focus tests</h3>
     <input type="text" class="form-control mr-1" style="width: 100px" placeholder="before" id="before" />
 
     <div class="input-group">
-      <div ngbDropdown class="d-inline-block mr-1" [container]="container">
+      <div ngbDropdown class="d-inline-block mr-3" [container]="container">
         <button class="btn btn-outline-secondary" id="dropdown" ngbDropdownToggle>Choose and option</button>
         <div ngbDropdownMenu aria-labelledby="dropdown">
-          <button ngbDropdownItem id="item-1">One</button>
-          <hr>
-          <button ngbDropdownItem disabled>Disabled</button>
-          <button class="dropdown-item">Not arrow-focusable</button>
-          <hr>
-          <button ngbDropdownItem id="item-2">Two</button>
+
+          <ng-container *ngIf="withItems">
+            <button ngbDropdownItem id="item-1">One</button>
+            <hr>
+            <button ngbDropdownItem disabled>Disabled</button>
+            <button class="dropdown-item">Not arrow-focusable</button>
+            <hr>
+            <button ngbDropdownItem id="item-2">Two</button>
+          </ng-container>
+
+          <ng-container *ngIf="!withItems">
+            <button class="dropdown-item">One</button>
+            <button class="dropdown-item">Two</button>
+          </ng-container>
+
         </div>
       </div>
     </div>
 
-    <input type="text" class="form-control mr-3" style="width: 100px" placeholder="after" id="after" />
-
     <button type="button" class="btn btn-outline-secondary mr-1" id="container-inline" (click)="container = undefined">Append inline</button>
     <button type="button" class="btn btn-outline-secondary mr-1" id="container-body" (click)="container = 'body'">Append to body</button>
+    <button type="button" class="btn btn-outline-secondary mr-1" id="items-true" (click)="withItems = true">With items</button>
+    <button type="button" class="btn btn-outline-secondary mr-1" id="items-false" (click)="withItems = false">Without items</button>
 
     <span class="ml-3">container is {{ container ? container : 'inline' }}</span>
 

diff --git a/e2e-app/src/app/dropdown/focus/dropdown-focus.component.ts b/e2e-app/src/app/dropdown/focus/dropdown-focus.component.ts
@@ -3,4 +3,5 @@ import {Component} from '@angular/core';
 @Component({templateUrl: './dropdown-focus.component.html'})
 export class DropdownFocusComponent {
   container;
+  withItems = true;
 }
diff --git a/e2e-app/src/app/dropdown/focus/dropdown-focus.e2e-spec.ts b/e2e-app/src/app/dropdown/focus/dropdown-focus.e2e-spec.ts
@@ -2,38 +2,129 @@ import {$, ElementFinder, Key} from 'protractor';
 import {expectFocused, sendKey, openUrl} from '../../tools.po';
 import {DropdownFocusPage} from './dropdown-focus.po';
 
-const containers = ['inline', 'body'];
-containers.forEach((container) => {
+describe(`Dropdown focus`, () => {
 
-  describe(`Dropdown focus with container = ${container}`, () => {
-    let page: DropdownFocusPage;
-    let dropdown: ElementFinder;
-    let toggle: ElementFinder;
+  let page: DropdownFocusPage;
+  let dropdown: ElementFinder;
+  let toggle: ElementFinder;
 
-    beforeAll(() => page = new DropdownFocusPage());
+  beforeAll(() => page = new DropdownFocusPage());
 
-    beforeEach(async() => {
-      await openUrl('dropdown/focus');
-      dropdown = page.getDropdown();
-      toggle = page.getDropdownToggle();
-      await page.selectContainer(container);
-    });
-
-    it(`should not be present on the page initially`,
-       async() => { expect(await page.isOpened(dropdown)).toBeFalsy(`Dropdown should be closed initially`); });
+  beforeEach(async() => {
+    await openUrl('dropdown/focus');
+    dropdown = page.getDropdown();
+    toggle = page.getDropdownToggle();
+  });
 
-    it(`should open dropdown with 'Space' and focus toggling element`, async() => {
-      await page.focusToggle();
-      await sendKey(Key.SPACE);
-      await expectFocused(toggle, `Toggling element should be focused`);
-      expect(await page.isOpened(dropdown)).toBeTruthy(`Dropdown should be opened on 'Space' press`);
+  const containers = ['inline', 'body'];
+  containers.forEach((container) => {
+
+    describe(`with container = ${container}`, () => {
+
+      beforeEach(async() => {
+        await page.selectContainer(container);
+        await page.selectWithItems(true);
+      });
+
+      it(`should not be present on the page initially`,
+         async() => { expect(await page.isOpened(dropdown)).toBeFalsy(`Dropdown should be closed initially`); });
+
+      it(`should open dropdown with 'Space' and focus toggling element`, async() => {
+        await page.focusToggle();
+        await sendKey(Key.SPACE);
+        await expectFocused(toggle, `Toggling element should be focused`);
+        expect(await page.isOpened(dropdown)).toBeTruthy(`Dropdown should be opened on 'Space' press`);
+      });
+
+      it(`should open dropdown with 'Enter' and focus toggling element`, async() => {
+        await page.focusToggle();
+        await sendKey(Key.ENTER);
+        await expectFocused(toggle, `Toggling element should be focused`);
+        expect(await page.isOpened(dropdown)).toBeTruthy(`Dropdown should be opened on 'Enter' press`);
+      });
+
+      it(`should open dropdown with 'ArrowDown' and focus toggling element`, async() => {
+        await page.focusToggle();
+        await sendKey(Key.ARROW_DOWN);
+        await expectFocused(toggle, `Toggling element should be focused`);
+        expect(await page.isOpened(dropdown)).toBeTruthy(`Dropdown should be opened on 'ArrowDown' press`);
+      });
+
+      it(`should open dropdown with 'ArrowUp' and focus toggling element`, async() => {
+        await page.focusToggle();
+        await sendKey(Key.ARROW_UP);
+        await expectFocused(toggle, `Toggling element should be focused`);
+        expect(await page.isOpened(dropdown)).toBeTruthy(`Dropdown should be opened on 'ArrowUp' press`);
+      });
+
+      it(`should focus dropdown items with 'ArrowUp' / 'ArrowDown'`, async() => {
+        // Open
+        await page.open(dropdown);
+        await expectFocused(toggle, `Toggling element should be focused`);
+
+        // Down -> first
+        await sendKey(Key.ARROW_DOWN);
+        await expectFocused(page.getDropdownItem(1), `first dropdown item should be focused`);
+
+        // Down -> second
+        await sendKey(Key.ARROW_DOWN);
+        await expectFocused(page.getDropdownItem(2), `second dropdown item should be focused`);
+
+        // Down -> second
+        await sendKey(Key.ARROW_DOWN);
+        await expectFocused(page.getDropdownItem(2), `second dropdown item should stay focused`);
+
+        // Up -> first
+        await sendKey(Key.ARROW_UP);
+        await expectFocused(page.getDropdownItem(1), `first dropdown item should be focused`);
+
+        // Up -> first
+        await sendKey(Key.ARROW_UP);
+        await expectFocused(page.getDropdownItem(1), `first dropdown item should stay focused`);
+      });
+
+      it(`should focus dropdown first and last items with 'Home' / 'End'`, async() => {
+        // Open
+        await page.open(dropdown);
+        await expectFocused(toggle, `Toggling element should be focused`);
+
+        // End -> last
+        await sendKey(Key.END);
+        await expectFocused(page.getDropdownItem(2), `last dropdown item should be focused`);
+
+        // Home -> first
+        await sendKey(Key.HOME);
+        await expectFocused(page.getDropdownItem(1), `first dropdown item should be focused`);
+      });
+
+      it(`should close dropdown with 'Escape' and focus toggling element (toggle was focused)`, async() => {
+        await page.open(dropdown);
+        await expectFocused(toggle, `Toggling element should be focused`);
+
+        await sendKey(Key.ESCAPE);
+        expect(await page.isOpened(dropdown)).toBeFalsy(`Dropdown should be closed on 'Escape' press`);
+        await expectFocused(toggle, `Toggling element should be focused`);
+      });
+
+      it(`should close dropdown with 'Escape' and focus nothing (item was focused)`, async() => {
+        await page.open(dropdown);
+        await expectFocused(toggle, `Toggling element should be focused`);
+
+        await sendKey(Key.ARROW_DOWN);
+        await expectFocused(page.getDropdownItem(1), `first dropdown item should be focused`);
+
+        await sendKey(Key.ESCAPE);
+        expect(await page.isOpened(dropdown)).toBeFalsy(`Dropdown should be closed on 'Escape' press`);
+        await expectFocused($('body'), `Nothing should be focused after dropdown is closed`);
+      });
     });
+  });
 
-    it(`should open dropdown with 'Enter' and focus toggling element`, async() => {
-      await page.focusToggle();
-      await sendKey(Key.ENTER);
-      await expectFocused(toggle, `Toggling element should be focused`);
-      expect(await page.isOpened(dropdown)).toBeTruthy(`Dropdown should be opened on 'Enter' press`);
+  describe(`without ngbDropdownItems`, () => {
+
+    beforeEach(async() => {
+      await page.selectContainer('inline');
+      await page.selectWithItems(false);
     });
 
     it(`should open dropdown with 'ArrowDown' and focus toggling element`, async() => {
@@ -49,66 +140,6 @@ containers.forEach((container) => {
       await expectFocused(toggle, `Toggling element should be focused`);
       expect(await page.isOpened(dropdown)).toBeTruthy(`Dropdown should be opened on 'ArrowUp' press`);
     });
-
-    it(`should focus dropdown items with 'ArrowUp' / 'ArrowDown'`, async() => {
-      // Open
-      await page.open(dropdown);
-      await expectFocused(toggle, `Toggling element should be focused`);
-
-      // Down -> first
-      await sendKey(Key.ARROW_DOWN);
-      await expectFocused(page.getDropdownItem(1), `first dropdown item should be focused`);
-
-      // Down -> second
-      await sendKey(Key.ARROW_DOWN);
-      await expectFocused(page.getDropdownItem(2), `second dropdown item should be focused`);
-
-      // Down -> second
-      await sendKey(Key.ARROW_DOWN);
-      await expectFocused(page.getDropdownItem(2), `second dropdown item should stay focused`);
-
-      // Up -> first
-      await sendKey(Key.ARROW_UP);
-      await expectFocused(page.getDropdownItem(1), `first dropdown item should be focused`);
-
-      // Up -> first
-      await sendKey(Key.ARROW_UP);
-      await expectFocused(page.getDropdownItem(1), `first dropdown item should stay focused`);
-    });
-
-    it(`should focus dropdown first and last items with 'Home' / 'End'`, async() => {
-      // Open
-      await page.open(dropdown);
-      await expectFocused(toggle, `Toggling element should be focused`);
-
-      // End -> last
-      await sendKey(Key.END);
-      await expectFocused(page.getDropdownItem(2), `last dropdown item should be focused`);
-
-      // Home -> first
-      await sendKey(Key.HOME);
-      await expectFocused(page.getDropdownItem(1), `first dropdown item should be focused`);
-    });
-
-    it(`should close dropdown with 'Escape' and focus toggling element (toggle was focused)`, async() => {
-      await page.open(dropdown);
-      await expectFocused(toggle, `Toggling element should be focused`);
-
-      await sendKey(Key.ESCAPE);
-      expect(await page.isOpened(dropdown)).toBeFalsy(`Dropdown should be closed on 'Escape' press`);
-      await expectFocused(toggle, `Toggling element should be focused`);
-    });
-
-    it(`should close dropdown with 'Escape' and focus nothing (item was focused)`, async() => {
-      await page.open(dropdown);
-      await expectFocused(toggle, `Toggling element should be focused`);
-
-      await sendKey(Key.ARROW_DOWN);
-      await expectFocused(page.getDropdownItem(1), `first dropdown item should be focused`);
-
-      await sendKey(Key.ESCAPE);
-      expect(await page.isOpened(dropdown)).toBeFalsy(`Dropdown should be closed on 'Escape' press`);
-      await expectFocused($('body'), `Nothing should be focused after dropdown is closed`);
-    });
   });
+
 });
diff --git a/e2e-app/src/app/dropdown/focus/dropdown-focus.po.ts b/e2e-app/src/app/dropdown/focus/dropdown-focus.po.ts
@@ -12,4 +12,6 @@ export class DropdownFocusPage extends DropdownPage {
   getDropdownItem(item: number) { return $(`#item-${item}`); }
 
   async selectContainer(container: string) { await $(`#container-${container}`).click(); }
+
+  async selectWithItems(withItems: boolean) { await $(`#items-${withItems}`).click(); }
 }
diff --git a/e2e-app/src/app/start.e2e-spec.ts b/e2e-app/src/app/start.e2e-spec.ts
@@ -1,3 +1,12 @@
 import {browser} from 'protractor';
 
 beforeAll(async() => await browser.get('#/'));
+
+afterEach(async() => {
+  const browserLog = await browser.manage().logs().get('browser');
+
+  if (browserLog.length > 0) {
+    console.error(browserLog);
+    fail(`Unexpected console messages found`);
+  }
+});
diff --git a/src/dropdown/dropdown.ts b/src/dropdown/dropdown.ts
@@ -265,7 +265,7 @@ export class NgbDropdown implements OnInit, OnDestroy {
     let isEventFromItems = false;
     const isEventFromToggle = this._isEventFromToggle(event);
 
-    if (!isEventFromToggle) {
+    if (!isEventFromToggle && itemElements.length) {
       itemElements.forEach((itemElement, index) => {
         if (itemElement.contains(event.target as HTMLElement)) {
           isEventFromItems = true;
@@ -277,29 +277,30 @@ export class NgbDropdown implements OnInit, OnDestroy {
     }
 
     if (isEventFromToggle || isEventFromItems) {
-      if (!this.isOpen()) {
-        this.open();
-      }
-      // tslint:disable-next-line:deprecation
-      switch (event.which) {
-        case Key.ArrowDown:
-          position = Math.min(position + 1, itemElements.length - 1);
-          break;
-        case Key.ArrowUp:
-          if (this._isDropup() && position === -1) {
+      this.open();
+
+      if (itemElements.length) {
+        // tslint:disable-next-line:deprecation
+        switch (event.which) {
+          case Key.ArrowDown:
+            position = Math.min(position + 1, itemElements.length - 1);
+            break;
+          case Key.ArrowUp:
+            if (this._isDropup() && position === -1) {
+              position = itemElements.length - 1;
+              break;
+            }
+            position = Math.max(position - 1, 0);
+            break;
+          case Key.Home:
+            position = 0;
+            break;
+          case Key.End:
             position = itemElements.length - 1;
             break;
-          }
-          position = Math.max(position - 1, 0);
-          break;
-        case Key.Home:
-          position = 0;
-          break;
-        case Key.End:
-          position = itemElements.length - 1;
-          break;
+        }
+        itemElements[position].focus();
       }
-      itemElements[position].focus();
       event.preventDefault();
     }
   }