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.

Sign up for GitHub

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

Jump to bottom

Post visibility panel better keyboard interaction. #1886

Closed
wants to merge 4 commits into from
Closed

Post visibility panel better keyboard interaction. #1886

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5047c00
Post visibility panel better keyboard interaction.
afercia Jul 13, 2017
91d9e90
Use onKeyDown prop and refs.
afercia Jul 17, 2017
d2d3765
Fix uncontrolled controlled input warning.
afercia Jul 17, 2017
84d7587
Restore keyboard interaction after Popover introduction.
afercia Aug 3, 2017
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
115 changes: 71 additions & 44 deletions editor/sidebar/post-visibility/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { find } from 'lodash';
import { __ } from '@wordpress/i18n';
import { Component } from '@wordpress/element';
import { PanelRow, Popover, withInstanceId } from '@wordpress/components';
import { keycodes } from '@wordpress/utils';

/**
* Internal Dependencies
Expand All @@ -22,6 +23,8 @@ import {
} from '../../selectors';
import { editPost, savePost } from '../../actions';

const { ESCAPE } = keycodes;

class PostVisibility extends Component {
constructor( props ) {
super( ...arguments );
Expand All @@ -30,6 +33,8 @@ class PostVisibility extends Component {
this.setPublic = this.setPublic.bind( this );
this.setPrivate = this.setPrivate.bind( this );
this.setPasswordProtected = this.setPasswordProtected.bind( this );
this.handleKeyDown = this.handleKeyDown.bind( this );
this.handleBlur = this.handleBlur.bind( this );

this.state = {
opened: false,
Expand Down Expand Up @@ -71,6 +76,21 @@ class PostVisibility extends Component {
this.setState( { opened: false } );
}

handleBlur( event ) {
if ( this.state.opened && event.relatedTarget !== null && ! this.dialog.contains( event.relatedTarget ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

While explicitness could be handy, I don't know that there's ever a case that event.relatedTarget is null and this.dialog.contains( null ) would ever be true. In other words, we could safely drop && event.relatedTarget !== null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This needs to be changed anyways, since relatedTarget doesn't work on Safari 10 when using VoiceOver (see comment on the issue). We need to find an alternative solution.

Copy link
Member

Choose a reason for hiding this comment

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

Apparently Firefox has some issues with relatedTarget as well? There's an interesting idea here for determining the related target via a setTimeout( () => { const relatedTarget = document.activeElement } , 0 ) when the blur occurs.

facebook/react#2011 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes the setTimeout 0 trick is the only possible solution I've found so far, not so spectacularly happy to have to use setTimeout. I guess other solutions would be fragile though. Re: Firefox, I think at the time that comments were posted, Firefox was not supporting relatedTarget. It does now, since version 48 https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/relatedTarget
Instead, IE 11 could be a problem but I can't test in IE 11 at the moment 🙂 see #1397 and #1863

this.setState( { opened: false } );
}
}

handleKeyDown( event ) {
if ( this.state.opened && event.keyCode === ESCAPE ) {
event.preventDefault();
event.stopPropagation();
this.setState( { opened: false } );
this.toggle.focus();
}
}

render() {
const { status, visibility, password, onUpdateVisibility, instanceId } = this.props;

Expand Down Expand Up @@ -101,8 +121,6 @@ class PostVisibility extends Component {
];
const getVisibilityLabel = () => find( visibilityOptions, { value: visibility } ).label;

// Disable Reason: The input is inside the label, we shouldn't need the htmlFor
/* eslint-disable jsx-a11y/label-has-for */
return (
<PanelRow className="editor-post-visibility">
<span>{ __( 'Visibility' ) }</span>
Expand All @@ -112,62 +130,71 @@ class PostVisibility extends Component {
aria-expanded={ this.state.opened }
className="editor-post-visibility__toggle button-link"
onClick={ this.toggleDialog }
ref={ toggle => this.toggle = toggle }
>
{ getVisibilityLabel( visibility ) }
</button>

{ this.state.opened &&
<Popover position="bottom left" className="editor-post-visibility__dialog">
<fieldset>
<legend className="editor-post-visibility__dialog-legend">
{ __( 'Post Visibility' ) }
</legend>
{ visibilityOptions.map( ( { value, label, info, onSelect, checked } ) => (
<div key={ value } className="editor-post-visibility__choice">
<input
type="radio"
name={ `editor-post-visibility__setting-${ instanceId }` }
value={ value }
onChange={ onSelect }
checked={ checked }
id={ `editor-post-${ value }-${ instanceId }` }
aria-describedby={ `editor-post-${ value }-${ instanceId }-description` }
className="editor-post-visibility__dialog-radio"
/>
<Popover
position="bottom left"
className="editor-post-visibility__dialog"
>
<div
onKeyDown={ this.handleKeyDown }
onBlur={ this.handleBlur }
ref={ dialog => this.dialog = dialog }
>
<fieldset>
<legend className="editor-post-visibility__dialog-legend">
{ __( 'Post Visibility' ) }
</legend>
{ visibilityOptions.map( ( { value, label, info, onSelect, checked } ) => (
<div key={ value } className="editor-post-visibility__choice">
<input
type="radio"
name={ `editor-post-visibility__setting-${ instanceId }` }
value={ value }
onChange={ onSelect }
checked={ checked }
id={ `editor-post-${ value }-${ instanceId }` }
aria-describedby={ `editor-post-${ value }-${ instanceId }-description` }
className="editor-post-visibility__dialog-radio"
/>
<label
htmlFor={ `editor-post-${ value }-${ instanceId }` }
className="editor-post-visibility__dialog-label"
>
{ label }
</label>
{ <p id={ `editor-post-${ value }-${ instanceId }-description` } className="editor-post-visibility__dialog-info">{ info }</p> }
</div>
) ) }
</fieldset>
{ this.state.hasPassword &&
<div className="editor-post-visibility__dialog-password">
<label
htmlFor={ `editor-post-${ value }-${ instanceId }` }
className="editor-post-visibility__dialog-label"
htmlFor={ `editor-post-visibility__dialog-password-input-${ instanceId }` }
className="screen-reader-text"
>
{ label }
{ __( 'Create password' ) }
</label>
{ <p id={ `editor-post-${ value }-${ instanceId }-description` } className="editor-post-visibility__dialog-info">{ info }</p> }
<input
className="editor-post-visibility__dialog-password-input"
id={ `editor-post-visibility__dialog-password-input-${ instanceId }` }
type="text"
onChange={ updatePassword }
value={ password || '' }
placeholder={ __( 'Use a secure password' ) }
/>
</div>
) ) }
</fieldset>
{ this.state.hasPassword &&
<div className="editor-post-visibility__dialog-password">
<label
htmlFor={ `editor-post-visibility__dialog-password-input-${ instanceId }` }
className="screen-reader-text"
>
{ __( 'Create password' ) }
</label>
<input
className="editor-post-visibility__dialog-password-input"
id={ `editor-post-visibility__dialog-password-input-${ instanceId }` }
type="text"
onChange={ updatePassword }
value={ password }
placeholder={ __( 'Use a secure password' ) }
/>
</div>
}
}
</div>
</Popover>
}
</span>
</PanelRow>
);
/* eslint-enable jsx-a11y/label-has-for */
}
}

Expand Down