fix: simplify skintone listbox (#336)

src/picker/components/Picker/Picker.html

@@ -31,8 +31,13 @@
       <label class="sr-only" for="search">{i18n.searchLabel}</label>
       <span id="search-description" class="sr-only">{i18n.searchDescription}</span>
     </div>
-    <!-- For the pattern used for the skintone dropdown, see
-         https://www.w3.org/WAI/ARIA/apg/patterns/combobox/examples/combobox-select-only/ -->
+    <!-- For the pattern used for the skintone dropdown, see:
+         https://www.w3.org/WAI/ARIA/apg/patterns/combobox/examples/combobox-select-only/
+         The one case where we deviate from the example is that we move focus from the button to the
+         listbox. (The example uses a combobox, so it's not exactly the same.) This was tested in NVDA and VoiceOver.
+         Note that Svelte's a11y checker will warn if the listbox does not have a tabindex.
+         https://github.com/sveltejs/svelte/blob/3bc791b/site/content/docs/06-accessibility-warnings.md#a11y-aria-activedescendant-has-tabindex
+         -->
     <div class="skintone-button-wrapper {skinTonePickerExpandedAfterAnimation ? 'expanded' : ''}">
       <button id="skintone-button"
               class="emoji {skinTonePickerExpanded ? 'hide-focus' : ''}"
@@ -47,31 +52,25 @@
       </button>
     </div>
     <span id="skintone-description" class="sr-only">{i18n.skinToneDescription}</span>
-    <!-- In this pattern, the listbox is not focusable, only the button is focusable.
-         This was tested in NVDA and VoiceOver and worked in both. -->
-    <!-- svelte-ignore a11y-aria-activedescendant-has-tabindex -->
     <div id="skintone-list"
-         class="skintone-list {skinTonePickerExpanded ? '' : 'hidden no-animate'}"
+         class="skintone-list hide-focus {skinTonePickerExpanded ? '' : 'hidden no-animate'}"
          style="transform:translateY({ skinTonePickerExpanded ? 0 : 'calc(-1 * var(--num-skintones) * var(--total-emoji-size))'})"
          role="listbox"
          aria-label={i18n.skinTonesLabel}
          aria-activedescendant="skintone-{activeSkinTone}"
          aria-hidden={!skinTonePickerExpanded}
+         tabindex="-1"
          on:focusout={onSkinToneOptionsFocusOut}
          on:click={onSkinToneOptionsClick}
          on:keydown={onSkinToneOptionsKeydown}
          on:keyup={onSkinToneOptionsKeyup}
          bind:this={skinToneDropdown}>
       {#each skinTones as skinTone, i (skinTone)}
-        <!-- would use a button here, but iOS Safari misreports relatedTarget in that case, see issue #14 -->
-        <!-- see https://stackoverflow.com/a/42764495/680742 -->
-        <!-- see also https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#Clicking_and_focus -->
         <div    id="skintone-{i}"
-                class="emoji hide-focus {i === activeSkinTone ? 'active' : ''}"
+                class="emoji {i === activeSkinTone ? 'active' : ''}"
                 aria-selected={i === activeSkinTone}
                 role="option"
                 title={i18n.skinTones[i]}
-                tabindex="-1"
                 aria-label={i18n.skinTones[i]}>
           {skinTone}
         </div>

src/picker/components/Picker/Picker.js

@@ -1,5 +1,5 @@
 /* eslint-disable prefer-const,no-labels,no-inner-declarations */
-import { onMount, tick } from 'svelte'
+import { onMount } from 'svelte'
 import { groups as defaultGroups, customGroup } from '../../groups'
 import { MIN_SEARCH_TEXT_LENGTH, NUM_SKIN_TONES } from '../../../shared/constants'
 import { requestIdleCallback } from '../../utils/requestIdleCallback'
@@ -89,9 +89,6 @@ const labelWithSkin = (emoji, currentSkinTone) => (
   uniq([(emoji.name || unicodeWithSkin(emoji, currentSkinTone)), ...(emoji.shortcodes || [])]).join(', ')
 )
 
-// Detect a skintone option button
-const isSkinToneOption = element => /^skintone-/.test(element.id)
-
 //
 // Determine the emoji support level (in requestIdleCallback)
 //
@@ -503,13 +500,7 @@ async function onEmojiClick (event) {
 //
 
 // eslint-disable-next-line no-unused-vars
-async function onSkinToneOptionsClick (event) {
-  const { target } = event
-  if (!isSkinToneOption(target)) {
-    return
-  }
-  halt(event)
-  const skinTone = parseInt(target.id.slice(9), 10) // remove 'skintone-' prefix
+function changeSkinTone (skinTone) {
   currentSkinTone = skinTone
   skinTonePickerExpanded = false
   focus('skintone-button')
@@ -518,12 +509,24 @@ async function onSkinToneOptionsClick (event) {
 }
 
 // eslint-disable-next-line no-unused-vars
-async function onClickSkinToneButton (event) {
+function onSkinToneOptionsClick (event) {
+  const { target: { id } } = event
+  const match = id && id.match(/^skintone-(\d)/) // skintone option format
+  if (!match) { // not a skintone option
+    return
+  }
+  halt(event)
+  const skinTone = parseInt(match[1], 10) // remove 'skintone-' prefix
+  changeSkinTone(skinTone)
+}
+
+// eslint-disable-next-line no-unused-vars
+function onClickSkinToneButton (event) {
   skinTonePickerExpanded = !skinTonePickerExpanded
   activeSkinTone = currentSkinTone
   if (skinTonePickerExpanded) {
     halt(event)
-    requestAnimationFrame(() => focus(`skintone-${activeSkinTone}`))
+    requestAnimationFrame(() => focus('skintone-list'))
   }
 }
 
@@ -549,8 +552,6 @@ function onSkinToneOptionsKeydown (event) {
   const changeActiveSkinTone = async nextSkinTone => {
     halt(event)
     activeSkinTone = nextSkinTone
-    await tick()
-    focus(`skintone-${activeSkinTone}`)
   }
 
   switch (event.key) {
@@ -565,7 +566,8 @@ function onSkinToneOptionsKeydown (event) {
     case 'Enter':
       // enter on keydown, space on keyup. this is just how browsers work for buttons
       // https://lists.w3.org/Archives/Public/w3c-wai-ig/2019JanMar/0086.html
-      return onSkinToneOptionsClick(event)
+      halt(event)
+      return changeSkinTone(activeSkinTone)
     case 'Escape':
       halt(event)
       skinTonePickerExpanded = false
@@ -582,16 +584,16 @@ function onSkinToneOptionsKeyup (event) {
     case ' ':
       // enter on keydown, space on keyup. this is just how browsers work for buttons
       // https://lists.w3.org/Archives/Public/w3c-wai-ig/2019JanMar/0086.html
-      return onSkinToneOptionsClick(event)
+      halt(event)
+      return changeSkinTone(activeSkinTone)
   }
 }
 
 // eslint-disable-next-line no-unused-vars
 async function onSkinToneOptionsFocusOut (event) {
-  // On blur outside of the skintone options, collapse the skintone picker.
-  // Except if focus is just moving to another skintone option, e.g. pressing up/down to change focus
+  // On blur outside of the skintone listbox, collapse the skintone picker.
   const { relatedTarget } = event
-  if (!relatedTarget || !isSkinToneOption(relatedTarget)) {
+  if (!relatedTarget || relatedTarget.id !== 'skintone-list') {
     skinTonePickerExpanded = false
   }
 }

test/spec/picker/Picker.test.js

@@ -21,7 +21,7 @@ describe('Picker tests', () => {
       }
     }
   })
-  const { getAllByRole, getByRole, queryAllByRole } = proxy
+  const { getAllByRole, getByRole, queryAllByRole, queryByRole } = proxy
 
   const activeElement = () => container.getRootNode().activeElement
 
@@ -85,13 +85,23 @@ describe('Picker tests', () => {
     })
 
     await openSkintoneListbox(container)
-    await waitFor(() => expect(getByRole('option', { name: 'Default', selected: true }))
-      .toBe(activeElement()))
+
+    await waitFor(() => (
+      expect(
+        getByRole('option', { name: 'Default', selected: true }).id)
+        .toBe(queryByRole('listbox', { name: 'Skin tones' }).getAttribute('aria-activedescendant'))
+    )
+    )
 
     const pressKeyAndExpectActiveOption = async (key, name) => {
       await new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(resolve))) // delay
       await fireEvent.keyDown(activeElement(), { key, code: key })
-      await waitFor(() => expect(getByRole('option', { name, selected: true })).toBe(activeElement()))
+      await waitFor(() => {
+        const selectedOption = getByRole('option', { name, selected: true })
+        return expect(selectedOption.id).toBe(
+          queryByRole('listbox', { name: 'Skin tones' }).getAttribute('aria-activedescendant')
+        )
+      })
     }
 
     await pressKeyAndExpectActiveOption('ArrowDown', 'Light')
@@ -106,7 +116,7 @@ describe('Picker tests', () => {
     await pressKeyAndExpectActiveOption('End', 'Dark')
     await pressKeyAndExpectActiveOption('End', 'Dark')
 
-    await fireEvent.click(activeElement(), { key: 'Enter', code: 'Enter' })
+    await fireEvent.keyDown(activeElement(), { key: 'Enter', code: 'Enter' })
 
     await waitFor(() => expect(event && event.detail).toStrictEqual({ skinTone: 5 }))
     await waitFor(() => expect(queryAllByRole('listbox', { name: 'Skin tones' })).toHaveLength(0))
@@ -152,8 +162,12 @@ describe('Picker tests', () => {
 
   test('Escape key dismisses skintone listbox', async () => {
     await openSkintoneListbox(container)
-    await waitFor(() => expect(getByRole('option', { name: 'Default', selected: true }))
-      .toBe(activeElement()))
+    await waitFor(() => (
+      expect(
+        getByRole('option', { name: 'Default', selected: true }).id)
+        .toBe(queryByRole('listbox', { name: 'Skin tones' }).getAttribute('aria-activedescendant'))
+    )
+    )
 
     await fireEvent.keyDown(activeElement(), { key: 'Escape', code: 'Escape' })
 
@@ -354,7 +368,6 @@ describe('Picker tests', () => {
     await waitFor(() => expect(emoji && emoji.name === 'donkey'))
   }, 5000)
 
-  // TODO: re-enable this behavior. See https://github.com/nolanlawson/emoji-picker-element/issues/14
   test('Closes skintone picker when blurred', async () => {
     fireEvent.click(getByRole('button', { name: /Choose a skin tone/ }))
     await waitFor(() => expect(getByRole('listbox', { name: 'Skin tones' })).toBeVisible())
@@ -364,6 +377,16 @@ describe('Picker tests', () => {
     await waitFor(() => expect(queryAllByRole('listbox', { name: 'Skin tones' })).toHaveLength(0))
   })
 
+  test('Closes skintone picker when focus moves to skintone trigger button', async () => {
+    const chooseSkintoneButton = getByRole('button', { name: /Choose a skin tone/ })
+    fireEvent.click(chooseSkintoneButton)
+    await waitFor(() => expect(getByRole('listbox', { name: 'Skin tones' })).toBeVisible())
+    // Simulating a focusout event is hard, have to both focus and blur
+    chooseSkintoneButton.focus()
+    fireEvent.focusOut(getByRole('listbox', { name: 'Skin tones' }))
+    await waitFor(() => expect(queryAllByRole('listbox', { name: 'Skin tones' })).toHaveLength(0))
+  })
+
   test('Custom emoji with categories', async () => {
     picker.customEmoji = [
       {

test/spec/picker/shared.js

@@ -16,6 +16,11 @@ export async function openSkintoneListbox (container) {
   // JSDom doesn't fire transitionend events, so we do it manually here
   // https://github.com/jsdom/jsdom/issues/1781#issuecomment-467935000
   fireEvent(getByRole(container, 'listbox', { name: 'Skin tones' }), new Event('transitionend'))
+
+  await waitFor(() => (
+    expect(container.getRootNode().activeElement)
+      .toBe(getByRole(container, 'listbox', { name: 'Skin tones' }))
+  ))
 }
 
 export function checkEmojiEquals (actual, expected) {