Fix split button markup, visuals, performance (#4837)

- Refactor the component markup to follow the HTML5 specification - Improve the component architecture so that there is a clear separation between "display" and "logic" elements - Improve performance with removing unnecessary focus trapping (the base Context Menu takes care of that) and refs - Fix visual glitches (bottom outline not shown in some cases, see screenshots) - Add test attributes to improve the component testability
apostrophecms · Jan 20, 2025 · 24ee40f · 24ee40f
1 parent b5cbfa1
commit 24ee40f
Show file tree

Hide file tree

Showing 2 changed files with 53 additions and 53 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,9 +4,10 @@
 
 ### Fixes
 
-* Fixes ability to change color hue by clicking the color hue bar rather than dragging the indicator
-* Prevents the rich text control bar from closing while using certain UI within the color picker
+* Fixes ability to change color hue by clicking the color hue bar rather than dragging the indicator.
+* Prevents the rich text control bar from closing while using certain UI within the color picker.
 * Saving a document via the dialog box properly refreshes the main content area when on a "show page" (when the context document is a piece rather than a page)
+* Fixes the `AposButtonSplit` markup to follow the HTML5 specification, optimizes the component performance, visuals and testability.
 
 ### Adds
 

diff --git a/modules/@apostrophecms/ui/ui/apos/components/AposButtonSplit.vue b/modules/@apostrophecms/ui/ui/apos/components/AposButtonSplit.vue
@@ -1,11 +1,16 @@
 <template>
-  <div class="apos-button-split" :class="modifiers">
+  <div
+    class="apos-button-split"
+    :class="modifiers"
+    :data-apos-test-button-split-type="type"
+  >
     <AposButton
       class="apos-button-split__button"
       v-bind="button"
       :label="label"
       :disabled="disabled"
       :tooltip="tooltip"
+      data-apos-test-button-split-submit
       @click="emit('click', action)"
     />
     <AposContextMenu
@@ -15,57 +20,58 @@
       :button="contextMenuButton"
       :disabled="disabled"
       menu-placement="bottom-end"
+      data-apos-test-button-split-trigger
       @open="menuOpen"
       @close="menuClose"
     >
-      <dl
+      <ul
         class="apos-button-split__menu__dialog"
         role="menu"
         :aria-label="menuLabel"
+        data-apos-test-button-split-menu
       >
-        <button
+        <li
           v-for="item in menu"
           :key="item.action"
-          ref="choices"
           class="apos-button-split__menu__dialog-item"
           :class="{ 'apos-is-selected': item.action === action }"
           :aria-checked="item.action === action ? 'true' : 'false'"
+          :data-apos-test-button-split-item="item.action"
           role="menuitemradio"
-          :value="item.action"
-          @click="selectionHandler(item.action)"
-          @keydown="cycleElementsToFocus"
         >
+          <button
+            class="apos-button-split__menu__dialog-button"
+            :value="item.action"
+            :data-apos-test-button-split-item-trigger="item.action"
+            @click="selectionHandler(item.action)"
+          >
+            <span style="display: none;">
+              {{ $t(item.label) }}
+            </span>
+          </button>
           <AposIndicator
             v-if="action === item.action"
             class="apos-button-split__menu__dialog-check"
             icon="check-bold-icon"
             :icon-size="18"
             icon-color="var(--a-primary)"
           />
-          <dt class="apos-button-split__menu__dialog-label">
+          <span class="apos-button-split__menu__dialog-label">
             {{ $t(item.label) }}
-          </dt>
-          <dd v-if="item.description" class="apos-button-split__menu__dialog-description">
+          </span>
+          <span v-if="item.description" class="apos-button-split__menu__dialog-description">
             {{ $t(item.description) }}
-          </dd>
-        </button>
-      </dl>
+          </span>
+        </li>
+      </ul>
     </AposContextMenu>
   </div>
 </template>
 
 <script setup>
 import {
-  ref, computed, watch, nextTick
+  ref, computed, watch
 } from 'vue';
-import { useAposFocus } from 'Modules/@apostrophecms/modal/composables/AposFocus';
-
-const {
-  elementsToFocus,
-  cycleElementsToFocus,
-  focusElement,
-  focusLastModalFocusedElement
-} = useAposFocus();
 
 const props = defineProps({
   menu: {
@@ -111,7 +117,6 @@ const button = ref({
   attrs: props.attrs
 });
 const contextMenu = ref();
-const choices = ref([]);
 const contextMenuButton = ref({
   iconOnly: true,
   icon: 'chevron-down-icon',
@@ -151,27 +156,6 @@ function initialize() {
   setButton(initial);
 }
 
-function trapFocus() {
-  const selectedElementIndex = props.menu.findIndex(i => i.action === action.value) || 0;
-
-  // use map to keep items order:
-  elementsToFocus.value = props.menu.map(
-    ({ action }) => choices.value.find(choice => {
-      return choice.value === action;
-    })
-  );
-
-  focusElement(elementsToFocus.value[selectedElementIndex]);
-}
-function menuOpen() {
-  nextTick(() => {
-    trapFocus();
-  });
-}
-
-function menuClose() {
-  focusLastModalFocusedElement();
-}
 </script>
 <style lang="scss" scoped>
   .apos-button-split {
@@ -182,28 +166,30 @@ function menuClose() {
     display: flex;
     flex-direction: column;
     margin: 0;
+    padding: 0;
     min-width: 300px;
+    list-style: none;
   }
 
   .apos-button-split__menu__dialog-item {
-    @include apos-button-reset();
     @include apos-transition();
 
     & {
+      position: relative;
       padding: $spacing-base + $spacing-half $spacing-double $spacing-base + $spacing-half $spacing-quadruple;
       border-bottom: 1px solid var(--a-base-9);
     }
 
-    &:hover,
-    &:focus,
-    &:active,
+    &:has(.apos-button-split__menu__dialog-button:hover),
+    &:focus-within,
+    &:has(.apos-button-split__menu__dialog-button:active),
     &.apos-is-selected {
       background-color: var(--a-base-9);
     }
 
-    &:focus,
-    &:active {
-      outline: 1px solid var(--a-primary);
+    &:focus-within,
+    &:has(.apos-button-split__menu__dialog-button:active) {
+      box-shadow: inset 0 0 0 1px var(--a-primary);
     }
 
     &:last-child {
@@ -212,6 +198,18 @@ function menuClose() {
     }
   }
 
+  .apos-button-split__menu__dialog-button {
+    @include apos-button-reset();
+
+    & {
+      position: absolute;
+      outline: none;
+      background: transparent;
+      inset: 0;
+      cursor: pointer;
+    }
+  }
+
   .apos-button-split__menu__dialog-check {
     position: absolute;
     left: $spacing-base;
@@ -221,6 +219,7 @@ function menuClose() {
     @include type-large;
 
     & {
+      display: block;
       margin-bottom: $spacing-half;
     }
   }