Fix issue number #33 #34

kloots · 2017-06-05T20:21:16Z

This PR addresses issue #33 by refining the heuristics for when the focus-ring class is applied: if the user navigates to it via Tab or Shift + Tab.

In addition, this PR also:

Removes use of data-focus-ring-added attribute
Removes the need for use of dom-matches polyfill dependency
Removes the need for setTimeout() when handling the keydown event.

…o the target if the user navigates to it via Tab or Shift + Tab 2. Remove use of data-focus-ring-added 3. Remove need for use of matches polyfill 4. Remove need for setTimeout()

alice · 2017-06-05T21:10:07Z

Mostly looks good, but I'd like to get at least @bkardell @robdodson or @marcysutton to take a look before we merge, since this is a big behaviour change.

alice · 2017-06-05T20:39:26Z

src/focus-ring.js

+      return;
+
+    var isTabKey = e.keyCode == 9;
+    var isTabNavigation = isTabKey || isTabKey && e.shiftKey;


This reduces to just isTabKey, doesn't it? Perhaps a comment instead noting that the shift key may or may not be pressed?

It does indeed. Previous approach was more out of the desire to have the code be explicit & self-documenting about when we match. But if you don't like it, then consider it changed.

alice · 2017-06-05T20:39:42Z

src/focus-ring.js

