Revert behavior to fix failing tests. Add arrow key support. Fixes #65, #54, #63, #44 #68

robdodson · 2017-11-15T05:08:25Z

This reverts the behavior that's currently on master because it was breaking mouse focus and causing most of the tests to fail. It uses a different approach to address the URL bar issue from #15 and adds support for arrow keys to address #54, #63.

This also removes the polyfill for classList. This is in accordance with the W3C's new Polyfill guidance which suggests that polyfills should not bundle other polyfills. Instead, we'll update our docs to mention that the polyfill depends on classList and users should use a service like polyfill.io to add it.

@kloots FYI because I needed to back out the previous onKeyUp change.

alice · 2017-11-15T06:57:33Z

src/focus-ring.js

  var elWithFocusRing;

  var inputTypesWhitelist = {
-    'radio': true,


Should these have stayed in?

I don't think so. That would cause focus-ring to be applied on mouse click because these get checked during the focus event. The reverted version added them because it was checking only on keyUp.

alice · 2017-11-15T06:59:20Z

src/focus-ring.js

   */
-  function onKeyUp(e) {
+  function onKeyDown(e) {
+    const allowedKeys = [9, 37, 38, 39, 40];


Can you comment each of these with what key it represents?

There's a comment above that indicates it's tab and arrow keys but yeah.

alice · 2017-11-15T07:01:04Z

src/focus-ring.js

+  function onKeyDown(e) {
+    const allowedKeys = [9, 37, 38, 39, 40];
+
+    // If the user is holding down a modifier key, abort.


Suggestion: "Ignore keypresses if the user is holding down a modifier key"

alice · 2017-11-15T07:05:02Z

src/focus-ring.js

      return;

-    if (e.keyCode != 9)
+    // If the key can't be found in the list of allowed keys, abort.


Suggestion: "Ignore keypresses which aren't related to keyboard navigation."

alice · 2017-11-15T07:05:58Z

src/focus-ring.js

+
+  /**
+   * On `focus`, add the `focus-ring` class to the target if:
+   * - the target received focus as a result of keyboard navigation


s/navigation/navigation, or/

alice · 2017-11-15T07:09:17Z

src/focus-ring.js

+
+  /**
+   * When the polfyill first loads, assume the user is in keyboard modality.
+   * If any event is received from a fine-grained pointing device (mouse, pointer, touch),


Again, touch isn't fine-grained.

alice · 2017-11-15T07:11:26Z

src/focus-ring.js

+   * If any event is received from a fine-grained pointing device (mouse, pointer, touch),
+   * turn off keyboard modality.
+   * This accounts for situations where focus enters the page from the URL bar.
+   * In that scenario, the keydown event is inconsistent, so we can't use it to detect modality.


I would delete this and the following two lines - more justification than necessary.

alice · 2017-11-15T07:12:36Z

test/index.js

 function browserFilter(browser) {
  return browser.getReleaseName() === 'stable'
-    && ['firefox', 'chrome'].includes(browser.getId());
+    && ['chrome', 'firefox'].includes(browser.getId());


was just double checking something. selenium-assistant stopped killing firefox 57 properly and I wasn't sure if the order in which the browsers opened/closed had anything to do with it. turns out it doesn't. the ordering doesn't matter and the issue isn't actually a show stopper. it just means if you test locally you need to close firefox when you're done. doesn't seem to bother travis though. I filed GoogleChromeLabs/selenium-assistant#105

alice · 2017-11-15T07:14:29Z

test/fixtures/contenteditable-textbox.html

+      https://jsbin.com/cawevo/edit?html,output
+      Add a dummy button so Selenium can use it as the first focus target.
+    -->
+    <button id="start">Dummy</button>


It confused me a little that the id was "start" but the contents was "Dummy".

alice · 2017-11-15T07:15:24Z

test/specs/contenteditable-textbox.js

-  // https://jsbin.com/cawevo/edit?html,output
-  it.skip('should apply .focus-ring on keyboard focus', function() {
-    return matchesKeyboard();
+  // FF won't focus a div with contenteditable if it's the first element on the page.


That sounds like it was a lot of fun to debug 🙄

Not as much fun as https://github.com/WICG/focus-ring/pull/68/files#diff-6d1daa3464793cab0c4c5d8eeb17e778R178

robdodson

Fixed comments and pushing up to master.

robdodson · 2017-11-15T07:53:39Z

src/focus-ring.js

  var elWithFocusRing;

  var inputTypesWhitelist = {
-    'radio': true,


I don't think so. That would cause focus-ring to be applied on mouse click because these get checked during the focus event. The reverted version added them because it was checking only on keyUp.

robdodson · 2017-11-15T07:54:09Z

src/focus-ring.js

   */
-  function onKeyUp(e) {
+  function onKeyDown(e) {
+    const allowedKeys = [9, 37, 38, 39, 40];


There's a comment above that indicates it's tab and arrow keys but yeah.

robdodson · 2017-11-15T07:57:42Z

src/focus-ring.js

+  function onKeyDown(e) {
+    const allowedKeys = [9, 37, 38, 39, 40];
+
+    // If the user is holding down a modifier key, abort.


robdodson · 2017-11-15T07:57:46Z

src/focus-ring.js

      return;

-    if (e.keyCode != 9)
+    // If the key can't be found in the list of allowed keys, abort.


robdodson · 2017-11-15T08:01:13Z

src/focus-ring.js


-  document.addEventListener('keyup', onKeyUp, true);
+  /**
+   * Add a group of listeners to detect a fine-grained pointing device.


robdodson · 2017-11-15T08:01:40Z

src/focus-ring.js

+
+  /**
+   * When the polfyill first loads, assume the user is in keyboard modality.
+   * If any event is received from a fine-grained pointing device (mouse, pointer, touch),


robdodson · 2017-11-15T08:01:48Z

src/focus-ring.js

+   * If any event is received from a fine-grained pointing device (mouse, pointer, touch),
+   * turn off keyboard modality.
+   * This accounts for situations where focus enters the page from the URL bar.
+   * In that scenario, the keydown event is inconsistent, so we can't use it to detect modality.


robdodson · 2017-11-15T08:02:27Z

test/fixtures/contenteditable-textbox.html

+      https://jsbin.com/cawevo/edit?html,output
+      Add a dummy button so Selenium can use it as the first focus target.
+    -->
+    <button id="start">Dummy</button>


robdodson · 2017-11-15T08:04:12Z

test/index.js

 function browserFilter(browser) {
  return browser.getReleaseName() === 'stable'
-    && ['firefox', 'chrome'].includes(browser.getId());
+    && ['chrome', 'firefox'].includes(browser.getId());


was just double checking something. selenium-assistant stopped killing firefox 57 properly and I wasn't sure if the order in which the browsers opened/closed had anything to do with it. turns out it doesn't. the ordering doesn't matter and the issue isn't actually a show stopper. it just means if you test locally you need to close firefox when you're done. doesn't seem to bother travis though. I filed GoogleChromeLabs/selenium-assistant#105

robdodson · 2017-11-15T08:04:51Z

test/specs/contenteditable-textbox.js

-  // https://jsbin.com/cawevo/edit?html,output
-  it.skip('should apply .focus-ring on keyboard focus', function() {
-    return matchesKeyboard();
+  // FF won't focus a div with contenteditable if it's the first element on the page.


Not as much fun as https://github.com/WICG/focus-ring/pull/68/files#diff-6d1daa3464793cab0c4c5d8eeb17e778R178

robdodson added 4 commits November 14, 2017 20:25

Fixup all tests and url bar issue

f28a311

Add fixed up version of focus-ring

95d9ae9

Fix keyboard support for radio groups

6e1aae9

Fixup travis.yml

e4fe07b

robdodson changed the title ~~Revert behavior to fix failing tests. Add keyboard support. Fixes #65, #54, #63, #44~~ Revert behavior to fix failing tests. Add arrow key support. Fixes #65, #54, #63, #44 Nov 15, 2017

robdodson added 2 commits November 14, 2017 21:37

Fixup skipped tests

8fc6752

Tidy package-lock.json

d35b9b9

robdodson merged commit 989804d into master Nov 15, 2017

alice reviewed Nov 15, 2017

View reviewed changes

robdodson commented Nov 15, 2017

View reviewed changes

robdodson deleted the fix-tests branch February 7, 2018 16:26

Revert behavior to fix failing tests. Add arrow key support. Fixes #65, #54, #63, #44 #68

Revert behavior to fix failing tests. Add arrow key support. Fixes #65, #54, #63, #44 #68

Uh oh!

Conversation

robdodson commented Nov 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robdodson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants