-
Notifications
You must be signed in to change notification settings - Fork 55
[RFC] feat(Accessibility): Add focus trap zone #239
Conversation
…at/acc-focuszone-in-rendercomponent
…in-rendercomponent
…in-rendercomponent
…b.com/stardust-ui/react into feat/acc-focuszone-in-rendercomponent
…in-rendercomponent
…/react into feat/focus-trap-zone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please, see comments
<Button content={btnContent} onClick={this.openPortal} /> | ||
<Portal | ||
trapFocus={{ | ||
// When 'false', all clicks outside the Portal will be caught and not handled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
return ( | ||
open && ( | ||
<Ref innerRef={this.handlePortalRef}> | ||
<PortalInner onMount={this.handleMount} onUnmount={this.handleUnmount}> | ||
{childrenExist(children) ? children : content} | ||
{trapFocus ? ( | ||
<FocusTrapZone {...focusTrapZoneProps}>{contentToRender}</FocusTrapZone> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
private createRef = elem => (this._root.current = ReactDOM.findDOMNode(elem) as HTMLElement) | ||
|
||
static propTypes = { | ||
as: customPropTypes.as, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't we provide any descriptions for these? Thos will be helpful even from the dev perspective
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Descriptions for props is provided here FocusTrapZone.types.tsx
. Do you think we should duplicate it?
} | ||
} | ||
|
||
private _findElementAndFocusAsync = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
private _unsubscribeFromEvents = () => { | ||
const { forceFocusInsideTrap, isClickableOutsideFocusTrap } = this.props | ||
if (forceFocusInsideTrap) { | ||
eventStack.unsub('focus', this._handleOutsideFocus, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not related to this PR, but this is so prone to accidental mistakes like in the following cases:
Case 1
eventStack.sub('focus', this._handleOutsideFocus, { target: window })
...
// nothing is unsubscribed, by default `document` is used as a target
eventStack.unsub('focus', e => this._handleOutsideFocus(e))
Case 2
eventStack.sub('focus', e => this._handleOutsideFocus(e))
...
// oops, nothing is unsubscribed
eventStack.unsub('focus', e => this._handleOutsideFocus(e))
Case 3
private _handleOutsideFocus(e) {
...
// oops, I am not 'this' here (thoughts of the type's instance)
this.handle(e)
}
...
eventStack.sub('focus', this._handleOutsideFocus)
} | ||
|
||
private _hideContentFromAccessibilityTree = () => { | ||
const bodyChildren = (document.body && document.body.children) || [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets explicitly put in comments somewhere (probably, in comments to FocusTrapZone
class) the fact that this is a guy who is able to work in browser environment only
if (bodyChildren.length && !document.body.contains(this._root.current)) { | ||
// In case popup render options will change | ||
console.warn( | ||
'Body does not contain trap zone element as expected. If it is done intentionally, please, make sure to update FocusTrapZone.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still, this message is a bit confusing - as we need just to ask client to use ensure that trap zone element is placed inside body. Lets express exactly the same thought in the warning message:
Body does not contain trap zone element as expected - ensure that trapped DOM element is a child of .
|
||
if (win) { | ||
// element.focus() is a no-op if the element is no longer in the DOM, meaning this is always safe | ||
win.requestAnimationFrame(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems that setTimeout
would be enough to achieve this goal - why we are using animation frames?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as an additional benefit of this approach - we might use handler object returned from this method to avoid potential situations where callback will be called multiple times (as multiple subscriptions might be made)
export type KeyActions = { [partName: string]: { [actionName: string]: KeyAction } } | ||
export interface AccessibilityDefinition { | ||
attributes?: AccessibilityAttributesBySlot | ||
keyActions?: KeyActions | ||
handledProps?: (keyof AccessibilityAttributes)[] | ||
focusZone?: FocusZoneDefinition | ||
focusTrapZone?: FocusTrapZoneDefinition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note that this becomes harder for the client to immediately get an idea of why these two are separated and what, actually, drives this division - as both are related to focus handling aspects. Maybe we should take it as a note for the future and address this (API interface) problem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
This PR introduces
FocusTrapZone
.FocusTrapZone
is taken fromui-fabric
library asFocusZone
, and was changed a bit to be consistent with a Stardust library.Notes:
FocusTrapZone
is only used in aPortal
component and is diabled by default.FocusTrapZone
will wrap aPortal
component, in casefocusTrap
is set totrue
FocusTrapZone
, additional propfocusTrapZoneProps
should be passed toPortal
component. Later, in can be configered through accessibility behaviors of the components which will usePortal
, likePopup
.FocusTrapZone
is enabled, by default it will haveisClickableOutsideFocusTrap
prop set totrue
.componentDidMount
,FocusTrapZone
will hide all other elements in the body from accessibility tree, by setting attributearia-hidden=true
, and will remove it oncomponentWillUnmount