Ensure focus-ring class is applied to contenteditable elements #37
Conversation
…ven if no ARIA role attribute is applied. Remove dependency on the dom-matches polyfill. Update the demo with more use cases.
jonathantneal
left a comment
There was a problem hiding this comment.
I’ve left you a note about some small optimizations which I believe will keep the code better focused.
The toLowerCase() methods are unnecessary, and errantly suggest to some developers that the code may be working with non-standard elements (which are the only times tagName returns lowercase).
src/focus-ring.js
Outdated
| function focusTriggersKeyboardModality(el) { | ||
| return matches(el, keyboardModalityWhitelist) && matches(el, ':not([readonly])'); | ||
| var type = el.type; | ||
| var tagName = el.tagName.toLowerCase(); |
There was a problem hiding this comment.
You’re only testing tagName against 'input' and 'textarea'. Therefore, you could safely omit .toLowerCase() and test for 'INPUT' and 'TEXTAREA' instead.
src/focus-ring.js
Outdated
| if (matches(document.activeElement, ':focus')) | ||
| addFocusRingClass(document.activeElement); | ||
| var activeElement = document.activeElement; | ||
| if (activeElement.tagName.toLowerCase() == 'body') |
There was a problem hiding this comment.
Similarly here, there is no browser that will return the tagName of 'BODY' in lowercase.
There was a problem hiding this comment.
@jonathantneal are you sure? I recall IE returning the declared case if in quirks mode. If you're 100% sure I'm happy to remove the use of toLowerCase()
There was a problem hiding this comment.
I am sure.
<body></body><script>document.write(document.body.tagName)</script>In older IE, there would be an issue with new “unknown” elements (back then, HTML5 elements) returning lowercase. It’s unneeded here, because you’re testing for some of the oldest elements known to web. :)
src/focus-ring.js
Outdated
| var tagName = el.tagName.toLowerCase(); | ||
| var tagName = el.tagName; | ||
|
|
||
| if (tagName == 'input' && inputTypesWhitelist[type] && !el.readonly) |
There was a problem hiding this comment.
Be sure to test these changes. I’m not sure 'input' will work because it should now be uppercase?
dist/focus-ring.js
Outdated
| if (tagName == 'textarea' && !el.readonly) | ||
| return true; | ||
|
|
||
| if (el.contentEditable) |
There was a problem hiding this comment.
The possible values of contentEditable are 'inherit', 'true', or 'false'.
|
@alice, any thoughts on this PR? |
alice
left a comment
There was a problem hiding this comment.
Generally in favour, but one concern per my comment.
src/focus-ring.js
Outdated
| if (activeElement.tagName == 'BODY') | ||
| return; | ||
|
|
||
| hadKeyboardEvent = true; |
There was a problem hiding this comment.
Why was this moved below the check?
There was a problem hiding this comment.
Because the more I thought about it, we don't want to set that to true if focus hasn't moved to a focusable control. This way we can use hadKeyboardEvent to determine if a focus moved to a focusable element as a result of keyboard navigation.
There was a problem hiding this comment.
hasKeyboardEvent is not intended for that use, though. I think this could cause a bug if a keyboard event triggered a focus move (for example, activating a button which caused a piece of modal UI to appear). Please revert this change.
There was a problem hiding this comment.
Sorry, I'm not 100% following you regarding the bug that could result from this change. Mind explaining in great detail?
There was a problem hiding this comment.
Ah - I had it backwards. However, since the keyboard event precedes the focus event, this is checking where focus is coming from, rather than going to, isn't it?
There was a problem hiding this comment.
No worries at all. To answer your question: it's the other way around. The fact the keyboard event precedes the focus event actually works to our advantage here because we know the event sequence is: keydown then focus, and therefore the focus handler can key off of hadKeyboardEvent to know focus happened as a result of keyboard, and then add the focus-ring class.
There was a problem hiding this comment.
Right: so at the point where this fires, activeElement is the element which is about to lose focus, isn't it? So why not set hadKeyboardEvent based on that?
There was a problem hiding this comment.
Ah, yes! Now I see what you're saying. Will change.
|
👍 |
This PR addresses issue #7 and ensures the
focus-ringclass is applied to contenteditable elements. Additionally, it removes the dom-matches polyfill dependency and updates the demo with more use cases.