-      addFocusRingClass(document.activeElement);
-    if (keyboardThrottleTimeoutID !== 0)
-      clearTimeout(keyboardThrottleTimeoutID);
-    keyboardThrottleTimeoutID = setTimeout(function() {


Hmmm how do you feel about the case where, say, using Enter to activate a button causes a UI change? Should :focus-ring match then?

Isn't adding a focus ring superfluous in that case as the UI change alone is an indicator?

I think it depends what ends up being focused. If it's a widget you might immediately want to interact with, I think it might be disorienting to hide the focus ring. Using enter to activate a button is a stronger signal than using a keyboard shortcut.

I'm not entirely clear on the use case you are advocating for. Would you mind explaining in more detail? Perhaps with a real-life example.

I don't have a real life example, unfortunately, but I'm imagining something like a custom date picker which pops up some extra UI when you press a button.

Another thought: this also means :focus-ring wouldn't match for UI which supports directional navigation.

Isn't directional navigation in the same category as a screen reader's virtual navigation, and therefore the visual indicator is provided by the host service (e.g. like VoiceOver's black outline).

I mean author-implemented directional navigation as opposed to "spatial navigation" as a UA feature using some kind of D-pad.

The example I just saw was in Google Slides, where you go to add a new slide and you get a (in my case at least) 3x4 grid of slide layout options that you can choose between.

Not all keyboard users are also using AT, if you had a menu of menu items for example you want them to show the ring, right?

@bkardell wouldn't a focus ring be superfluous for menu items? Consider the menubar in Mac OS—you don't get both the selection highlight + a focus ring when navigating via the up & down arrow keys, you simply see the selection state change (blue background).

I don't know how we could account for all author-defined directional navigation while at the same time not matching too aggressively (as we are now). Hence my comment in issue #33 indicating we should be more conservative with this polyfill and allow authors to decide when to show the focus ring when they are implementing author-defined keyboard shortcuts or navigation.

For example, take your menu use case. Any menu I have ever implemented with keyboard accessibility has code that looks like this:

function onKeyDown(e) { if (e.keyCode == 40) { $(e.target).next().focus(); $(e.target).next().addClass('selected'); } }

In such cases I'm already explicitly managing both where focus should move and what it should look like via adding a class—so I can match the selection style also used by the mouse. So, having a focus-ring class also applied in this case wouldn't be desirable to me. In fact, it would be duplicating efforts.

alice · 2017-06-05T20:40:22Z

src/focus-ring.js

   * @param {Event} e
   */
  function onFocus(e) {
    if (hadKeyboardEvent || focusTriggersKeyboardModality(e.target))
      addFocusRingClass(e.target);
+      hadKeyboardEvent = false;


Is this weirdly indented or is it meant to be part of the if block?

always use {} and save yourself the hassle is my motto :)

@bkardell agreed. All {} all the time. Done. Only didn't previously as previously the code didn't use them and I thought that may have been the preferred style by @alice or @robdodson.

For single lines I'm in the habit of avoiding them since that's the style we use in Chromium. But you have to be used to that in order to be used to adding them for additional lines (since not adding them can create bugs along the lines of GOTOFAIL)

alice · 2017-06-05T20:40:53Z

src/focus-ring.js

-   * - a keyboard event happened in the past 100ms, or
-   * - the focus event target triggers "keyboard modality" and should always
-   *   have a focus ring drawn.
+   * - a keydown event preceded the focus event, meaning the target received


suggestion: remove everything up to and including "meaning"

… ifs.

alice · 2017-06-05T21:52:00Z

dist/focus-ring.js

@@ -265,7 +225,6 @@ document.addEventListener('DOMContentLoaded', function() {
    if (index(el).contains('focus-ring'))
      return;
    index(el).add('focus-ring');
-    el.setAttribute('data-focus-ring-added', '');


Oh, why is this being removed here?

Because I couldn't work out why it was necessary in the first place. Isn't it superfluous? Doesn't the presence of the focus-ring class give us enough?

It disambiguates an explicit, author-added focus-ring class (to allow always matching .focus-ring) from a programmatically added one. See line 281 for where it's used. It was added in 8d00580

What's a use case for an author-added focus-ring class? And why would we want to support that if this is a polyfill for :focus-ring and there would be no similar pattern once this hopefully is implemented as a pseudo selector in browsers. Wouldn't we want to discourage authors adding focus-ring?

Check out the change for context.

Can you explain how this is more 'with-the-grain' of the eventual pseudo? I'm not following.

In any case, this is orthogonal to the main point of this PR, so maybe let's revert it here and continue the discussion on a bug?

@bkardell I don't think this polyfill should give authors abilities they won't have with the actual :focus-ring psuedo. Therefore, once actual support for :focus-ring lands in browsers the only change authors need to make to their CSS is changing their rules from .focus-ring to :focus-ring. Hence, more with the grain.

Can we file a bug for this discussion and revert this change in the context of this PR?

@alice bug is filed. And this PR has been updated to restore data-focus-ring-added.

alice · 2017-06-05T22:08:40Z

src/focus-ring.js

@@ -52,8 +52,10 @@ document.addEventListener('DOMContentLoaded', function() {
   * @param {Element} el
   */
  function addFocusRingClass(el) {
-    if (classList(el).contains('focus-ring'))
+    if (classList(el).contains('focus-ring')) {


Can we please revert these?

...and done.

marcysutton · 2017-06-06T17:09:06Z

dist/focus-ring.js

@@ -278,42 +237,49 @@ document.addEventListener('DOMContentLoaded', function() {
   * @param {Element} el
   */
  function removeFocusRingClass(el) {
-    if (!el.hasAttribute('data-focus-ring-added'))


I see your commit to add these back in, but for some reason the diff is showing these are still removed.

Edit: Ah, I see it's on the dist version of the file. I wonder if git should exclude the dist files and generate them with build scripts instead? Makes reviewing easier.

@marcysutton Sorry about that! I forgot to re-build the dist. Fixed now.

marcysutton · 2017-06-06T17:16:03Z

src/focus-ring.js

@@ -72,31 +76,37 @@ document.addEventListener('DOMContentLoaded', function() {
   * are no further keyboard events.  The 100ms throttle handles cases where


If throttling is going away this comment should probably be updated, too.

@marcysutton nice eagle eye. Thank you! Fixed.

bkardell · 2017-06-06T17:22:20Z

dist/focus-ring.js

+      return;
+
+    var isTabKey = e.keyCode == 9;
+    var isTabNavigation = isTabKey || isTabKey && e.shiftKey;


this seems a bit of a weird and'ing and or'ing?

@bkardell it is right in the source code, I just forgot to re-build the dist. Will do.

@bkardell fixed now.

marcysutton · 2017-06-06T18:17:03Z

Initially I wasn't sure if suppressing focus on keyboard shortcuts was a good idea, but you can still override the focus style using outline-width...so this works. Here's a demo. https://codepen.io/marcysutton/pen/ryqKmd

kloots · 2017-06-07T00:04:29Z

@alice wanted to check in on this PR and if you've got any outstanding concerns that need to be addressed. @marcysutton and @bkardell have reviewed and I've addressed their issues. (@marcysutton, @bkardell either of you have any additional comments, or are you good?) I'm still waiting on a review from @robdodson.

alice · 2017-06-07T00:15:28Z

Apologies, I've been in meetings all day. Yeah, would like to get @robdodson's 2c but he's out till next week.

alice · 2017-06-07T00:17:52Z

@marcysutton

Initially I wasn't sure if suppressing focus on keyboard shortcuts was a good idea, but you can still override the focus style using outline-width...so this works.

Can you expand on how this plays into the context of whether focus has come from tab/shift tab or not?

kloots · 2017-06-07T00:48:52Z

@alice I think @marcysutton's comment with respect to "suppressing focus on keyboard shortcuts" was related to the thread you, @bkardell and I had going yesterday with respect to author-implemented directional navigation.

marcysutton · 2017-06-07T03:16:13Z

Yes, it was. With this version of the polyfill, tab/shift+tab worked as I expected and the keyboard shortcut case was still achievable with :focus.

alice · 2017-06-07T05:08:40Z

Sure, the part I don't follow is how the outline style applies in the keyboard shortcut case - do you mean you would listen for the keyboard shortcut and then apply that style?

marcysutton · 2017-06-07T05:20:41Z

Here's a demo–I got a button to show a focus style after manually sending focus there via keyboard shortcut by using a combination of :focus and :focus-ring. I'm not sure if that's the use case you were talking about, but the change in this PR did suppress the focus style for that navigation pattern before I added a :focus override.

alice · 2017-06-07T05:23:55Z

Right - that button also gets a focus ring if you just click it. So the part I'm wondering about is how we handle the case where we do want the button to get a focus ring on keyboard shortcuts, without reverting to simply :focus-style behaviour where it also gets a focus ring on click.

So perhaps it's just as simple as applying an extra class to the button after a keyboard shortcut is fired, to cause :focus to match?

marcysutton · 2017-06-07T17:45:53Z

Oh yeah, duh! This doesn't actually work then. And it highlights another issue–what if I want to show the focus style for focus management? I use that pattern often to show keyboard users where they are on the screen after sending focus to a new area. That style is suppressed with this change.

kloots · 2017-06-07T18:23:43Z

@alice @marcysutton I have to admit we're so far into this thread I've lost the original context with respect to:

The problem as perceived by @marcysutton
If the perceived problem is new/different from one we've already hashed out in previous comments for this PR or in issue When to match :focus-ring:? #33.

So, my apologies for not following along with you both.

If @marcysutton's concern is how to indicate focus when the user switches contexts (e.g. into a dialog, into a menu, or a side panel), I was under the impression @alice had already addressed that in her summary for why the recommendation to UAs is :focus-ring should only match on Tab or Shift + Tab:

"If a keyboard shortcut is likely to move focus, it is probably either showing some new UI, or it is specifically designed to assist keyboard users, in which case a manual focus style is likely to be applied."

@alice's comment is inline with my experience with any webapp I've ever worked on; the code managing a context switch—especially when moving to a dialog or menu—is also explicitly setting focus programmatically to the next control representing the next action the user is likely to take, and also applying some class to draw attention to said control.

In the case of a dialog this is most often either:

The button confirming an action (e.g. Highlighting "Ok" in a confirmation dialog)
Moving focus into the next most logical text box in a form in a dialog so the user can start typing

In either of these use case, authors can leverage the cascade to ensure the focus ring is visible in response to programatic focus following a context switch:

/* Hide focus ring by default */
:focus {
  outline: none
}

/* Show a focus outline when navigating via Tab */
:focus-ring {
  outline: 3px solid blue;
}

/* Ensure the default action button in dialogs has a focus outline */
.default-action-button:focus {
  background: green;
  outline: 3px solid blue;
}

In the case of a popup menu I've never had a need to render a focus outline as the selection change made via keyboard navigation is always intended to render to match the selection state on mouseover.

Anyway, sorry for the long message. I hope this resolves this issue (presuming this is the issue you are actually talking about).

marcysutton · 2017-06-07T19:25:07Z

The reason I bring up the focus management use case is that it's impacted by this change. Sadly, I wouldn't use :focus-ring if it didn't provide adequate styling hooks for my use cases–I can continue to use What Input for that.

The problem I hoped :focus-ring would solve is that custom elements and focus management wrappers with tabindex should persist focus styles only after keyboard interaction (including "indirectly", per the spec). I encountered that same gotcha in my demo in this thread when I tried to override the behavior in this PR with a :focus style on a custom element. In my opinion, that puts us right back at the core of the problem.

For example, this code persists the focus style on click if the element is a custom control with tabindex="0" (now included in my demo):

// <div class="default-action-button" role="button" tabindex="0">
.default-action-button:focus {
  background: green;
  outline: 3px solid blue;
}

Custom controls aren't ideal, I know, but when they happen in the real world and persist on click, that's when focus styles get disabled entirely.

Whether you move focus to a wrapper element with tabindex="-1", or a native button element, or even a custom control, is widely dependent on implementation. My hope is that :focus-ring would give us better control over additional scenarios beyond native buttons with custom CSS styles. But I think we're having difficulty reconciling all of these use cases at the same time, and the big question is whether we can reliably suppress focus for Slack's use case while providing style hooks for custom controls and focus management targets.

Here are the various use cases I would hope this polyfill could cover:

Custom styles on natively focusable elements like button:focus that have historically persisted on click and triggered bug reports.
Custom controls using ARIA and tabindex="0", both directly and indirectly focused, which persist both the default focus ring and custom CSS :focus styles on click.
Focus management targets using tabindex="-1". (Should they not show focus at all? This is subjective)

This isn't an easy one to solve, so my apologies if this feels like piling on. But it was important for me to go back and remember my motivations for a tool like :focus-ring in the first place. Hopefully these details help bring that all into...focus. (I'll show myself out. :))

kloots · 2017-06-07T20:11:40Z

@marcysutton the custom control issue you call out is a legit problem, but that's just a bug and (IMO) orthogonal to the issue of the heuristics for the keyboard behavior we should match on. I'll open an issue and PR to address that... one moment.

kloots · 2017-06-07T20:23:41Z

@marcysutton actually, looking at the code and testing the demo we're already supporting custom controls save those with a tabindex set -1—which seems correct to me.

marcysutton · 2017-06-07T20:25:35Z

looking at the code and testing the demo we're already supporting custom controls save those with a tabindex set -1

@kloots how? When I click on a custom element with tabindex="0", the focus ring shows. I want to make sure I didn't misunderstand the usage API.

kloots · 2017-06-07T20:28:32Z

@marcysutton ohhhh that! Yes, that is a bug. In fact, I believe I have fixed that in a separate PR.

kloots · 2017-06-07T20:34:47Z

@marcysutton I take it back, I cannot repro your bug in the demo on master or on this branch. Try clicking the <div> with a tabindex of 0 via the mouse. I don't see a focus ring except if I navigate to the element via the keyboard. Unless I still don't understand what you're talking about.

…listener such that it no longer calls addFocusRingClass()

alice · 2017-08-04T04:58:06Z

👍

So sorry for letting this go cold!

robdodson · 2017-08-04T04:59:38Z

@kloots do you think this is ready to merge? If so let's 🚀

kloots · 2017-08-04T20:01:57Z

@robdodson It's ready to merge now. Just added one more commit from my fork which prevents handling focus and blur when the bubble to the document — which can caused errors in Firefox.

robdodson · 2017-08-06T16:51:49Z

Just started reviewing :)

Sent from GH.

robdodson · 2017-08-06T17:58:00Z

I wanted to double check one last thing based on @marcysutton's comments.

In this example snippet you provided:

/* Hide focus ring by default */
:focus {
  outline: none
}

/* Show a focus outline when navigating via Tab */
:focus-ring {
  outline: 3px solid blue;
}

/* Ensure the default action button in dialogs has a focus outline */
.default-action-button:focus {
  background: green;
  outline: 3px solid blue;
}

The .default-action-button will always display a focus indicator of some kind. Including if it is clicked on with a mouse. Was that your intention? I think what you were trying to show was how to have it display a focus indicator in response to programmatic focus.

kloots · 2017-08-07T15:38:01Z

@robdodson yes, expected. In that instance the context (if I remember) was a focus style for a button that mapped to the default action for a dialog, and as such, the focus style should always be visible regardless of how the button received focus: programmatic (on initial display of the dialog) or via user interaction (via keyboard or mouse).

robdodson · 2017-08-07T16:21:58Z

@kloots ok. Just curious, do you have an example of using this behavior in Slack? I just wanted to see it in action if possible.

kloots · 2017-08-07T22:08:42Z

@robdodson no example of that behavior in Slack (at least I don't think). But I think this specific point is outside the scope of this PR and is the reason why @alice started a separate discussion for this in issue #42, no? Or is this blocking the merging of this PR?

robdodson · 2017-08-08T01:45:40Z

I think it's blocking me because I'm starting to wonder if a one-size-fits-all :focus-ring style, by itself, is workable. Seeing Slack and Twitter fork it so they can add specific behavior is really interesting to me and makes me wonder if we need to add in the power of #42 at the same time. Otherwise it seems like someone will always be unhappy. As @marcysutton stated, I think she prefers the way it currently works and if we made this change she would switch back to WhatInput.

alice · 2017-08-08T03:54:33Z

I say let's go ahead and merge (and apologies again for how long that took) - but definitely stay open to feedback from people who feel this change breaks things.

1. Refine heuristics such that the focus-ring class is only applied t…

577c00a

…o the target if the user navigates to it via Tab or Shift + Tab 2. Remove use of data-focus-ring-added 3. Remove need for use of matches polyfill 4. Remove need for setTimeout()

This was referenced Jun 5, 2017

When to match :focus-ring:? #33

Closed

Add a focusring custom event #30

Closed

alice reviewed Jun 5, 2017

View reviewed changes

Remove checking Shift for Tab keypresses. Always use curly braces for…

ab6d94b

… ifs.

alice reviewed Jun 5, 2017

View reviewed changes

kloots added 2 commits June 5, 2017 15:16

Remove curly braces for single-line conditionals

a24ac32

Restore use of the data-focus-ring-added attribute

5cf8e65

kloots mentioned this pull request Jun 6, 2017

Remove support for author-added focus-ring class #35

Closed

jonathantneal mentioned this pull request Jun 6, 2017

white list #7

Closed

marcysutton reviewed Jun 6, 2017

View reviewed changes

bkardell reviewed Jun 6, 2017

View reviewed changes

Cleanup comments. Remove use of toLowerCase() when checking tagName

eb8db57

Resolve merge conflicts with upstream master. Simplify keydown event …

1eccb9c

…listener such that it no longer calls addFocusRingClass()

Don't handle focus and blur events that bubble to the document.

5f17842

alice merged commit ced1148 into WICG:master Aug 8, 2017

robdodson mentioned this pull request Aug 8, 2017

No focus styling when navigating with keyboard from outside the web content #15

Closed

majornista mentioned this pull request Oct 10, 2017

Focus ring does not account for radio buttons #54

Closed

		@@ -72,31 +76,37 @@ document.addEventListener('DOMContentLoaded', function() {
		* are no further keyboard events. The 100ms throttle handles cases where

Fix issue number #33 #34

Fix issue number #33 #34

Conversation

kloots commented Jun 5, 2017

alice commented Jun 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kloots Jun 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kloots Jun 5, 2017 • edited

Choose a reason for hiding this comment

alice Jun 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcysutton Jun 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcysutton commented Jun 6, 2017

kloots commented Jun 7, 2017

alice commented Jun 7, 2017

alice commented Jun 7, 2017

kloots commented Jun 7, 2017

marcysutton commented Jun 7, 2017

alice commented Jun 7, 2017

marcysutton commented Jun 7, 2017

alice commented Jun 7, 2017 • edited

marcysutton commented Jun 7, 2017

kloots commented Jun 7, 2017

marcysutton commented Jun 7, 2017 • edited

kloots commented Jun 7, 2017

kloots commented Jun 7, 2017

marcysutton commented Jun 7, 2017 • edited

kloots commented Jun 7, 2017

kloots commented Jun 7, 2017

alice commented Aug 4, 2017

robdodson commented Aug 4, 2017

kloots commented Aug 4, 2017

robdodson commented Aug 6, 2017

robdodson commented Aug 6, 2017

kloots commented Aug 7, 2017

robdodson commented Aug 7, 2017

kloots commented Aug 7, 2017

robdodson commented Aug 8, 2017

alice commented Aug 8, 2017

kloots Jun 5, 2017 •

edited

kloots Jun 5, 2017 •

edited

alice Jun 5, 2017 •

edited

marcysutton Jun 6, 2017 •

edited

alice commented Jun 7, 2017 •

edited

marcysutton commented Jun 7, 2017 •

edited

marcysutton commented Jun 7, 2017 •

edited