ui: Rework popover-menu auto closing (#8340)

* ui: Move more menu subcomponents deeper down into popovermenu

* ui: Simplify aria-menu component+remove auto menu close on route change

* Add ember-string-fns

* Use new PopoverMenu sub components and fix up tests

* Fix up wrong closing let

* Remove dcs from the service show page now we have it in the navigation

ui-v2/app/components/aria-menu/index.hbs

@@ -1,8 +1,10 @@
 {{yield
   (action 'change')
   (action 'keypress')
-  (concat 'component-aria-menu-trigger-' guid)
-  (concat 'component-aria-menu-menu-' guid)
-  (if expanded 'true' undefined)
   (action 'keypressClick')
+  (hash
+    labelledBy=(concat 'component-aria-menu-trigger-' guid)
+    controls=(concat 'component-aria-menu-menu-' guid)
+    expanded=(if expanded 'true' undefined)
+  )
 }}

ui-v2/app/components/aria-menu/index.js

@@ -39,7 +39,6 @@ const MENU_ITEMS = '[role^="menuitem"]';
 export default Component.extend({
   tagName: '',
   dom: service('dom'),
-  router: service('router'),
   guid: '',
   expanded: false,
   orientation: 'vertical',
@@ -56,9 +55,6 @@ export default Component.extend({
     this.$menu = this.dom.element(`#${COMPONENT_ID}menu-${this.guid}`);
     const labelledBy = this.$menu.getAttribute('aria-labelledby');
     this.$trigger = this.dom.element(`#${labelledBy}`);
-    this._routelisteners.add(this.router, {
-      routeWillChange: () => this.actions.close.apply(this, [{}]),
-    });
   },
   willDestroyElement: function() {
     this._super(...arguments);

ui-v2/app/components/hashicorp-consul/index.hbs

@@ -17,7 +17,7 @@
     {{#if (and (eq nspaces.length 1) (not canManageNspaces)) }}
                         <span data-test-nspace-selected={{nspace.Name}}>{{nspace.Name}}</span>
     {{ else }}
-                        <PopoverMenu @position="left">
+                        <PopoverMenu @position="left" as |components api|>
                           <BlockSlot @name="trigger">
                             {{nspace.Name}}
                           </BlockSlot>
@@ -29,25 +29,39 @@
                           </BlockSlot>
                         {{/if}}
                           <BlockSlot @name="menu">
-                            <li role="separator">
-                              Namespaces
-                              <DataSource
-                                @src="/*/*/namespaces"
-                                @onchange={{action (mut nspaces) value="data"}}
-                                @loading="lazy"
-                              />
-                            </li>
+                            {{#let components.MenuItem components.MenuSeparator as |MenuItem MenuSeparator|}}
+                              <MenuSeparator>
+                                <BlockSlot @name="label">
+                                  Namespaces
+                                  <DataSource
+                                    @src="/*/*/namespaces"
+                                    @onchange={{action (mut nspaces) value="data"}}
+                                    @loading="lazy"
+                                  />
+                                </BlockSlot>
+                              </MenuSeparator>
                             {{#each (reject-by 'DeletedAt' nspaces) as |item|}}
-                              <li role="none" class={{if (eq nspace.Name item.Name) 'is-active'}}>
-                                <a tabindex="-1" role="menuitem" href={{href-mut (hash nspace=(concat '~' item.Name))}}>{{item.Name}}</a>
-                              </li>
+                              <MenuItem
+                                class={{if (eq nspace.Name item.Name) 'is-active'}}
+                                @href={{href-mut (hash nspace=(concat '~' item.Name))}}
+                              >
+                                <BlockSlot @name="label">
+                                  {{item.Name}}
+                                </BlockSlot>
+                              </MenuItem>
                             {{/each}}
                     {{#if canManageNspaces }}
-                              <li role="separator"></li>
-                              <li role="none" data-test-main-nav-nspaces>
-                                <a tabindex="-1" role="menuitem" href={{href-to 'dc.nspaces' dc.Name}}>Manage namespaces</a>
-                              </li>
+                              <MenuSeparator />
+                              <MenuItem
+                                data-test-main-nav-nspaces
+                                @href={{href-to 'dc.nspaces' dc.Name}}
+                              >
+                                <BlockSlot @name="label">
+                                  Manage Namespaces
+                                </BlockSlot>
+                              </MenuItem>
                     {{/if}}
+                            {{/let}}
                           </BlockSlot>
                         </PopoverMenu>
     {{/if}}
@@ -57,24 +71,34 @@
   {{#if (or (not dcs) (eq dcs.length 1)) }}
                         <span data-test-datacenter-selected={{dc.Name}}>{{dc.Name}}</span>
   {{ else }}
-                        <PopoverMenu @position="left">
+                        <PopoverMenu @position="left" as |components|>
                           <BlockSlot @name="trigger">
                             {{dc.Name}}
                           </BlockSlot>
                           <BlockSlot @name="menu">
-                            <li role="separator">
-                              Datacenters
-                              <DataSource
-                                @src="/*/*/datacenters"
-                                @onchange={{action (mut dcs) value="data"}}
-                                @loading="lazy"
-                              />
-                            </li>
+                            {{#let components.MenuItem components.MenuSeparator as |MenuItem MenuSeparator|}}
+                            <MenuSeparator>
+                              <BlockSlot @name="label">
+                                Datacenters
+                                <DataSource
+                                  @src="/*/*/datacenters"
+                                  @onchange={{action (mut dcs) value="data"}}
+                                  @loading="lazy"
+                                />
+                              </BlockSlot>
+                            </MenuSeparator>
                             {{#each (sort-by 'Name' dcs) as |item|}}
-                              <li role="none" data-test-datacenter-picker class={{if (eq dc.Name item.Name) 'is-active'}}>
-                                <a tabindex="-1" role="menuitem" href={{href-mut (hash dc=item.Name)}}>{{item.Name}}</a>
-                              </li>
+                              <MenuItem
+                                data-test-datacenter-picker
+                                class={{if (eq dc.Name item.Name) 'is-active'}}
+                                @href={{href-mut (hash dc=item.Name)}}
+                              >
+                                <BlockSlot @name="label">
+                                  {{item.Name}}
+                                </BlockSlot>
+                              </MenuItem>
                             {{/each}}
+                            {{/let}}
                           </BlockSlot>
                         </PopoverMenu>
 
@@ -101,21 +125,38 @@
             <nav>
                 <ul>
                     <li data-test-main-nav-help>
-                      <PopoverMenu @position="right">
+                      <PopoverMenu @position="right" as |components|>
                         <BlockSlot @name="trigger">
                           Help
                         </BlockSlot>
-                        <BlockSlot @name="menu" as |id send keypressClick change|>
-                          <li role="none" class="docs-link">
-                            <a tabindex="-1" role="menuitem" href={{env 'CONSUL_DOCS_URL'}} rel="noopener noreferrer" target="_blank" onclick={{change}}>Documentation</a>
-                          </li>
-                          <li role="none" class="learn-link">
-                            <a tabindex="-1" role="menuitem" href={{concat (env 'CONSUL_DOCS_LEARN_URL') '/consul'}} rel="noopener noreferrer" target="_blank" onclick={{change}}>HashiCorp Learn</a>
-                          </li>
-                          <li role="separator"></li>
-                          <li role="none" class="feedback-link">
-                            <a tabindex="-1" role="menuitem" href={{env 'CONSUL_REPO_ISSUES_URL'}} target="_blank" rel="noopener noreferrer" onclick={{change}}>Provide Feedback</a>
-                          </li>
+                        <BlockSlot @name="menu">
+                          {{#let components.MenuItem components.MenuSeparator as |MenuItem MenuSeparator|}}
+                            <MenuItem
+                              class="docs-link"
+                              @href={{env 'CONSUL_DOCS_URL'}}
+                            >
+                              <BlockSlot @name="label">
+                                Documentation
+                              </BlockSlot>
+                            </MenuItem>
+                            <MenuItem
+                              class="learn-link"
+                              @href={{concat (env 'CONSUL_DOCS_LEARN_URL') '/consul'}}
+                            >
+                              <BlockSlot @name="label">
+                                HashiCorp Learn
+                              </BlockSlot>
+                            </MenuItem>
+                            <MenuSeparator />
+                            <MenuItem
+                              class="learn-link"
+                              @href={{env 'CONSUL_REPO_ISSUES_URL'}}
+                            >
+                              <BlockSlot @name="label">
+                                Provide Feedback
+                              </BlockSlot>
+                            </MenuItem>
+                          {{/let}}
                         </BlockSlot>
                       </PopoverMenu>
                     </li>
@@ -168,21 +209,28 @@
                                 </button>
                               </BlockSlot>
                             </ModalDialog>
-                            <PopoverMenu @position="right">
+                            <PopoverMenu @position="right" as |components api|>
                               <BlockSlot @name="trigger">
                                 Logout
                               </BlockSlot>
                               <BlockSlot @name="menu">
-    {{!TODO: It might be nice to use one of our recursive components here}}
-    {{#if authDialog.token.AccessorID}}
-                                <li role="none">
-                                  <AuthProfile />
-                                </li>
-                                <li role="separator"></li>
-    {{/if}}
-                                <li class="dangerous" role="none">
-                                  <button type="button" tabindex="-1" role="menuitem" onclick={{action authDialog.logout}}>Logout</button>
-                                </li>
+                                {{#let components.MenuItem components.MenuSeparator as |MenuItem MenuSeparator|}}
+        {{!TODO: It might be nice to use one of our recursive components here}}
+        {{#if authDialog.token.AccessorID}}
+                                    <li role="none">
+                                      <AuthProfile />
+                                    </li>
+                                    <MenuSeparator />
+        {{/if}}
+                                    <MenuItem
+                                      class="dangerous"
+                                      @onclick={{action authDialog.logout}}
+                                    >
+                                      <BlockSlot @name="label">
+                                        Logout
+                                      </BlockSlot>
+                                    </MenuItem>
+                                  {{/let}}
                               </BlockSlot>
                             </PopoverMenu>
                           </BlockSlot>

ui-v2/app/components/hashicorp-consul/pageobject.js

@@ -1,4 +1,4 @@
-export default (clickable, attribute, is, authForm) => scope => {
+export default (collection, clickable, attribute, is, authForm) => scope => {
   const page = {
     navigation: [
       'services',
@@ -47,5 +47,8 @@ export default (clickable, attribute, is, authForm) => scope => {
     ':checked',
     '[data-test-nspace-menu] > input[type="checkbox"]'
   );
+  page.navigation.dcs = collection('[data-test-datacenter-picker]', {
+    name: clickable('a'),
+  });
   return page;
 };

ui-v2/app/components/more-popover-menu/index.hbs

@@ -1,16 +1,10 @@
 <div class="more-popover-menu">
-  <PopoverMenu @expanded={{expanded}} @onchange={{action onchange}} @keyboardAccess={{false}} as |api|>
+  <PopoverMenu @expanded={{expanded}} @onchange={{action onchange}} @keyboardAccess={{false}} as |components api|>
     <BlockSlot @name="trigger">
       More
     </BlockSlot>
-    <BlockSlot @name="menu" as |confirm send keypressClick clickTrigger|>
-      {{yield (component 'more-popover-menu/action' menu=(hash
-        addSubmenu=api.addSubmenu
-        removeSubmenu=api.removeSubmenu
-        confirm=confirm
-        keypressClick=keypressClick
-        clickTrigger=clickTrigger
-      ))}}
+    <BlockSlot @name="menu">
+      {{yield components.MenuItem}}
     </BlockSlot>
   </PopoverMenu>
 </div>

ui-v2/app/components/popover-menu/index.hbs

@@ -1,44 +1,62 @@
-{{yield}}
-<div class="popover-menu" ...attributes>
-<AriaMenu @keyboardAccess={{keyboardAccess}} as |change keypress ariaLabelledBy ariaControls ariaExpanded keypressClick|>
-  <ToggleButton
-    @checked={{if keyboardAccess ariaExpanded expanded}}
-    @onchange={{queue change (action "change")}}
-  as |api|>
-    <Ref @target={{this}} @name="toggle" @value={{api}} />
-    <button type="button" aria-haspopup="menu" onkeydown={{keypress}} onclick={{this.toggle.click}} id={{ariaLabelledBy}} aria-controls={{ariaControls}}>
-      <YieldSlot @name="trigger">
-        {{yield (hash
-          addSubmenu=(action 'addSubmenu')
-          removeSubmenu=(action 'removeSubmenu')
-        )}}
-      </YieldSlot>
-    </button>
-  </ToggleButton>
-  <MenuPanel @position={{position}} id={{ariaControls}} aria-labelledby={{ariaLabelledBy}} aria-expanded={{ariaExpanded}} as |api|>
-    <BlockSlot @name="controls">
-      <input type="checkbox" id={{concat 'popover-menu-' guid '-'}} />
-    {{#each submenus as |sub|}}
-      <input type="checkbox" id={{concat 'popover-menu-' guid '-' sub}} onchange={{api.change}} />
-    {{/each}}
-    </BlockSlot>
-    {{#if hasHeader}}
-      <BlockSlot @name="header">
-        {{yield (hash
-          addSubmenu=(action 'addSubmenu')
-          removeSubmenu=(action 'removeSubmenu')
-        )}}
-        {{#yield-slot name="header"}}{{else}}{{/yield-slot}}
-      </BlockSlot>
-    {{/if}}
-    <BlockSlot @name="menu">
-      <YieldSlot @name="menu" @params={{block-params (concat "popover-menu-" guid "-") send keypressClick this.toggle.click}}>
-        {{yield (hash
-          addSubmenu=(action 'addSubmenu')
-          removeSubmenu=(action 'removeSubmenu')
-        )}}
-      </YieldSlot>
-    </BlockSlot>
-  </MenuPanel>
-</AriaMenu>
-</div>
+  {{yield}}
+  <div class="popover-menu" ...attributes>
+    <AriaMenu @keyboardAccess={{keyboardAccess}} as |change keypress keypressClick aria|>
+
+{{#let (hash
+  MenuItem=(component
+    'popover-menu/menu-item'
+    menu=(hash
+      addSubmenu=(action "addSubmenu")
+      removeSubmenu=(action "removeSubmenu")
+      confirm=(concat "popover-menu-" guid "-")
+      clickTrigger=this.toggle.click
+      keypressClick=keypressClick
+    )
+  )
+  MenuSeparator=(component
+    'popover-menu/menu-separator'
+  )
+)
+as |components|}}
+  {{#let (hash
+    toggle=this.toggle.click
+  )
+  as |api|}}
+
+      <ToggleButton
+        @checked={{if keyboardAccess aria.expanded expanded}}
+        @onchange={{queue change (action "change")}}
+      as |toggle|>
+        <Ref @target={{this}} @name="toggle" @value={{toggle}} />
+        <button type="button" aria-haspopup="menu" onkeydown={{keypress}} onclick={{this.toggle.click}} id={{aria.labelledBy}} aria-controls={{aria.controls}}>
+          <YieldSlot @name="trigger">
+            {{yield components api}}
+          </YieldSlot>
+        </button>
+      </ToggleButton>
+
+      <MenuPanel @position={{position}} id={{aria.controls}} aria-labelledby={{aria.labelledBy}} aria-expanded={{aria.expanded}} as |menu|>
+        <BlockSlot @name="controls">
+          <input type="checkbox" id={{concat 'popover-menu-' guid '-'}} />
+        {{#each submenus as |sub|}}
+          <input type="checkbox" id={{concat 'popover-menu-' guid '-' sub}} onchange={{menu.change}} />
+        {{/each}}
+        </BlockSlot>
+        {{#if hasHeader}}
+          <BlockSlot @name="header">
+            {{yield components api}}
+            {{#yield-slot name="header"}}{{else}}{{/yield-slot}}
+          </BlockSlot>
+        {{/if}}
+        <BlockSlot @name="menu">
+          <YieldSlot @name="menu" @params={{block-params (concat "popover-menu-" guid "-") send keypressClick this.toggle.click}}>
+            {{yield components api}}
+          </YieldSlot>
+        </BlockSlot>
+      </MenuPanel>
+
+  {{/let}}
+{{/let}}
+
+    </AriaMenu>
+  </div>

ui-v2/app/components/popover-menu/menu-item/index.hbs

@@ -13,11 +13,19 @@
     }}>{{yield}}</YieldSlot>
   </div>
 {{else if href}}
-  <a role="menuitem" tabindex="-1" href={{href}}>
+{{#let (string-includes href '://') as |external|}}
+  <a
+    role="menuitem" tabindex="-1"
+    onclick={{action menu.clickTrigger}}
+    href={{href}}
+    target={{if external '_blank'}}
+    rel={{if external 'noopener noreferrer'}}
+  >
     <YieldSlot @name="label">
-      {{yield}}
+    {{yield}}
     </YieldSlot>
   </a>
+{{/let}}
 {{else}}
   <button role="menuitem" tabindex="-1" type="button" onclick={{action this.onclick}}>
     <YieldSlot @name="label">
@@ -26,3 +34,4 @@
   </button>
 {{/if}}
 </li>
+

ui-v2/app/components/popover-menu/menu-separator/index.hbs

@@ -0,0 +1,4 @@
+{{yield}}
+<li role="separator">
+  <YieldSlot @name="label">{{yield}}</YieldSlot>
+</li>

ui-v2/app/components/popover-menu/menu-separator/index.js

@@ -0,0 +1,7 @@
+import Component from '@ember/component';
+
+import Slotted from 'block-slots';
+
+export default Component.extend(Slotted, {
+  tagName: '',
+});

ui-v2/app/components/popover-select/index.hbs

@@ -1,19 +1,28 @@
 <div class="popover-select" ...attributes>
-  <PopoverMenu>
+  <PopoverMenu as |components menu|>
     <BlockSlot @name="trigger">
       <span>
         {{selected.value}}
       </span>
     </BlockSlot>
-    <BlockSlot @name="menu" as |id send keypressClick change|>
-      <li role="separator">
-        {{title}}
-      </li>
-    {{#each options as |option|}}
-      <li role="none" class={{if (eq selected.key option.key) 'is-active'}}>
-        <button tabindex="-1" role="menuitem" type="button" value={{option.key}} onclick={{action (queue (action 'change' option) change )}}>{{option.value}}</button>
-      </li>
-    {{/each}}
+    <BlockSlot @name="menu">
+{{#let components.MenuItem components.MenuSeparator as |MenuItem MenuSeparator|}}
+        <MenuSeparator>
+          <BlockSlot @name="label">
+            {{title}}
+          </BlockSlot>
+        </MenuSeparator>
+  {{#each options as |option|}}
+        <MenuItem
+          class={{if (eq selected.key option.key) 'is-active'}}
+          @onclick={{action (queue (action 'change' option) (if multiple (noop) menu.toggle))}}
+        >
+          <BlockSlot @name="label">
+            {{option.value}}
+          </BlockSlot>
+        </MenuItem>
+  {{/each}}
+{{/let}}
     </BlockSlot>
   </PopoverMenu>
 </div>

ui-v2/app/components/popover-select/index.js

@@ -4,6 +4,8 @@ import { inject as service } from '@ember/service';
 export default Component.extend({
   tagName: '',
   dom: service('dom'),
+  multiple: false,
+  onchange: function() {},
   actions: {
     change: function(option, e) {
       this.onchange(this.dom.setEventTargetProperty(e, 'selected', selected => option));

ui-v2/package.json

@@ -108,6 +108,7 @@
     "ember-sinon-qunit": "5.0.0",
     "ember-source": "~3.16.0",
     "ember-stargate": "^0.2.0",
+    "ember-string-fns": "^1.4.0",
     "ember-test-selectors": "^4.0.0",
     "ember-tooltips": "^3.4.3",
     "ember-truth-helpers": "^2.0.0",

ui-v2/tests/acceptance/dc/error.feature

@@ -30,5 +30,5 @@ Feature: dc / error: Recovering from a dc 500 error
   Scenario: Choosing a different dc from the dc menu
     Given the url "/v1/internal/ui/services" responds with a 200 status
     When I click dc on the navigation
-    And I click dcs.0.name
+    And I click dcs.0.name on the navigation
     Then I see 3 service models

ui-v2/tests/acceptance/dc/services/dc-switch.feature

@@ -29,14 +29,14 @@ Feature: dc / services / dc-switch : Switching Datacenters
     Then the url should be /dc-1/services
     Then I see 6 service models
     When I click dc on the navigation
-    And I click dcs.1.name
+    And I click dcs.1.name on the navigation
     Then the url should be /dc-2/services
     Then I see 6 service models
     When I click dc on the navigation
-    And I click dcs.0.name
+    And I click dcs.0.name on the navigation
     Then the url should be /dc-1/services
     Then I see 6 service models
     When I click dc on the navigation
-    And I click dcs.1.name
+    And I click dcs.1.name on the navigation
     Then the url should be /dc-2/services
     Then I see 6 service models

ui-v2/tests/acceptance/dc/services/error.feature

@@ -28,4 +28,4 @@ Feature: dc / services / error
     # When running through namespaces as the dc menu says 'Error'
     # which is still kind of ok
     When I click dc on the navigation
-    And I see 2 datacenter models
+    And I see 2 datacenter models on the navigation component

ui-v2/tests/acceptance/dc/services/show/dc-switch.feature

@@ -22,6 +22,6 @@ Feature: dc / services / show / dc-switch : Switching Datacenters
     Then the url should be /dc-1/services/consul/instances
     And I see instancesUrl on the tabs contains "/dc-1/services/consul/instances"
     When I click dc on the navigation
-    And I click dcs.1.name
+    And I click dcs.1.name on the navigation
     Then the url should be /dc-2/services/consul/instances
     And I see instancesUrl on the tabs contains "/dc-2/services/consul/instances"

ui-v2/tests/pages.js

@@ -124,7 +124,7 @@ const consulPolicyList = consulPolicyListFactory(
   morePopoverMenu
 );
 
-const page = pageFactory(clickable, attribute, is, authForm);
+const page = pageFactory(collection, clickable, attribute, is, authForm);
 
 // pages
 const create = function(appView) {
@@ -150,16 +150,7 @@ export default {
     )
   ),
   service: create(
-    service(
-      visitable,
-      clickable,
-      attribute,
-      collection,
-      text,
-      consulIntentionList,
-      catalogToolbar,
-      tabgroup
-    )
+    service(visitable, attribute, collection, text, consulIntentionList, catalogToolbar, tabgroup)
   ),
   instance: create(instance(visitable, attribute, collection, text, tabgroup)),
   nodes: create(nodes(visitable, clickable, attribute, collection, catalogFilter)),

ui-v2/tests/pages/dc/services/index.js

@@ -9,9 +9,6 @@ export default function(visitable, clickable, text, attribute, present, collecti
   return {
     visit: visitable('/:dc/services'),
     services: collection('.consul-service-list > ul > li:not(:first-child)', service),
-    dcs: collection('[data-test-datacenter-picker]', {
-      name: clickable('a'),
-    }),
     home: clickable('[data-test-home]'),
     sort: popoverSelect(),
   };

ui-v2/tests/pages/dc/services/show.js

@@ -1,13 +1,4 @@
-export default function(
-  visitable,
-  clickable,
-  attribute,
-  collection,
-  text,
-  intentions,
-  filter,
-  tabs
-) {
+export default function(visitable, attribute, collection, text, intentions, filter, tabs) {
   const page = {
     visit: visitable('/:dc/services/:service'),
     externalSource: attribute('data-test-external-source', '[data-test-external-source]', {
@@ -25,11 +16,6 @@ export default function(
       'tags',
     ]),
     filter: filter(),
-
-    dcs: collection('[data-test-datacenter-picker]', {
-      name: clickable('a'),
-    }),
-
     // TODO: These need to somehow move to subpages
     instances: collection('.consul-service-instance-list > ul > li:not(:first-child)', {
       address: text('[data-test-address]'),

ui-v2/yarn.lock

@@ -5921,6 +5921,13 @@ ember-stargate@^0.2.0:
     ember-in-element-polyfill "^1.0.0"
     tracked-maps-and-sets "^2.1.0"
 
+ember-string-fns@^1.4.0:
+  version "1.4.0"
+  resolved "https://registry.yarnpkg.com/ember-string-fns/-/ember-string-fns-1.4.0.tgz#bde188643d54b58c501d332e15ba5c04733a6233"
+  integrity sha512-1pcdUA40PdzzsUAUQ7U4eMRDtxOeUpKwyyInCkj3f/Qdt/QbXZkA/Ix9vWWly9N4QcO7akK0xHWIJiu4XzP8Rw==
+  dependencies:
+    ember-cli-babel "^7.7.3"
+
 ember-template-lint@^2.0.1:
   version "2.9.0"
   resolved "https://registry.yarnpkg.com/ember-template-lint/-/ember-template-lint-2.9.0.tgz#0580468e052c53451c1e983578beb7ee59885141"