From ca88e63d1946ad73aac73509e6419cb6df098c35 Mon Sep 17 00:00:00 2001
From: Joey Arhar <jarhar@chromium.org>
Date: Thu, 5 Oct 2023 17:26:48 -0700
Subject: [PATCH] selectlist: implement new keyboard behavior

This patch implements several keyboard behaviors:
- TODO

This patch also implements the resolution here to stop changing the
visible value of the selected option while the user is switching the
focused option using the arrow keys:
https://github.com/openui/open-ui/issues/742

Fixed: 1422275
Change-Id: If5e7328ad739f9c7339dcd17561c57875d4255e7
---
 .../selectlist-events.tentative.html          |  49 ++---
 ...electlist-keyboard-behavior.tentative.html | 182 ++++++++++++++++++
 .../selectlist-keyboard.tentative.html        |  57 ++++--
 3 files changed, 238 insertions(+), 50 deletions(-)
 create mode 100644 html/semantics/forms/the-selectlist-element/selectlist-keyboard-behavior.tentative.html

diff --git a/html/semantics/forms/the-selectlist-element/selectlist-events.tentative.html b/html/semantics/forms/the-selectlist-element/selectlist-events.tentative.html
index 94577815cd075c7..a88a3b1f7da0b62 100644
--- a/html/semantics/forms/the-selectlist-element/selectlist-events.tentative.html
+++ b/html/semantics/forms/the-selectlist-element/selectlist-events.tentative.html
@@ -130,20 +130,15 @@
 
   promise_test(async () => {
     const selectList = document.getElementById("selectList2");
-    let input_event_count = 0;
-    let change_event_count = 0;
+    const events = [];
 
     selectList.addEventListener("input", (e) => {
-      assert_true(e.composed, "input event should be composed");
-      assert_equals(input_event_count, 0, "input event should not fire twice");
-      assert_equals(change_event_count, 0, "input event should not fire before change");
-      input_event_count++;
+      assert_true(e.composed, "input event should be composed.");
+      events.push('input');
     });
     selectList.addEventListener("change", (e) => {
-      assert_false(e.composed, "change event should not be composed");
-      assert_equals(input_event_count, 1, "change event should fire after input");
-      assert_equals(change_event_count, 0, "change event should not fire twice");
-      change_event_count++;
+      assert_false(e.composed, "change event should not be composed.");
+      events.push('change');
     });
 
     await clickOn(selectList);
@@ -151,47 +146,39 @@
     await test_driver.send_keys(selectList, KEY_CODE_MAP.Enter);
     assert_false(selectList.open);
     assert_equals(selectList.value, "one");
-    assert_equals(input_event_count, 0, "input event shouldn't fire if value wasn't changed");
-    assert_equals(change_event_count, 0, "change event shouldn't fire if value wasn't changed");
+    assert_array_equals(events, [], "input and change shouldn't fire if value wansn't changed.");
 
     await clickOn(selectList);
     assert_true(selectList.open);
     await test_driver.send_keys(selectList, KEY_CODE_MAP.ArrowDown);
-    assert_equals(selectList.value, "two", "value should change when user switches options with arrow key");
-    assert_equals(input_event_count, 1, "input event should fire when user switches options with arrow key");
-    assert_equals(change_event_count, 0, "change event shouldn't fire until popover is closed");
+    assert_equals(selectList.value, "one", "value shouldn't change when user switches options with arrow key.");
+    assert_array_equals(events, ['input'], "input event should fire when user switches options with arrow key.");
 
     await test_driver.send_keys(selectList, KEY_CODE_MAP.Enter);
     assert_equals(selectList.value, "two");
-    assert_equals(input_event_count, 1, "input event should have fired");
-    assert_equals(change_event_count, 1, "change event should have fired");
-  }, "<selectlist> should fire input and change events when new option is selected");
+    assert_array_equals(events, ['input', 'input', 'change'], "input and change should fire after pressing enter.");
+  }, "<selectlist> should fire input and change events when new option is selected.");
 
   promise_test(async () => {
     const selectList = document.getElementById("selectList3");
-    let input_event_count = 0;
-    let change_event_count = 0;
+    const events = [];
 
     selectList.addEventListener("input", (e) => {
-      assert_true(e.composed, "input event should be composed");
-      assert_equals(input_event_count, 0, "input event should not fire twice");
-      assert_equals(change_event_count, 0, "input event should not fire before change");
-      input_event_count++;
+      assert_true(e.composed, "input event should be composed.");
+      events.push('input');
     });
     selectList.addEventListener("change", (e) => {
-      assert_false(e.composed, "change event should not be composed");
-      assert_equals(input_event_count, 1, "change event should fire after input");
-      assert_equals(change_event_count, 0, "change event should not fire twice");
-      change_event_count++;
+      assert_false(e.composed, "change event should not be composed.");
+      events.push('change');
     });
 
     await clickOn(selectList);
     assert_true(selectList.open);
     await test_driver.send_keys(selectList, KEY_CODE_MAP.ArrowDown);
+    assert_array_equals(events, ['input'], "input event should have fired after ArrowDown.");
     await test_driver.send_keys(selectList, KEY_CODE_MAP.Enter);
-    assert_equals(input_event_count, 1, "input event should have fired");
-    assert_equals(change_event_count, 1, "change event should have fired");
-  }, "<selectlist> should fire input and change events even when new selected option has the same value as the old");
+    assert_array_equals(events, ['input', 'input', 'change'], "input and change should fire after pressing Enter.");
+  }, "<selectlist> should fire input and change events even when new selected option has the same value as the old.");
 
   promise_test(async () => {
     const selectList = document.getElementById("selectList4");
diff --git a/html/semantics/forms/the-selectlist-element/selectlist-keyboard-behavior.tentative.html b/html/semantics/forms/the-selectlist-element/selectlist-keyboard-behavior.tentative.html
new file mode 100644
index 000000000000000..a70ab7e6a5d5de5
--- /dev/null
+++ b/html/semantics/forms/the-selectlist-element/selectlist-keyboard-behavior.tentative.html
@@ -0,0 +1,182 @@
+<!DOCTYPE html>
+<link rel=author href="mailto:jarhar@chromium.org">
+<link rel=help href="https://bugs.chromium.org/p/chromium/issues/detail?id=1422275">
+<link rel=help href="https://github.com/openui/open-ui/issues/433#issuecomment-1452461404">
+<link rel=help href="https://github.com/openui/open-ui/issues/386#issuecomment-1452469497">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="/resources/testdriver.js"></script>
+<script src="/resources/testdriver-vendor.js"></script>
+<script src="/resources/testdriver-actions.js"></script>
+
+<form></form>
+
+<div id=notform>
+  <selectlist id=defaultbutton>
+    <option class=one>one</option>
+    <option class=two>two</option>
+    <option class=three>three</option>
+  </selectlist>
+
+  <selectlist id=custombutton>
+    <button type=selectlist>custom button</button>
+    <option class=one>one</option>
+    <option class=two>two</option>
+    <option class=three>three</option>
+  </selectlist>
+</div>
+
+<script>
+const Enter = '\uE007';
+const Escape = '\uE00C';
+const ArrowLeft = '\uE012';
+const ArrowUp = '\uE013';
+const ArrowRight = '\uE014';
+const ArrowDown = '\uE015';
+const Space = ' ';
+const form = document.querySelector('form');
+const notform = document.getElementById('notform');
+
+for (const id of ['defaultbutton', 'custombutton']) {
+  const selectlist = document.getElementById(id);
+
+  async function closeListbox() {
+    await test_driver.click(selectlist);
+  }
+
+  function addCloseCleanup(t) {
+    t.add_cleanup(async () => {
+      if (selectlist.matches(':open')) {
+        await closeListbox();
+      }
+      if (selectlist.matches(':open')) {
+        throw new Error('selectlist failed to close!');
+      }
+      selectlist.value = 'one';
+    });
+  }
+
+  promise_test(async t => {
+    addCloseCleanup(t);
+    // TODO(http://crbug.com/1350299): When focus for custom buttons is fixed,
+    // then we shouldn't need to explicitly focus the custom button like this
+    // anymore.
+    const customButton = selectlist.querySelector('button');
+    if (customButton) {
+      customButton.focus();
+    } else {
+      selectlist.focus();
+    }
+    assert_false(selectlist.matches(':open'),
+      'The selectlist should initially be closed.');
+    await test_driver.send_keys(selectlist, Space);
+    assert_true(selectlist.matches(':open'),
+      'The selectlist should be open after pressing space.');
+  }, `${id}: When the listbox is closed, spacebar should open the listbox.`);
+
+  promise_test(async t => {
+    addCloseCleanup(t);
+    selectlist.value = 'two';
+    selectlist.focus();
+    assert_false(selectlist.matches(':open'),
+      'The selectlist should initially be closed.');
+
+    await test_driver.send_keys(selectlist, ArrowLeft);
+    assert_true(selectlist.matches(':open'),
+      'Arrow left should open the listbox.');
+    assert_equals(selectlist.value, 'two',
+      'Arrow left should not change the selected value.');
+    await closeListbox();
+
+    await test_driver.send_keys(selectlist, ArrowUp);
+    assert_true(selectlist.matches(':open'),
+      'Arrow up should open the listbox.');
+    assert_equals(selectlist.value, 'two',
+      'Arrow up should not change the selected value.');
+    await closeListbox();
+
+    await test_driver.send_keys(selectlist, ArrowRight);
+    assert_true(selectlist.matches(':open'),
+      'Arrow right should open the listbox.');
+    assert_equals(selectlist.value, 'two',
+      'Arrow right should not change the selected value.');
+    await closeListbox();
+
+    await test_driver.send_keys(selectlist, ArrowDown);
+    assert_true(selectlist.matches(':open'),
+      'Arrow down should open the listbox.');
+    assert_equals(selectlist.value, 'two',
+      'Arrow down should not change the selected value.');
+  }, `${id}: When the listbox is closed, all arrow keys should open the listbox.`);
+
+  promise_test(async t => {
+    addCloseCleanup(t);
+
+    // TODO(http://crbug.com/1350299): When focus for custom buttons is fixed,
+    // then we shouldn't need to explicitly use the custom button like this
+    // anymore.
+    const customButton = selectlist.querySelector('button');
+    if (customButton) {
+      await test_driver.send_keys(customButton, Enter);
+    } else {
+      await test_driver.send_keys(selectlist, Enter);
+    }
+    assert_false(selectlist.matches(':open'),
+      'Enter should not open the listbox when outside a form.');
+
+    form.appendChild(selectlist);
+    let formWasSubmitted = false;
+    form.addEventListener('submit', event => {
+      event.preventDefault();
+      formWasSubmitted = true;
+    }, {once: true});
+    if (customButton) {
+      await test_driver.send_keys(customButton, Enter);
+    } else {
+      await test_driver.send_keys(selectlist, Enter);
+    }
+    assert_true(formWasSubmitted,
+      'Enter should submit the form when the listbox is closed.');
+    assert_false(selectlist.matches(':open'),
+      'Enter should not open the listbox when it is in a form.');
+  }, `${id}: When the listbox is closed, the enter key should submit the form or do nothing.`);
+
+  promise_test(async t => {
+    addCloseCleanup(t);
+    const optionOne = selectlist.querySelector('.one');
+    const optionTwo = selectlist.querySelector('.two');
+    const optionThree = selectlist.querySelector('.three');
+
+    selectlist.value = 'two';
+    await test_driver.click(selectlist);
+    assert_true(selectlist.matches(':open'),
+      'The selectlist should open when clicked.');
+    assert_equals(document.activeElement, optionTwo,
+      'The selected option should receive initial focus.');
+
+    await test_driver.send_keys(document.activeElement, ArrowDown);
+    assert_equals(document.activeElement, optionThree,
+      'The next option should receive focus when the down arrow key is pressed.');
+    assert_equals(selectlist.value, 'two',
+      'The selectlists value should not change when focusing another option.');
+
+    await test_driver.send_keys(document.activeElement, ArrowUp);
+    assert_equals(document.activeElement, optionTwo,
+      'The previous option should receive focus when the up arrow key is pressed.');
+    assert_equals(selectlist.value, 'two',
+      'The selectlists value should not change when focusing another option.');
+
+    await test_driver.send_keys(document.activeElement, ArrowUp);
+    assert_equals(document.activeElement, optionOne,
+      'The first option should be selected.');
+    assert_equals(selectlist.value, 'two',
+      'The selectlists value should not change when focusing another option.');
+
+    await test_driver.send_keys(document.activeElement, Enter);
+    assert_false(selectlist.matches(':open'),
+      'The listbox should be closed after pressing enter.');
+    assert_equals(selectlist.value, 'one',
+      'The selectlists value should change after pressing enter on a different option.');
+  }, `${id}: When the listbox is open, the enter key should commit the selected option.`);
+}
+</script>
diff --git a/html/semantics/forms/the-selectlist-element/selectlist-keyboard.tentative.html b/html/semantics/forms/the-selectlist-element/selectlist-keyboard.tentative.html
index 38c5fd7dc4ed95c..5d1c65e9413d0a6 100644
--- a/html/semantics/forms/the-selectlist-element/selectlist-keyboard.tentative.html
+++ b/html/semantics/forms/the-selectlist-element/selectlist-keyboard.tentative.html
@@ -6,10 +6,10 @@
 <script src="/resources/testdriver-actions.js"></script>
 <script src="/resources/testdriver-vendor.js"></script>
   <selectlist id="selectList0">
-    <div id="selectList0-button0" slot="button" behavior="button" tabindex="0">button</div>
-    <option>one</option>
-    <option>two</option>
-    <option>three</option>
+    <button id="selectList0-button0" type=selectlist>button</button>
+    <option class=one>one</option>
+    <option class=two>two</option>
+    <option class=three>three</option>
   </selectlist>
 
   <selectlist id="selectList1">
@@ -17,17 +17,17 @@
   </selectlist>
 
   <selectlist id="selectList2" disabled>
-    <div id="selectList2-button0" slot="button" behavior="button" tabindex="0">button</div>
+    <button id="selectList2-button0" type=selectlist>button</button>
     <option disabled>one</option>
     <option>two</option>
     <option>three</option>
   </selectlist>
 
   <selectlist id="selectList3">
-    <div id="selectList3-button0" slot="button" behavior="button" tabindex="0">button</div>
-    <option>one</option>
+    <button id="selectList3-button0" type=selectlist>button</button>
+    <option class=one>one</option>
     <option disabled>two</option>
-    <option>three</option>
+    <option class=three>three</option>
   </selectlist>
 <script>
 // See https://w3c.github.io/webdriver/#keyboard-actions
@@ -52,31 +52,45 @@
   assert_false(selectList.open, "selectlist should not be initially open");
 
   await test_driver.send_keys(button, KEY_CODE_MAP.Enter);
-  assert_true(selectList.open, "Enter key should open selectlist");
+  assert_false(selectList.open, "Enter key shouldn't open selectlist");
+  await test_driver.send_keys(button, KEY_CODE_MAP.Space);
+  assert_true(selectList.open, "Space key should open selectlist");
   assert_equals(selectList.value, "one");
 
   await test_driver.send_keys(selectList, KEY_CODE_MAP.ArrowDown);
-  assert_equals(selectList.value, "two", "Down arrow should go to next option");
+  assert_equals(document.activeElement, selectList.querySelector('.two'),
+    "Down arrow should focus the next option.");
+  assert_equals(selectList.value, "one", "Down arrow should not commit the newly focused option.");
 
   await test_driver.send_keys(selectList, KEY_CODE_MAP.ArrowDown);
-  assert_equals(selectList.value, "three", "Down arrow should go to next option");
+  assert_equals(document.activeElement, selectList.querySelector('.three'),
+    "Down arrow should focus the next option.");
+  assert_equals(selectList.value, "one", "Down arrow should not commit the newly focused option.");
 
   await test_driver.send_keys(selectList, KEY_CODE_MAP.ArrowDown);
-  assert_equals(selectList.value, "three", "Down arrow should do nothing if already at the last option");
+  assert_equals(document.activeElement, selectList.querySelector('.three'),
+    "Down arrow should do nothing if already at the last option.");
+  assert_equals(selectList.value, "one", "Down arrow should not commit the newly focused option.");
 
   await test_driver.send_keys(selectList, KEY_CODE_MAP.ArrowUp);
-  assert_equals(selectList.value, "two", "Up arrow should go to the previous option");
+  assert_equals(document.activeElement, selectList.querySelector('.two'),
+    "Up arrow should focus the previous option.");
+  assert_equals(selectList.value, "one", "Up arrow should not commit the newly focused option.");
 
   await test_driver.send_keys(selectList, KEY_CODE_MAP.ArrowUp);
-  assert_equals(selectList.value, "one", "Up arrow should go to the previous option");
+  assert_equals(document.activeElement, selectList.querySelector('.one'),
+    "Up arrow should focus the previous option.");
+  assert_equals(selectList.value, "one", "Up arrow should not commit the newly focused option.");
 
   await test_driver.send_keys(selectList, KEY_CODE_MAP.ArrowUp);
-  assert_equals(selectList.value, "one", "Up arrow should do nothing if already at the first option");
+  assert_equals(document.activeElement, selectList.querySelector('.one'),
+    "Up arrow should do nothing if already at the first option.");
+  assert_equals(selectList.value, "one", "Up arrow should not commit the newly focused option.");
 
   await test_driver.send_keys(selectList, KEY_CODE_MAP.Enter);
   assert_false(selectList.open, "Enter key should close selectlist");
 
-  await test_driver.send_keys(selectList, " ");
+  await test_driver.send_keys(button, KEY_CODE_MAP.Space);
   assert_true(selectList.open, "Space key should open selectlist");
 
   // This behavior is suspicious (since Space key can open the selectlist),
@@ -111,13 +125,18 @@
   assert_false(selectList3.open, "selectlist should not be initially open");
 
   await test_driver.send_keys(selectList3Button, KEY_CODE_MAP.Enter);
-  assert_true(selectList3.open, "Enter key should open selectlist");
+  assert_false(selectList3.open, "Enter key shouldn't open selectlist");
+
+  await test_driver.send_keys(selectList3Button, KEY_CODE_MAP.Space);
+  assert_true(selectList3.open, "Space key should open selectlist");
   assert_equals(selectList3.value, "one");
 
   await test_driver.send_keys(selectList3, KEY_CODE_MAP.ArrowDown);
-  assert_equals(selectList3.value, "three", "Down arrow should go to next non-disabled option");
+  assert_equals(document.activeElement, selectList3.querySelector('.three'),
+    "Down arrow should go to next non-disabled option");
 
   await test_driver.send_keys(selectList3, KEY_CODE_MAP.ArrowUp);
-  assert_equals(selectList3.value, "one", "Up arrow should go to the previous non-disabled option");
+  assert_equals(document.activeElement, selectList3.querySelector('.one'),
+    "Up arrow should go to the previous non-disabled option");
 }, "Validate Enter, Up/Down Arrow keyboard accessibility support for disabled <selectlist>");
 </script>