Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue number #33 #34

Merged
merged 7 commits into from
Aug 8, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
150 changes: 58 additions & 92 deletions dist/focus-ring.js
Original file line number Diff line number Diff line change
Expand Up @@ -170,80 +170,25 @@ ClassList.prototype.toggle = function (token, force) {
return (typeof force == 'boolean' ? force : !hasToken);
};

/**
* Determine if a DOM element matches a CSS selector
*
* @param {Element} elem
* @param {String} selector
* @return {Boolean}
* @api public
*/

function matches(elem, selector) {
// Vendor-specific implementations of `Element.prototype.matches()`.
var proto = window.Element.prototype;
var nativeMatches = proto.matches ||
proto.mozMatchesSelector ||
proto.msMatchesSelector ||
proto.oMatchesSelector ||
proto.webkitMatchesSelector;

if (!elem || elem.nodeType !== 1) {
return false;
}

var parentElem = elem.parentNode;

// use native 'matches'
if (nativeMatches) {
return nativeMatches.call(elem, selector);
}

// native support for `matches` is missing and a fallback is required
var nodes = parentElem.querySelectorAll(selector);
var len = nodes.length;

for (var i = 0; i < len; i++) {
if (nodes[i] === elem) {
return true;
}
}

return false;
}

/**
* Expose `matches`
*/

var index$1 = matches;

/* https://github.com/WICG/focus-ring */
document.addEventListener('DOMContentLoaded', function() {
var hadKeyboardEvent = false;
var keyboardThrottleTimeoutID = 0;
var elWithFocusRing;

// These elements should always have a focus ring drawn, because they are
// associated with switching to a keyboard modality.
var keyboardModalityWhitelist = [
'input:not([type])',
'input[type=text]',
'input[type=search]',
'input[type=url]',
'input[type=tel]',
'input[type=email]',
'input[type=password]',
'input[type=number]',
'input[type=date]',
'input[type=month]',
'input[type=week]',
'input[type=time]',
'input[type=datetime]',
'input[type=datetime-local]',
'textarea',
'[role=textbox]',
].join(',');
var inputTypesWhitelist = {
'text': true,
'search': true,
'url': true,
'tel': true,
'email': true,
'password': true,
'number': true,
'date': true,
'month': true,
'week': true,
'time': true,
'datetime': true,
'datetime-local': true,
};

/**
* Computes whether the given element should automatically trigger the
Expand All @@ -253,7 +198,22 @@ document.addEventListener('DOMContentLoaded', function() {
* @return {boolean}
*/
function focusTriggersKeyboardModality(el) {
return index$1(el, keyboardModalityWhitelist) && index$1(el, ':not([readonly])');
var type = el.type;
var tagName = el.tagName.toLowerCase();

if (tagName == 'input' && inputTypesWhitelist[type] && !el.readonly) {
return true;
}

if (tagName == 'textarea' && !el.readonly) {
return true;
}

if (el.getAttribute('role').toLowerCase() == 'textbox') {
return true;
}

return false;
}

/**
Expand All @@ -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', '');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, why is this being removed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@kloots kloots Jun 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@alice alice Jun 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check out the change for context.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Keep a reference to the element to which the focus-ring class is applied
// so the focus-ring class can be restored to it if the window regains
// focus after being blurred.
Expand All @@ -278,42 +237,49 @@ document.addEventListener('DOMContentLoaded', function() {
* @param {Element} el
*/
function removeFocusRingClass(el) {
if (!el.hasAttribute('data-focus-ring-added'))
Copy link

@marcysutton marcysutton Jun 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return;
index(el).remove('focus-ring');
el.removeAttribute('data-focus-ring-added');
}

/**
* On `keydown`, set `hadKeyboardEvent`, to be removed 100ms later if there
* are no further keyboard events. The 100ms throttle handles cases where
* focus is redirected programmatically after a keyboard event, such as
* opening a menu or dialog.
* @param {Event} e
*/
function onKeyDown() {
hadKeyboardEvent = true;
function onKeyDown(e) {
if (e.altKey || e.ctrlKey || e.metaKey)
return;

var isTabKey = e.keyCode == 9;
var isTabNavigation = isTabKey || isTabKey && e.shiftKey;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bkardell fixed now.


if (!isTabNavigation)
return;

// `activeElement` defaults to document.body if nothing focused,
// so check the active element is actually focused.
if (index$1(document.activeElement, ':focus'))
addFocusRingClass(document.activeElement);
if (keyboardThrottleTimeoutID !== 0)
clearTimeout(keyboardThrottleTimeoutID);
keyboardThrottleTimeoutID = setTimeout(function() {
hadKeyboardEvent = false;
keyboardThrottleTimeoutID = 0;
}, 100);
var activeElement = document.activeElement;
if (activeElement.tagName == 'body') {
return;
}

addFocusRingClass(activeElement);
hadKeyboardEvent = true;
}

/**
* On `focus`, add the `focus-ring` class to the target if:
* - 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
* focus as a result of keyboard navigation.
* - the event target is an element that will likely require interaction
* via the keyboard (e.g. a text box)
* @param {Event} e
*/
function onFocus(e) {
if (hadKeyboardEvent || focusTriggersKeyboardModality(e.target))
addFocusRingClass(e.target);
hadKeyboardEvent = false;
}

/**
Expand All @@ -325,7 +291,7 @@ document.addEventListener('DOMContentLoaded', function() {
}

/**
* When the window regains focus, restore the focus-ring class to the element
* When the window regains focus, restore the focus-ring class to the element
* to which it was previously applied.
*/
function onWindowFocus() {
Expand All @@ -334,9 +300,9 @@ document.addEventListener('DOMContentLoaded', function() {
}
}

document.body.addEventListener('keydown', onKeyDown, true);
document.body.addEventListener('focus', onFocus, true);
document.body.addEventListener('blur', onBlur, true);
document.addEventListener('keydown', onKeyDown, true);
document.addEventListener('focus', onFocus, true);
document.addEventListener('blur', onBlur, true);
window.addEventListener('focus', onWindowFocus, true);
});

Expand Down
3 changes: 1 addition & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
"rollup-watch": "^3.2.2"
},
"dependencies": {
"dom-classlist": "^1.0.1",
"dom-matches": "^2.0.0"
"dom-classlist": "^1.0.1"
}
}
101 changes: 57 additions & 44 deletions src/focus-ring.js
Original file line number Diff line number Diff line change
@@ -1,32 +1,24 @@
import classList from 'dom-classlist';
import matches from 'dom-matches';

