From 62c6105e25bc4590f9e2fa92069b77ccbc17fac6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jacob=20M=C3=BCller?= <jacob.mueller.elz@gmail.com>
Date: Tue, 12 May 2020 01:12:36 +0200
Subject: [PATCH] feat(b-nav-item-dropdown): improve default handling of
 dropdown toggle link (closes #3942) (#5344)

* fix(b-nav-item-dropdown): let `<b-link>` handle `href` default

* Update nav-item-dropdown.js

* Update nav-item-dropdown.spec.js

* Update nav-item-dropdown.js

* Update nav-item-dropdown.js

* Update id.js

* Update nav-item-dropdown.js

* Update nav-item-dropdown.spec.js

* Update nav-item-dropdown.spec.js

* Update README.md

* Update nav-item-dropdown.spec.js

* Update nav-item-dropdown.js

* Update nav-item-dropdown.spec.js

* Update nav-item-dropdown.spec.js

* Update nav-item-dropdown.spec.js

* Update README.md

* Update nav-item-dropdown.js

* Update nav-item-dropdown.spec.js

Co-authored-by: Troy Morehouse <troymore@nbnet.nb.ca>
---
 src/components/nav/README.md                 |  16 +-
 src/components/nav/nav-item-dropdown.js      |  48 ++--
 src/components/nav/nav-item-dropdown.spec.js | 259 +++++++++++++++++--
 src/mixins/id.js                             |  22 +-
 4 files changed, 292 insertions(+), 53 deletions(-)

diff --git a/src/components/nav/README.md b/src/components/nav/README.md
index 379dcc6deb2..924287d2b52 100644
--- a/src/components/nav/README.md
+++ b/src/components/nav/README.md
@@ -190,12 +190,13 @@ Use `<b-nav-item-dropdown>` to place dropdown items within your nav.
 
 Sometimes you want to add your own class names to the generated dropdown toggle button, that by
 default have the classes `nav-link` and `dropdown-toggle`. Use the `toggle-class` prop to add them
-(like above) which will produce something like:
+(like above) which will render HTML similar to:
 
 ```html
 <li id="my-nav-dropdown" class="nav-item b-nav-dropdown dropdown">
   <a
-    href="#"
+    role="button"
+    href="#my-nav-dropdown"
     id="my-nav-dropdown__BV_button_"
     aria-haspopup="true"
     aria-expanded="false"
@@ -222,6 +223,17 @@ shown. When there are a large number of dropdowns rendered on the same page, per
 impacted due to larger overall memory utilization. You can instruct `<b-nav-item-dropdown>` to
 render the menu contents only when it is shown by setting the `lazy` prop to true.
 
+### Dropdown implementation note
+
+Note that the toggle button is actually rendered as a link `<a>` tag with `role="button"` for
+styling purposes, and typically has the `href` set to `#` unless an ID is provided via the `id`
+prop.
+
+The toggle will prevent scroll-top-top behaviour (via JavaScript) when clicking the toggle link. In
+some cases when using SSR, and the user clicks the toggle button _before_ Vue has had a chance to
+hydrate the component, the page will scroll to top. In these cases, simply providing a unique ID via
+the `id` prop will prevent the unwanted scroll-to-top behaviour.
+
 ## Nav text content
 
 Use the `<b-nav-text>` child component to place plain text content into the nav:
diff --git a/src/components/nav/nav-item-dropdown.js b/src/components/nav/nav-item-dropdown.js
index 85cfc190890..199d552951b 100644
--- a/src/components/nav/nav-item-dropdown.js
+++ b/src/components/nav/nav-item-dropdown.js
@@ -1,25 +1,28 @@
 import Vue from '../../utils/vue'
-import { props as BDropdownProps } from '../dropdown/dropdown'
-import idMixin from '../../mixins/id'
-import dropdownMixin from '../../mixins/dropdown'
-import normalizeSlotMixin from '../../mixins/normalize-slot'
 import pluckProps from '../../utils/pluck-props'
 import { htmlOrText } from '../../utils/html'
+import dropdownMixin from '../../mixins/dropdown'
+import idMixin from '../../mixins/id'
+import normalizeSlotMixin from '../../mixins/normalize-slot'
+import { props as BDropdownProps } from '../dropdown/dropdown'
 import { BLink } from '../link/link'
 
-// -- Constants --
-
+// --- Props ---
 export const props = pluckProps(
   ['text', 'html', 'menuClass', 'toggleClass', 'noCaret', 'role', 'lazy'],
   BDropdownProps
 )
 
+// --- Main component ---
 // @vue/component
 export const BNavItemDropdown = /*#__PURE__*/ Vue.extend({
   name: 'BNavItemDropdown',
   mixins: [idMixin, dropdownMixin, normalizeSlotMixin],
   props,
   computed: {
+    toggleId() {
+      return this.safeId('_BV_toggle_')
+    },
     isNav() {
       // Signal to dropdown mixin that we are in a navbar
       return true
@@ -41,49 +44,54 @@ export const BNavItemDropdown = /*#__PURE__*/ Vue.extend({
     }
   },
   render(h) {
-    const button = h(
+    const { toggleId, visible } = this
+
+    const $toggle = h(
       BLink,
       {
-        ref: 'toggle',
         staticClass: 'nav-link dropdown-toggle',
         class: this.toggleClasses,
         props: {
-          href: '#',
+          href: `#${this.id || ''}`,
           disabled: this.disabled
         },
         attrs: {
-          id: this.safeId('_BV_button_'),
+          id: toggleId,
+          role: 'button',
           'aria-haspopup': 'true',
-          'aria-expanded': this.visible ? 'true' : 'false'
+          'aria-expanded': visible ? 'true' : 'false'
         },
         on: {
           mousedown: this.onMousedown,
           click: this.toggle,
           keydown: this.toggle // Handle ENTER, SPACE and DOWN
-        }
+        },
+        ref: 'toggle'
       },
       [
-        this.$slots['button-content'] ||
-          this.$slots.text ||
+        // TODO: The `text` slot is deprecated in favor of the `button-content` slot
+        this.normalizeSlot(['button-content', 'text']) ||
           h('span', { domProps: htmlOrText(this.html, this.text) })
       ]
     )
-    const menu = h(
+
+    const $menu = h(
       'ul',
       {
         staticClass: 'dropdown-menu',
         class: this.menuClasses,
-        ref: 'menu',
         attrs: {
           tabindex: '-1',
-          'aria-labelledby': this.safeId('_BV_button_')
+          'aria-labelledby': toggleId
         },
         on: {
           keydown: this.onKeydown // Handle UP, DOWN and ESC
-        }
+        },
+        ref: 'menu'
       },
-      !this.lazy || this.visible ? this.normalizeSlot('default', { hide: this.hide }) : [h()]
+      !this.lazy || visible ? this.normalizeSlot('default', { hide: this.hide }) : [h()]
     )
+
     return h(
       'li',
       {
@@ -91,7 +99,7 @@ export const BNavItemDropdown = /*#__PURE__*/ Vue.extend({
         class: this.dropdownClasses,
         attrs: { id: this.safeId() }
       },
-      [button, menu]
+      [$toggle, $menu]
     )
   }
 })
diff --git a/src/components/nav/nav-item-dropdown.spec.js b/src/components/nav/nav-item-dropdown.spec.js
index 2ed200bc36f..df15c5e5b5a 100644
--- a/src/components/nav/nav-item-dropdown.spec.js
+++ b/src/components/nav/nav-item-dropdown.spec.js
@@ -1,55 +1,276 @@
 import { mount } from '@vue/test-utils'
+import { waitNT, waitRAF } from '../../../tests/utils'
 import { BNavItemDropdown } from './nav-item-dropdown'
 
 describe('nav-item-dropdown', () => {
-  it('should have custom toggle class in nav-item-dropdown', async () => {
+  it('has expected default structure', async () => {
+    const wrapper = mount(BNavItemDropdown)
+
+    expect(wrapper.vm).toBeDefined()
+    await waitNT(wrapper.vm)
+
+    expect(wrapper.element.tagName).toBe('LI')
+    expect(wrapper.element.hasAttribute('id')).toBe(true)
+    expect(wrapper.classes()).toContain('nav-item')
+    expect(wrapper.classes()).toContain('b-nav-dropdown')
+    expect(wrapper.classes()).toContain('dropdown')
+
+    const $toggle = wrapper.find('.dropdown-toggle')
+    expect($toggle.element.tagName).toBe('A')
+    expect($toggle.element.hasAttribute('id')).toBe(true)
+    expect($toggle.attributes('role')).toEqual('button')
+    expect($toggle.attributes('aria-haspopup')).toEqual('true')
+    expect($toggle.attributes('aria-expanded')).toEqual('false')
+    expect($toggle.attributes('href')).toEqual('#')
+    expect($toggle.classes()).toContain('dropdown-toggle')
+    expect($toggle.classes()).toContain('nav-link')
+
+    const $menu = wrapper.find('.dropdown-menu')
+    expect($menu.element.tagName).toBe('UL')
+    expect($menu.attributes('tabindex')).toEqual('-1')
+    expect($menu.attributes('aria-labelledby')).toEqual($toggle.attributes('id'))
+    expect($menu.classes()).toContain('dropdown-menu')
+
+    wrapper.destroy()
+  })
+
+  it('should have a flag that we are in a nav', async () => {
+    const wrapper = mount(BNavItemDropdown)
+
+    expect(wrapper.vm).toBeDefined()
+    expect(wrapper.vm.isNav).toBe(true)
+
+    wrapper.destroy()
+  })
+
+  it('should have custom toggle class when "toggle-class" prop set', async () => {
     const wrapper = mount(BNavItemDropdown, {
       propsData: {
-        text: 'toggle',
         toggleClass: 'nav-link-custom'
       }
     })
+
     expect(wrapper.vm).toBeDefined()
-    expect(wrapper.findAll('.dropdown-toggle').length).toBe(1)
-    const $toggle = wrapper.find('.dropdown-toggle')
+    await waitNT(wrapper.vm)
 
-    expect($toggle.classes()).toContain('nav-link')
-    expect($toggle.classes()).toContain('dropdown-toggle')
+    const $toggle = wrapper.find('.dropdown-toggle')
     expect($toggle.classes()).toContain('nav-link-custom')
 
     wrapper.destroy()
   })
 
-  it('should flag that we are in a nav', async () => {
+  it('should be disabled when "disabled" prop set', async () => {
     const wrapper = mount(BNavItemDropdown, {
       propsData: {
-        text: 'toggle'
+        disabled: true
       }
     })
+
     expect(wrapper.vm).toBeDefined()
+    await waitNT(wrapper.vm)
 
-    expect(wrapper.vm.isNav).toBe(true)
+    const $toggle = wrapper.find('.dropdown-toggle')
+    expect($toggle.classes()).toContain('disabled')
+    expect($toggle.attributes('aria-disabled')).toBeDefined()
 
     wrapper.destroy()
   })
 
-  it('should be disabled when disabled prop set', async () => {
+  it('should have href with ID when "id" prop set', async () => {
     const wrapper = mount(BNavItemDropdown, {
       propsData: {
-        text: 'toggle',
-        disabled: true
+        id: 'foo'
       }
     })
+
     expect(wrapper.vm).toBeDefined()
-    expect(wrapper.findAll('.dropdown-toggle').length).toBe(1)
+    await waitNT(wrapper.vm)
+
+    expect(wrapper.element.hasAttribute('id')).toBe(true)
+    expect(wrapper.attributes('id')).toEqual('foo')
+
     const $toggle = wrapper.find('.dropdown-toggle')
-    expect($toggle.element.tagName).toBe('A')
+    expect($toggle.attributes('href')).toEqual('#foo')
 
-    expect($toggle.attributes('aria-disabled')).toBeDefined()
-    expect($toggle.attributes('href')).toEqual('#')
-    expect($toggle.classes()).toContain('disabled')
-    expect($toggle.classes()).toContain('nav-link')
-    expect($toggle.classes()).toContain('dropdown-toggle')
+    wrapper.destroy()
+  })
+
+  it('should have correct toggle content when "text" prop set [DEPRECATED]', async () => {
+    const wrapper = mount(BNavItemDropdown, {
+      propsData: {
+        text: 'foo'
+      }
+    })
+
+    expect(wrapper.vm).toBeDefined()
+    await waitNT(wrapper.vm)
+
+    const $toggle = wrapper.find('.dropdown-toggle')
+    expect($toggle.text()).toEqual('foo')
+
+    wrapper.destroy()
+  })
+
+  it('should have correct toggle content when "html" prop set', async () => {
+    const wrapper = mount(BNavItemDropdown, {
+      propsData: {
+        text: 'foo',
+        html: '<span>bar</span>'
+      }
+    })
+
+    expect(wrapper.vm).toBeDefined()
+    await waitNT(wrapper.vm)
+
+    const $toggle = wrapper.find('.dropdown-toggle')
+    expect($toggle.find('span').exists()).toBe(true)
+    expect($toggle.text()).toEqual('bar')
+
+    wrapper.destroy()
+  })
+
+  it('should have correct toggle content from "text" slot', async () => {
+    const wrapper = mount(BNavItemDropdown, {
+      propsData: {
+        text: 'foo',
+        html: '<span>bar</span>'
+      },
+      slots: {
+        text: '<strong>baz</strong>'
+      }
+    })
+
+    expect(wrapper.vm).toBeDefined()
+    await waitNT(wrapper.vm)
+
+    const $toggle = wrapper.find('.dropdown-toggle')
+    expect($toggle.find('strong').exists()).toBe(true)
+    expect($toggle.text()).toEqual('baz')
+
+    wrapper.destroy()
+  })
+
+  it('should have correct toggle content from "button-content" slot', async () => {
+    const wrapper = mount(BNavItemDropdown, {
+      propsData: {
+        text: 'foo',
+        html: '<span>bar</span>'
+      },
+      slots: {
+        'button-content': '<article>foobar</article>',
+        text: '<strong>baz</strong>'
+      }
+    })
+
+    expect(wrapper.vm).toBeDefined()
+    await waitNT(wrapper.vm)
+
+    const $toggle = wrapper.find('.dropdown-toggle')
+    expect($toggle.find('article').exists()).toBe(true)
+    expect($toggle.text()).toEqual('foobar')
+
+    wrapper.destroy()
+  })
+
+  it('should have correct menu content for "default" slot', async () => {
+    let slotScope = null
+    const wrapper = mount(BNavItemDropdown, {
+      scopedSlots: {
+        default(scope) {
+          slotScope = scope
+          return this.$createElement('div', 'foo')
+        }
+      }
+    })
+
+    expect(wrapper.vm).toBeDefined()
+    await waitNT(wrapper.vm)
+
+    const $menu = wrapper.find('.dropdown-menu')
+    expect($menu.find('div').exists()).toBe(true)
+    expect($menu.text()).toEqual('foo')
+
+    expect(slotScope).toBeDefined()
+    expect(slotScope.hide).toBeDefined()
+
+    wrapper.destroy()
+  })
+
+  it('should only render menu content when visible when "lazy" prop set', async () => {
+    const wrapper = mount(BNavItemDropdown, {
+      propsData: {
+        lazy: true
+      },
+      scopedSlots: {
+        default() {
+          return this.$createElement('div', 'bar')
+        }
+      }
+    })
+
+    expect(wrapper.vm).toBeDefined()
+    expect(wrapper.vm.visible).toBe(false)
+    await waitNT(wrapper.vm)
+
+    const $menu = wrapper.find('.dropdown-menu')
+    expect($menu.find('div').exists()).toBe(false)
+
+    wrapper.vm.show()
+    await waitNT(wrapper.vm)
+    await waitRAF()
+    expect(wrapper.vm.visible).toBe(true)
+    expect($menu.find('div').exists()).toBe(true)
+    expect($menu.text()).toEqual('bar')
+
+    wrapper.vm.hide()
+    await waitNT(wrapper.vm)
+    await waitRAF()
+    expect(wrapper.vm.visible).toBe(false)
+    expect($menu.find('div').exists()).toBe(false)
+
+    wrapper.destroy()
+  })
+
+  it('should open/close on toggle click', async () => {
+    const wrapper = mount(BNavItemDropdown)
+
+    expect(wrapper.vm).toBeDefined()
+    await waitNT(wrapper.vm)
+
+    const $toggle = wrapper.find('.dropdown-toggle')
+    expect(wrapper.vm.visible).toBe(false)
+    expect($toggle.attributes('aria-expanded')).toEqual('false')
+
+    await $toggle.trigger('click')
+    await waitRAF()
+    expect(wrapper.vm.visible).toBe(true)
+    expect($toggle.attributes('aria-expanded')).toEqual('true')
+
+    await $toggle.trigger('click')
+    await waitRAF()
+    expect(wrapper.vm.visible).toBe(false)
+    expect($toggle.attributes('aria-expanded')).toEqual('false')
+
+    wrapper.destroy()
+  })
+
+  it('should prevent toggle click', async () => {
+    const wrapper = mount(BNavItemDropdown, {
+      propsData: {
+        text: 'toggle'
+      }
+    })
+
+    expect(wrapper.vm).toBeDefined()
+    await waitNT(wrapper.vm)
+
+    const $toggle = wrapper.find('.dropdown-toggle')
+
+    await $toggle.trigger('click')
+    expect(wrapper.emitted('toggle')).toBeDefined()
+    expect(wrapper.emitted('toggle').length).toBe(1)
+    expect(wrapper.emitted('toggle')[0]).toBeDefined()
+    expect(wrapper.emitted('toggle')[0][0].defaultPrevented).toBe(true)
 
     wrapper.destroy()
   })
diff --git a/src/mixins/id.js b/src/mixins/id.js
index 51572258488..44d8abd5d5e 100644
--- a/src/mixins/id.js
+++ b/src/mixins/id.js
@@ -1,8 +1,6 @@
-/*
- * SSR Safe Client Side ID attribute generation
- * id's can only be generated client side, after mount.
- * this._uid is not synched between server and client.
- */
+// SSR safe client-side ID attribute generation
+// ID's can only be generated client-side, after mount
+// `this._uid` is not synched between server and client
 
 // @vue/component
 export default {
@@ -19,13 +17,13 @@ export default {
   },
   computed: {
     safeId() {
-      // Computed property that returns a dynamic function for creating the ID.
-      // Reacts to changes in both .id and .localId_ And regens a new function
+      // Computed property that returns a dynamic function for creating the ID
+      // Reacts to changes in both `.id` and `.localId_` and regenerates a new function
       const id = this.id || this.localId_
 
       // We return a function that accepts an optional suffix string
-      // So this computed prop looks and works like a method!!!
-      // But benefits from Vue's Computed prop caching
+      // So this computed prop looks and works like a method
+      // but benefits from Vue's computed prop caching
       const fn = suffix => {
         if (!id) {
           return null
@@ -37,10 +35,10 @@ export default {
     }
   },
   mounted() {
-    // mounted only occurs client side
+    // `mounted()` only occurs client-side
     this.$nextTick(() => {
-      // Update dom with auto ID after dom loaded to prevent
-      // SSR hydration errors.
+      // Update DOM with auto-generated ID after mount
+      // to prevent SSR hydration errors
       this.localId_ = `__BVID__${this._uid}`
     })
   }