Issue19 a11y #21

burge-is · 2017-07-09T17:29:47Z

Issue #19

Adds a component to the demo code to handle keyboard accessibility.

The component:

wraps its children in a tabbable div
removes child tabIndexes
handles entering and leaving the child components
shows and hides a role='alert' tooltip when focused to announce new keyboard nav shortcuts (enter/esc)

-makes it tabbable -providing it shortcut keys to enter and exit the wrapped child component.

…sing a screenreader.

burge-is · 2017-07-09T17:30:55Z

@philpl @kentcdodds here's that PR

kentcdodds · 2017-07-10T04:56:17Z

I played with the demo and the interaction is great! One thing is it's unclear when I'm focused on the tabgate, so it's hard to know where the keyboard focus is. Normally the browser will give an outline to the focused element. Could we make sure there's some styling for :focus? Or do we need to do that ourselves as consumers?

burge-is · 2017-07-10T05:42:58Z

Huh. Thought I saw the outline in mine. I'll explicitly add that style so there's zero question.

burge-is · 2017-07-10T19:02:10Z

@kentcdodds The code is now adding a more explicitly styled outline onFocus and removing it onBlur

kentcdodds · 2017-07-10T19:15:27Z

Cool. Here's what it looks like for me:

So by adding the margin and padding on focus it's making things kinda jump around a little bit. I'd also rather this be done with regular CSS rather than inline styles and JavaScript events.

In addition, the blue outline is sub-optimal IMO. I wonder if there's another way we could highlight that it's focused...

Ok, I did a little bit of digging and just realized two things:

The TabGate component is only implemented in the demo. Haha, I thought this would be part of react-live's own implementation. Not a huge deal for me because I can copy/paste just fine, but I think this would be useful to other people
The reason the normal outline wasn't being applied is because StyledProvider had overflow: hidden which was hiding the outline. Kinda weird, but if you remove that then you don't need the custom logic with onFocus and onBlur and you don't need to apply your own outline. Win win win!

From here, I can copy the TabGate for my own use, or we can figure out a way to make react-live work out of the box. That's up to @philpl 😄

kentcdodds · 2017-07-10T19:17:39Z

Oh, and if you make the changes I suggest (remove outline: hidden), it'll look like this:

Much better 😄

burge-is · 2017-07-10T20:09:41Z

Awwww yaaa! I'm a huge fan of not doing the javascript-y css-y outline I added. Happy to see it cleared up. Deleting code is always more satisfying than adding it in my book.

I didn't look too much at the styled components, great find.

It is satisfying to see in action, the gifs are such a nice way to show off page interaction.

kitten · 2017-07-10T21:20:32Z

outline seems like a great addition and looks great :) The only problem I currently have with this is that it's only part of the example, while it's good default behaviour that the editor should include.

We could just include the TabGate in the editor but I was thinking (see my comments in the original issue) why can't we implement this just using preventDefault code that's part of the editor?

I mean we could include the TabGate code in the editor and call it a day, but why do we need this: findFirstFocusableChild and other code here?

@alexlande has made a modification a while ago where the editor's tab-capturing behaviour can be disabled:

react-live/src/components/Editor/index.js

Line 105 in 5677482

if (evt.keyCode === 9 && !this.props.ignoreTabKey) { // Tab Key

We can just add an enter handler there in an onFocus when ignoreTabKey is set and then change a flag in the state. When the flag is set we add the outline. Also when the flag is set and the tab key is pressed it is captured. When the enter key wasn't pressed we remove the preventDefault and have the ignore behaviour that the prop enables right now. This would then make tab unfocus (blur) the editor.

I think that's a much more elegant solution and we're really close to it already :)

Thanks for your effort already :) Looking great!

kentcdodds · 2017-07-10T21:22:09Z

Yup, I'm definitely more interested/excited about react-live having this baked in. I don't care as much how it's implemented, but what @philpl says makes sense 👍

burge-is · 2017-07-11T01:38:34Z

@philpl A lot of the design decisions were made with desire of avoiding touching existing code and behaviors. For this reason, code like the findFocusableChild exist purely to be a generic, reusable means to an end. I understand your preference and am currently investigating adding the code directly to the Editor without the overhead.

One thing to watch for onFocus is triggered by both mouse and keyboard. It seems wrong to make the mouse behavior now have an additional step to editing the code. Some eventListeners looking for mouse clicks should do the trick there, though. They could implement the same code that is used for hitting the Enter key.