/* https://github.com/WICG/focus-ring */
document.addEventListener('DOMContentLoaded', function() {
var hadKeyboardEvent = false;
var keyboardThrottleTimeoutID = 0;
var elWithFocusRing;

// These elements should always have a focus ring drawn, because they are
// associated with switching to a keyboard modality.
var keyboardModalityWhitelist = [
'input:not([type])',
'input[type=text]',
'input[type=search]',
'input[type=url]',
'input[type=tel]',
'input[type=email]',
'input[type=password]',
'input[type=number]',
'input[type=date]',
'input[type=month]',
'input[type=week]',
'input[type=time]',
'input[type=datetime]',
'input[type=datetime-local]',
'textarea',
'[role=textbox]',
].join(',');
var inputTypesWhitelist = {
'text': true,
'search': true,
'url': true,
'tel': true,
'email': true,
'password': true,
'number': true,
'date': true,
'month': true,
'week': true,
'time': true,
'datetime': true,
'datetime-local': true,
};

/**
* Computes whether the given element should automatically trigger the
Expand All @@ -36,7 +28,22 @@ document.addEventListener('DOMContentLoaded', function() {
* @return {boolean}
*/
function focusTriggersKeyboardModality(el) {
return matches(el, keyboardModalityWhitelist) && matches(el, ':not([readonly])');
var type = el.type;
var tagName = el.tagName.toLowerCase();

if (tagName == 'input' && inputTypesWhitelist[type] && !el.readonly) {
return true;
}

if (tagName == 'textarea' && !el.readonly) {
return true;
}

if (el.getAttribute('role').toLowerCase() == 'textbox') {
return true;
}

return false;
}

/**
Expand All @@ -48,7 +55,6 @@ document.addEventListener('DOMContentLoaded', function() {
if (classList(el).contains('focus-ring'))
return;
classList(el).add('focus-ring');
el.setAttribute('data-focus-ring-added', '');
// Keep a reference to the element to which the focus-ring class is applied
// so the focus-ring class can be restored to it if the window regains
// focus after being blurred.
Expand All @@ -61,42 +67,49 @@ document.addEventListener('DOMContentLoaded', function() {
* @param {Element} el
*/
function removeFocusRingClass(el) {
if (!el.hasAttribute('data-focus-ring-added'))
return;
classList(el).remove('focus-ring');
el.removeAttribute('data-focus-ring-added');
}

/**
* On `keydown`, set `hadKeyboardEvent`, to be removed 100ms later if there
* are no further keyboard events. The 100ms throttle handles cases where

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcysutton nice eagle eye. Thank you! Fixed.

* focus is redirected programmatically after a keyboard event, such as
* opening a menu or dialog.
* @param {Event} e
*/
function onKeyDown() {
hadKeyboardEvent = true;
function onKeyDown(e) {
if (e.altKey || e.ctrlKey || e.metaKey)
return;

var isTabKey = e.keyCode == 9;
var isTabNavigation = isTabKey || isTabKey && e.shiftKey;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if (!isTabNavigation)
return;

// `activeElement` defaults to document.body if nothing focused,
// so check the active element is actually focused.
if (matches(document.activeElement, ':focus'))
addFocusRingClass(document.activeElement);
if (keyboardThrottleTimeoutID !== 0)
clearTimeout(keyboardThrottleTimeoutID);
keyboardThrottleTimeoutID = setTimeout(function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@kloots kloots Jun 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

hadKeyboardEvent = false;
keyboardThrottleTimeoutID = 0;
}, 100);
var activeElement = document.activeElement;
if (activeElement.tagName == 'body') {
return;
}

addFocusRingClass(activeElement);
hadKeyboardEvent = true;
}

/**
* On `focus`, add the `focus-ring` class to the target if:
* - 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: remove everything up to and including "meaning"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

* focus as a result of keyboard navigation.
* - the event target is an element that will likely require interaction
* via the keyboard (e.g. a text box)
* @param {Event} e
*/
function onFocus(e) {
if (hadKeyboardEvent || focusTriggersKeyboardModality(e.target))
addFocusRingClass(e.target);
hadKeyboardEvent = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

}

/**
Expand All @@ -117,8 +130,8 @@ document.addEventListener('DOMContentLoaded', function() {
}
}

document.body.addEventListener('keydown', onKeyDown, true);
document.body.addEventListener('focus', onFocus, true);
document.body.addEventListener('blur', onBlur, true);
document.addEventListener('keydown', onKeyDown, true);
document.addEventListener('focus', onFocus, true);
document.addEventListener('blur', onBlur, true);
window.addEventListener('focus', onWindowFocus, true);
});