Clean up polyfill code; add discussion of default modality to explainer. #4

alice · 2017-03-08T23:31:53Z

marcoscaceres · 2017-03-09T06:21:26Z

@alice, I might be wrong, but it seems you have not yet joined the WICG as a Google rep. Please visit the following link, and click "join this group". That should guide you and give you the right permissions. https://www.w3.org/community/wicg/

In the mean time, I'll mark this as non-substitantive.

marcoscaceres · 2017-03-09T06:21:38Z

Marked as non-substantive for IPR from ash-nazg.

robdodson · 2017-03-09T06:43:12Z

README.md

+
+1. Add a new keyword value to the outline shorthand that represents whatever the default UA `::focus-ring` is. Then authors can do:
+
+        :focus {


suggest using :focus-ring

Not what you want, though. If you always want the focus ring to be shown (the use case in question), you want :focus.

oh sorry I thought this was more about our whole outline: ua discussion. Like if someone says :focus { outline: none } then there's no way for :focus-ring { } to ever get the default UA styles back

robdodson · 2017-03-09T06:43:42Z

README.md

+2. Add a new CSS property that controls "keyboard modality" vs non-"keyboard modality" behavior, e.g.
+
+        my-custom-texty-element {
+            should-show-focus-on-mouse-click-bikeshed: on-bikeshed;


I feel like if we do this then why have :focus-ring at all? Not saying we shouldn't do this, just that it would probably supersede whatever :focus-ring has to offer

No, this complements :focus-ring by allowing you to choose when it gets matched (unlike :focus which always matches on focus regardless).

OOOOOOOOH. got it. yes that would be nice. I think the way the use case was pitched didn't click in my head... also it's 1am

You may be able to rephrase it as:

In addition to `:focus-ring`, there are additional, speculative, missing primitives which may make working with focus easier. 1. An element should have a way to restore the UA default outline. Because each UA has a different default style, there's no easy way to restore the outline if a prior selector has overridden it (short of UA sniffing). 2. An element should have a way to express when it should display a focus ring. On keyboard focus, on mouse focus, or on both. These could help explain the behavior of native text fields...

robdodson · 2017-03-09T06:45:34Z

demo/index.html

    <p>
      This page contains various html controls and demonstrates a proposal for only drawing a focus ring when the keyboard is being used.
-      Navigate through the interactive elements various ways (using mouse, switch to keyboard, try it on a touch device). You should see a green focus indicator (or comment out green focus ring style to see default focus ring) on elements below differently based on whether you interact with them via keyboard or mouse.  If you are using the mouse and get lost, at any time you can tab/shift-tab to find your focus indicator.
+      Navigate through the interactive elements various ways (using mouse, switch to keyboard, try it on a touch device).


"Navigate through the interactive elements in various ways"

Yeah we accidentally a word there.

robdodson · 2017-03-09T06:46:44Z

src/focus-ring.js

@@ -1,67 +1,112 @@
-/* https://github.com/WICG/focusring */
+/* https://github.com/WICG/focus-ring */


oops I merged my own PR that had this same change
#1

robdodson · 2017-03-09T06:48:10Z

src/focus-ring.js

 document.addEventListener('DOMContentLoaded', function() {
-    var hadKeyboardEvent = false,
-        keyboardModalityWhitelist = [ 'input:not([type])',
+    var hadKeyboardEvent = false;


Suggest using let and const throughout so we can be cool and hip. 🏂

doesn't that mean it work work in some browsers and we'll have to go through all that pain again?

yeaaaah i guess. ok can leave as var

robdodson · 2017-03-09T06:49:57Z

src/focus-ring.js

        hadKeyboardEvent = true;
+        // `activeElement` defaults to document.body if nothing focused,
+        // so check the active element is actually focused.
        if (document.activeElement.matches(':focus')) {


should this be matcher.call(document.activeElement, ':focus')

robdodson · 2017-03-09T06:51:26Z

src/focus-ring.js

-            el.removeAttribute('data-focus-ring-added')
-        };
+                                      '[role=textbox]' ].join(',');
+    var matcher = (function() {


would you mind renaming this to matches? And adding a comment linking to the MDN post https://developer.mozilla.org/en-US/docs/Web/API/Element/matches

when I think of "matcher" my brain automatically assumes that this is already bound to a specific element. I have gone back and reread this chunk of code like 5 times and I still can't shake that feeling 😋

I had that briefly but matches by itself looks too much like it should be a list of matches (results) to me. matcher conveys that it's a function, IMO.

Ok how about matchesFunction.

robdodson · 2017-03-09T07:00:01Z

src/focus-ring.js


-    document.body.addEventListener('focus', function(e) {
+    /**
+     * On `focus`, add the `focus-ring` class to the target if necessary.


replace if necessary with if the element matches a selector in the keyboardModalityWhitelist whitelist

Not accurate:

Will also add it if focus came from keyboard and the element doesn't match the whitelist

Will not add it if it was already there.

could you add all of the above to the comment? 😸

Why? They're implementation details.

I guess I see the potential scenarios as critical pieces of information.

On `focus`, add the `focus-ring` class if one of the following scenarios is met: - If focus came from the keyboard. - If focus occurred before the keyboardThrottle expired. - If the `target` is in the keyboardModalityWhitelist.

Ok you convinced me.

robdodson · 2017-03-09T07:00:36Z

src/focus-ring.js

+    var hadKeyboardEvent = false;
+    var keyboardThrottleTimeoutID = 0;
+
+    var keyboardModalityWhitelist = [ 'input:not([type])',


This could use a comment explaining why the whitelist exists. That it's for elements we want to always display a :focus-ring for

Clean up polyfill code; add discussion of default modality to explainer.

f3d10a8

robdodson requested changes Mar 9, 2017

View reviewed changes

Alice Boxhall added 5 commits March 9, 2017 18:28

Address review comments

f990148

in

b472d18

Fix comment on onFocus

b45dffc

Flesh out discussion of specultive proposals

9dd7690

s/bikeshed/foo/

e55a562

robdodson approved these changes Mar 9, 2017

View reviewed changes

robdodson merged commit 7ff067a into WICG:master Mar 9, 2017


		1. Add a new keyword value to the outline shorthand that represents whatever the default UA `::focus-ring` is. Then authors can do:

		:focus {

		@@ -1,67 +1,112 @@
		/* https://github.com/WICG/focusring */
		/* https://github.com/WICG/focus-ring */

Clean up polyfill code; add discussion of default modality to explainer. #4

Clean up polyfill code; add discussion of default modality to explainer. #4

Uh oh!

Conversation

alice commented Mar 8, 2017

Uh oh!

marcoscaceres commented Mar 9, 2017

Uh oh!

marcoscaceres commented Mar 9, 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!

robdodson Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

robdodson Mar 9, 2017 •

edited

Loading