Keyboard: tab to Editor -> hit 'Enter' -> trigger whatever happens on activating the editor for change
Mouse: click Editor -> trigger whatever happens on activating the editor for change

Both would reset whatever flag onBlur

Also because the editor is the active element we need to address what happens when the user types keys after focus. I assume this flag we'd store in state seems like it would drive two different branches of interactions when keys are pressed. One for the 'focused and waiting to enter' state and the other for the 'focused and editing' state.

Instead of introducing a flag would it make sense to use the contentEditable flag for a lot of this behavior? Flipping it on and off accordingly.

Edge cases are the trouble makers in these kind of scenarios.

kitten · 2017-07-11T02:13:11Z

I understand your preference and am currently investigating adding the code directly to the Editor without the overhead.

Cheers! 🎉 😄

One thing to watch for onFocus is triggered by both mouse and keyboard. It seems wrong to make the mouse behavior now have an additional step to editing the code.

So we'd have to make sure that onClick skips the enter guard, i.e sets it to true immediately. Makes sense 👍

Also because the editor is the active element we need to address what happens when the user types keys after focus

It should probably skip the enter guard as well. I'd assume that if the user starts typing, that he's intentionally typing to edit the code.

Instead of introducing a flag would it make sense to use the contentEditable flag for a lot of this behavior? Flipping it on and off accordingly.

Well, then the behaviour before is tricky. If we manage to reproduce all of this with just the preventDefault for tab at the right time, that'd be the cleanest approach imho. I might be wrong of course ^^

burge-is · 2017-07-11T15:47:18Z

@philpl moved the logic over to where you directed, does this look more like what you were looking for?

For styling of the outline I introduced some css and added the following logic:

onFocus adds class of tabGuarded
onBlur or once editing begin removes class of tabGuarded
The demo won't reflect this unless it uses this updated css.

For navigation:

Tabbing moves between elements until editing beings.
Editing starts on the press of any key while focused on the editor. (per your great suggestion in the comment above this one)
This 'editing state' is ended onBlur or if the 'Esc' key is pressed.
The 'editing state' is handled by an internal flag named guarded, it is in charge of handling tab capture and triggering whether or not the outline should appear.

All in all there is now an onFocus, onBlur, Escape key handler, an internal flag called guarded, and special logic at the beginning of the onKeyDown to control toggling back and forth between guarded and not

kentcdodds · 2017-07-11T15:52:28Z

Could you provide a gif? I use LICECap 😄

kentcdodds · 2017-07-11T15:53:43Z

react-live.css

@@ -92,3 +92,9 @@
 .token.deleted {
  color: red;
 }
+
+.tabGuarded{


I don't think that this CSS is needed. The user agent styles should be good enough right? If it's not showing up, perhaps it's that overflow: hidden issue I mentioned earlier...

The problem is that now we are directly focusing the editor so it will have an outline the entire duration of the editing.... or that's what I was seeing. I'll mess with the overflow you mentioned.

https://github.com/burge-is/react-live/blob/24be04a17906ff8641d1df29cc0f4c00dce24717/react-live.css#L13
There is an outline: hidden being applied to the .prism-code that wasn't in play with the old TabGate code. The TabGate had a wrapper and that is where we were outlining before, now it's directly on the Editor with it's existing css. I say this having spent more time typing then messing with the CSS.

Looked into it further and deleting the overflow: hidden didn't work for me. It's the change in implementation of previously having a wrapper but now dealing directly with the Editor. Could be wrong though, but it's what I am seeing.

Interesting. Thanks for digging. Let's see what @philpl has to say 😄

The outline code is fine. The user should be able to deal with the details of how it should look :)

But can we put it at the top of the file, rename it to .tab-guarded, and make the rule more specific please (i.e. .prism-code.tab-guarded)? Cheers :)

Btw it'll also need to be added here: https://raw.githubusercontent.com/FormidableLabs/react-live/master/src/constants/css.js

