From 095eb22dd00932c93a208bbc3b855c818fdcd080 Mon Sep 17 00:00:00 2001
From: John Cowen <johncowen@users.noreply.github.com>
Date: Mon, 10 Aug 2020 16:00:05 +0100
Subject: [PATCH] ui: Dropdown/select improvements (#8468)

* ui: Better org of split-button/sort-button ready for design change

* ui: Improve keyboard accessibility of dropdown menu
---
 ui-v2/app/components/aria-menu/index.js        | 18 +++++++++++-------
 .../styles/base/components/buttons/index.scss  |  3 +++
 .../styles/base/components/buttons/layout.scss | 11 +++++++++++
 .../styles/base/components/buttons/skin.scss   | 14 +++++++-------
 ui-v2/app/styles/components/filter-bar.scss    |  4 ++++
 .../styles/components/filter-bar/layout.scss   |  1 +
 .../app/styles/components/filter-bar/skin.scss |  2 +-
 .../app/styles/components/popover-select.scss  | 18 +-----------------
 8 files changed, 39 insertions(+), 32 deletions(-)

diff --git a/ui-v2/app/components/aria-menu/index.js b/ui-v2/app/components/aria-menu/index.js
index 6643c2bb6..2033b725c 100644
--- a/ui-v2/app/components/aria-menu/index.js
+++ b/ui-v2/app/components/aria-menu/index.js
@@ -77,13 +77,17 @@ export default Component.extend({
       // the keypress
       // TODO: We need to use > somehow here so we don't select submenus
       const $items = [...this.dom.elements(MENU_ITEMS, this.$menu)];
-      if (!this.expanded) {
-        if (e.keyCode === ENTER || e.keyCode === SPACE) {
-          next(() => {
-            $items[0].focus();
-          });
-          return;
-        }
+      if (e.keyCode === ENTER || e.keyCode === SPACE) {
+        // If we are opening, get ready to focus the first item
+        // if we are already open don't control focus
+        let $focus = !this.expanded ? $items[0] : undefined;
+        next(() => {
+          // if we are now closed, focus the trigger instead
+          $focus = !this.expanded ? this.$trigger : $focus;
+          if (typeof $focus !== 'undefined') {
+            $focus.focus();
+          }
+        });
       }
       // this will prevent anything happening if you haven't pushed a
       // configurable key
diff --git a/ui-v2/app/styles/base/components/buttons/index.scss b/ui-v2/app/styles/base/components/buttons/index.scss
index bc1825219..8f5aa10dd 100644
--- a/ui-v2/app/styles/base/components/buttons/index.scss
+++ b/ui-v2/app/styles/base/components/buttons/index.scss
@@ -1,2 +1,5 @@
 @import './skin';
 @import './layout';
+%sort-button {
+  @extend %split-button;
+}
diff --git a/ui-v2/app/styles/base/components/buttons/layout.scss b/ui-v2/app/styles/base/components/buttons/layout.scss
index 34a3a2007..fcfdeb769 100644
--- a/ui-v2/app/styles/base/components/buttons/layout.scss
+++ b/ui-v2/app/styles/base/components/buttons/layout.scss
@@ -54,3 +54,14 @@
   position: relative;
   height: 100%;
 }
+%split-button::after {
+  margin-left: 16px;
+}
+%split-button::before {
+  @extend %as-pseudo;
+  height: 16px;
+}
+%sort-button {
+  @extend %button;
+  height: 35px !important;
+}
diff --git a/ui-v2/app/styles/base/components/buttons/skin.scss b/ui-v2/app/styles/base/components/buttons/skin.scss
index d0c95fc07..bc7627776 100644
--- a/ui-v2/app/styles/base/components/buttons/skin.scss
+++ b/ui-v2/app/styles/base/components/buttons/skin.scss
@@ -118,19 +118,19 @@
 %split-button span::after {
   @extend %as-pseudo;
   position: absolute;
-  background-color: $gray-300;
   width: 1px;
   top: 0;
   height: 100%;
   margin-left: 8px;
-}
-%split-button::before {
-  @extend %as-pseudo;
-  height: 16px;
+  background-color: $gray-300;
 }
 %sort-button::before {
-  @extend %with-sort-icon;
-  margin-top: 8px;
+  @extend %with-sort-mask, %as-pseudo;
+  position: relative;
+  top: 4px;
   width: 16px;
   height: 16px;
 }
+%sort-button::after {
+  top: 0 !important;
+}
diff --git a/ui-v2/app/styles/components/filter-bar.scss b/ui-v2/app/styles/components/filter-bar.scss
index 595e30e83..c27212a63 100644
--- a/ui-v2/app/styles/components/filter-bar.scss
+++ b/ui-v2/app/styles/components/filter-bar.scss
@@ -3,6 +3,10 @@
 .filter-bar {
   @extend %filter-bar;
 }
+%filter-bar {
+  position: relative;
+  z-index: 3;
+}
 %filter-bar:not(.with-sort) {
   @extend %filter-bar-reversed;
 }
diff --git a/ui-v2/app/styles/components/filter-bar/layout.scss b/ui-v2/app/styles/components/filter-bar/layout.scss
index 762622008..563b0d067 100644
--- a/ui-v2/app/styles/components/filter-bar/layout.scss
+++ b/ui-v2/app/styles/components/filter-bar/layout.scss
@@ -19,6 +19,7 @@
 }
 %filter-bar fieldset:first-child:not(:last-child) {
   flex: 1 1 auto;
+  margin-right: 12px;
 }
 %filter-bar-reversed fieldset:first-child:not(:last-child) {
   flex: 0 1 auto;
diff --git a/ui-v2/app/styles/components/filter-bar/skin.scss b/ui-v2/app/styles/components/filter-bar/skin.scss
index d0c4de171..f6090f4b7 100644
--- a/ui-v2/app/styles/components/filter-bar/skin.scss
+++ b/ui-v2/app/styles/components/filter-bar/skin.scss
@@ -8,7 +8,7 @@
 }
 // TODO: Move this elsewhere
 @media #{$--horizontal-selects} {
-  %filter-bar label:not(:last-child) {
+  %filter-bar .type-radio:not(:last-child) {
     border-right: $decor-border-100;
   }
 }
diff --git a/ui-v2/app/styles/components/popover-select.scss b/ui-v2/app/styles/components/popover-select.scss
index c8abc94b6..928eaf81a 100644
--- a/ui-v2/app/styles/components/popover-select.scss
+++ b/ui-v2/app/styles/components/popover-select.scss
@@ -1,22 +1,6 @@
 .popover-select {
   @extend %popover-select;
 }
-%popover-select {
-  @extend %popover-menu;
-}
 %popover-select label > * {
-  @extend %split-button, %sort-button;
-  height: 35px;
-}
-%popover-select label > *::after {
-  top: 0 !important;
-  margin-left: 16px;
-}
-%popover-select {
-  z-index: 3;
-  padding-left: 12px;
-  height: 100%;
-}
-%popover-select label {
-  border-right: none !important;
+  @extend %sort-button;
 }