Sorry for that not being in one file right now :(

kentcdodds · 2017-07-11T15:54:08Z

src/constants/css.js

@@ -93,4 +93,10 @@ export default `
 .token.deleted {
  color: red;
 }
+
+.tabGuarded{


Some comment here

burge-is · 2017-07-11T15:59:12Z

kentcdodds · 2017-07-11T16:05:26Z

Nice! Thanks for sharing that! Yeah, I think that there's the overflow: hidden issue which is why you had to do the extra CSS stuff. If you remove that in the demo code you shouldn't have to do the extra CSS stuff for .tabGate 😄

burge-is · 2017-07-11T16:09:26Z

I kind of like the addition of the class for the purposes of styling or possible adding in the ARIA tips that were talked about in the issue thread. Provides some context by way of class.

But if removing that fixes it all again, it's another one of those win wins you mentioned and I can delete some lines of code. Checking now.

Also, thanks for the software tip. I really like this. Now to find what I see some streamers using to display the keys they are pressing on screen.

kentcdodds · 2017-07-11T16:19:19Z

You might be interested in this 😄

kitten

just some minor code comments that we need to fix but it's overall looking good

kitten · 2017-07-11T21:26:19Z

react-live.css

@@ -92,3 +92,9 @@
 .token.deleted {
  color: red;
 }
+
+.tabGuarded{
+    outline: 1px dotted #212121;


Can we instead make this solid? I don't think dotted looks rather native.

kitten · 2017-07-11T21:28:07Z

src/components/Editor/index.js

-      document.execCommand('insertHTML', false, '&#009')
-      evt.preventDefault()
-    } else if (evt.keyCode === 13) { // Enter Key
+      if(!this.tabGuarded){


You can just wrap this into the above if statement.

Also we can remove the ignoreTabKey prop now, as the new behaviour is supposed to replace it.

kitten · 2017-07-11T21:28:54Z

src/components/Editor/index.js

+      }
+    }else if (evt.keyCode && evt.keyCode === 27) {//Esc Key
+      this.tabGuarded=true
+      this.ref.classList.add("tabGuarded")


Can we just replace the ref usage and the tabGuarded flag with a flag inside the component state? In this case we don't need to be clever about how to handle the element, and can just use the lifecycle instead :)

kitten · 2017-07-11T21:29:32Z

src/components/Editor/index.js

+        evt.preventDefault()
+      }
+    }else if (evt.keyCode && evt.keyCode === 27) {//Esc Key
+      this.tabGuarded=true


There's some stylistic divergence here (= instead of =). Can you fix this please? I promise I'll add prettier to this project soon ;)

Also when you move the flag over to the state you might want to add a check for !tabGuarded here

kitten · 2017-07-11T21:35:26Z

src/components/Editor/index.js

+  onBlur = evt => {
+    this.tabGuarded =true
+    this.ref.classList.remove("tabGuarded")
+    return evt


This and the above doesn't need to return anything ;)

But we need to proxy both callbacks to potential props like the existing ones do:

if (this.props.onKeyDown) { this.props.onKeyDown(evt) }

burge-is · 2017-07-12T11:31:02Z

@philpl here's most of the suggested changes.

When I originally added the flag I put it in the state. However, this got tricky during onBlur because the render triggered by the state change caused the element we were tabbing away from to regain focus.

I considered adding code for when should the state actually render / managing focus upon render but it felt more mentally taxing to consume than the idea of dealing with classList / a boolean flag on the object.

Toying with it again this A.M. a bit to see what it would look like if I can get it to behave within the lifecycle.

burge-is · 2017-07-12T17:39:17Z

Moved tabGuarded into state and got around that tab issue I was seeing.

kitten · 2017-07-12T18:21:15Z

@burge-is amazing! I'll review it again once I'm at my desk and merge it. Don't worry about the difference in coding style. I'll just add prettier later 👍 thanks again!

kentcdodds · 2017-07-12T19:12:57Z

Thanks so much for working on this @burge-is! This is going to be great for keyboard-only users. a11y FTW!

burge-is · 2017-07-12T20:15:27Z

Thanks for tweeting this issue out @kentcdodds , my first go at contributing to open source and I learned a lot from you and @philpl during the process. Keep up the good work on y'alls end.

kitten · 2017-07-12T21:09:42Z

src/components/Editor/index.js

+    }else if (evt.keyCode === 27) {// Esc Key
+      this.setState({tabGuarded:true})
+    }else if(!(evt.shiftKey || evt.ctrlKey || evt.altKey)){
+      this.setState({tabGuarded:false})


Hm, okay. I see what prompted you to use the flag before. This state change triggers early so that the selection save (this.selection = ...) in the keyup handler isn't respected on enter.

So now when the enter key is pressed the state changes, the component updates, and the selection is harmed.

We can try an easy fix. First of all we can add the flags to the if statement && this.state.tabGuarded) { etc

Then we can maybe move this logic into the keyup handler. It has to be after the rest of the logic there. ^^

Makes sense?

kitten · 2017-07-12T21:10:35Z

Almost there now! Sorry, I found a bug with the enter key not respecting the saved selection 😢

…ot interfere with all preceding key handling

burge-is · 2017-07-12T21:49:33Z

Pesky render. The logic for changing state is now at the end of keyUp to prevent accidentally skipping logic. Good find.

kitten

Found another small bug

kitten · 2017-07-12T22:23:46Z

src/components/Editor/index.js

+
+    if (evt.keyCode === 27 && !this.state.tabGuarded) {// Esc Key
+      this.setState({tabGuarded:true})
+    }else if(!(evt.shiftKey || evt.keyCode === 27 || evt.keyCode === 9) && this.state.tabGuarded){


In keyup the shift key (19) triggers separately when released. So here you'll need to add evt.keyCode === 16 as well. Just discovered that :/

Also, can we do this.state.tabGuarded && !evt.shiftKey && evt.keyCode !== 27 /* ... */ instead? i.e. negate the group there to remove it, and put the tabGuarded first? Should help readability a little.

Definitely, I'll be a little more thorough with the key clicking this go round after the change too.

✨ Accessibility ✨ — it's tough

Is the behavior your'e seeing a shift press is being treated as hitting any old key (the assume desire to edit on any key press logic)? That would be expected based on how I was thinking of it, the evt.shiftKey was originally put in to handle shift tabbing not necessarily an independent shift press.

Which I'm thinking shift+tab may be broken in the newest implementation not gonna lie I expect this out of event handlers. They have that wonderful ability to get tangled up like earbuds in a pocket if you aren't careful with your approach. Kind of why I originally wrapped the whole thing in a blocking div. The past trauma of dealing with edge cases was giving me flashbacks. I'll dig a little bit further later this evening.

@burge-is Yep, exactly, shift+tab is broken

I just realised that we're being a bit silly here haha 😆 We can actually just set tabGuarded to false inside an onChange handler. So we create a new callback method, proxy it up and:

onChange = evt => { if (this.props.onChange) { this.props.onChange(evt) } if (this.state.tabGuarded) { this.setState({ tabGuarded:false }) } }

That should work, right?

Sooooo much prettier. That's a much better way to conceptualize it. I couldn't see the forest for the trees. It reads more like English and that always feels nice. Gets us out of that keycode "magic number" hell 🔥👿🔥

Abstractions are a developer's best friend.

… attempts to change the contents of Editor

burge-is · 2017-07-13T03:28:47Z

Not quite there, I wrote it inline and tested with good results and was hasty with commiting after placing the function on the component up with the other on(X) handlers. I am seeing different behavior, back to the lab. But that is certainly the right approach.

kitten · 2017-07-13T11:26:16Z

@burge-is I think I'm not seeing any bugs anymore so it should be good to go? The only thing is, that I'm not seeing the highlighting / outline in the demo. I thought this must've worked sometime before, given that Kent posted a gif with it above?

… a focus causing a temporary outline that is immediately removed. tabGuarded is now reset onBlur instead of having any handlers in onFocus onClick logic moved to bottom of the handelr as to not interfere with seleciton logic

burge-is · 2017-07-13T19:02:29Z

Hmmm... my onChange function isn't being entered. Are you not seeing that same behavior?

As far as the outline you need to make sure wherever you are testing is getting the latest css, when checking in demo I had to go manually replace that minified css file. I just switched over to trying it out on the storybook code so that I could more rapidly check my changes and I am seeing the outline on focus.

I corrected the margin hoppy-ness while looking it over. As well as shifted logic away from onFocus into onBlur so there wasn't a need to worry about mouse vs keyboard focus (mouse was showing the outline for a second and then removing it)

But the onChange is what has me hesitant to say it's ready for merge. You mentioned not seeing any problems but my latest only removes the tabGuard onClick and not on any change like we were aiming for, from what I've seen.

…b or escape you are editing.

burge-is · 2017-07-17T02:23:29Z

I made the last check in keyUp see if the event were anything other than Esc or Tab while tabGuarded. This triggers in the fashion we were expecting, I saw no response when using onChange

So, funny to see my original over-engineered wrapper next to these small changes. Coding is growing .... or in this case shrinking.

kitten · 2017-07-17T16:24:46Z

@burge-is I'll review it this evening 😄 looking really solid now!

Yea, oftentimes the optimal solution ends up being simpler than any initial approach.... If one's lucky

burge-is · 2017-07-17T23:05:49Z

@philpl Closing so that the commit history added to react-live isn't riddled with my subtle changes.
New PR is #23

kentcdodds · 2017-07-17T23:25:14Z

In the future, you shouldn't need to worry about it, maintainers can just choose the "Squash and Merge" feature which will squash all changes into a single commit. It's pretty rad.

Alternatively, you can cleanup a PR yourself using an interactive rebase 😄

kitten · 2017-07-17T23:26:28Z

Yea, don't worry about it 😄 But it's not an issue, let's continue with the new PR

burge-is · 2017-07-17T23:30:34Z

@kentcdodds very good to know, thank you.

burge-is added 6 commits July 8, 2017 19:26

TabGate wraps a component

5483938

-makes it tabbable -providing it shortcut keys to enter and exit the wrapped child component.

Changed name to TabGate ... because naming is hard.

dca0037

Moving things to showcase them in demo first.

55b8663

Introducing the tabGate to the demo code

b5715ee

Added span with alert='role' to handle announcing keyboard shortcuts.

9f899ce

Default styling of the navigation tip is now invisible to users not u…

b42a3e4

…sing a screenreader.

Focus and blur now add and delete a styled outline

e0551d7

burge-is added 2 commits July 11, 2017 11:31

Removing TabGate and porting it's logic to the Editor component directly

9189aff

Removing conflicts related to spacing

24be04a

kentcdodds reviewed Jul 11, 2017

View reviewed changes

kitten suggested changes Jul 11, 2017

View reviewed changes

burge-is added 2 commits July 12, 2017 06:50

Removed unnecessary code and applied suggested formatting.

221ebd5

.tabGuarded becomes .prism.tab-guarded

00d9a41

Fixing unwanted Esc key behavior.

35aef0c

Moving the tabGuarded flag into the state.

f57dc93

kitten suggested changes Jul 12, 2017

View reviewed changes

Moving tabGuarded state logic to the end of onKeyUp so that it does n…

0972e4d

…ot interfere with all preceding key handling

kitten suggested changes Jul 12, 2017

View reviewed changes

adding onChange handler to remove the tabGuarded property when a user…

2965b3a

… attempts to change the contents of Editor

kitten approved these changes Jul 13, 2017

View reviewed changes

The logic is now if you are tabGuarded and hit anything other than ta…

faf82c1

…b or escape you are editing.

burge-is mentioned this pull request Jul 17, 2017

a11y fix - Correcting Editor swallowing a tab key press #23

Closed

burge-is closed this Jul 17, 2017

Issue19 a11y #21

Issue19 a11y #21

Conversation

burge-is commented Jul 9, 2017

burge-is commented Jul 9, 2017

kentcdodds commented Jul 10, 2017

burge-is commented Jul 10, 2017

burge-is commented Jul 10, 2017

kentcdodds commented Jul 10, 2017

kentcdodds commented Jul 10, 2017

burge-is commented Jul 10, 2017 • edited

kitten commented Jul 10, 2017

kentcdodds commented Jul 10, 2017

burge-is commented Jul 11, 2017

kitten commented Jul 11, 2017

burge-is commented Jul 11, 2017 • edited

kentcdodds commented Jul 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

burge-is Jul 11, 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

burge-is commented Jul 11, 2017 • edited

kentcdodds commented Jul 11, 2017

burge-is commented Jul 11, 2017

kentcdodds commented Jul 11, 2017

kitten left a 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

burge-is commented Jul 12, 2017 • edited

burge-is commented Jul 12, 2017

kitten commented Jul 12, 2017

kentcdodds commented Jul 12, 2017

burge-is commented Jul 12, 2017 • edited

Choose a reason for hiding this comment

kitten commented Jul 12, 2017

burge-is commented Jul 12, 2017

kitten left a 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

burge-is Jul 12, 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

burge-is commented Jul 13, 2017 • edited

kitten commented Jul 13, 2017

burge-is commented Jul 13, 2017 • edited

burge-is commented Jul 17, 2017

kitten commented Jul 17, 2017

burge-is commented Jul 17, 2017 • edited

kentcdodds commented Jul 17, 2017

kitten commented Jul 17, 2017

burge-is commented Jul 17, 2017

burge-is commented Jul 10, 2017 •

edited

burge-is commented Jul 11, 2017 •

edited

burge-is Jul 11, 2017 •

edited

burge-is commented Jul 11, 2017 •

edited

burge-is commented Jul 12, 2017 •

edited

burge-is commented Jul 12, 2017 •

edited

burge-is Jul 12, 2017 •

edited

burge-is commented Jul 13, 2017 •

edited

burge-is commented Jul 13, 2017 •

edited

burge-is commented Jul 17, 2017 •

edited