From a68972a42be5c136087c9a140164b98078d05c5e Mon Sep 17 00:00:00 2001 From: Andrew Duthie <andrew@andrewduthie.com> Date: Fri, 22 Sep 2017 17:12:19 -0400 Subject: [PATCH 1/2] Editable: Assign attributes to node by mirroring --- blocks/editable/index.js | 69 +++++++++++++++++++++++++++++++------- blocks/editable/style.scss | 36 +++++++++++--------- blocks/editable/tinymce.js | 62 +++++++++++++++++----------------- 3 files changed, 108 insertions(+), 59 deletions(-) diff --git a/blocks/editable/index.js b/blocks/editable/index.js index 7198db9f50d83..f79154570ec74 100644 --- a/blocks/editable/index.js +++ b/blocks/editable/index.js @@ -12,6 +12,7 @@ import { identity, find, defer, + pickBy, noop, } from 'lodash'; import { nodeListToReact } from 'dom-react'; @@ -74,6 +75,8 @@ export default class Editable extends Component { ); } + this.bindMirrorRef = this.bindNodeRef.bind( this, 'mirror' ); + this.bindEditorRef = this.bindNodeRef.bind( this, 'editor' ); this.onInit = this.onInit.bind( this ); this.getSettings = this.getSettings.bind( this ); this.onSetup = this.onSetup.bind( this ); @@ -89,6 +92,8 @@ export default class Editable extends Component { this.onBeforePastePreProcess = this.onBeforePastePreProcess.bind( this ); this.onPaste = this.onPaste.bind( this ); + this.nodeRefs = {}; + this.state = { formats: {}, empty: ! value || ! value.length, @@ -96,6 +101,10 @@ export default class Editable extends Component { }; } + bindNodeRef( name, node ) { + this.nodeRefs[ name ] = node; + } + getSettings( settings ) { return ( this.props.getSettings || identity )( { ...settings, @@ -143,6 +152,9 @@ export default class Editable extends Component { onInit() { this.updateFocus(); + + // Start mirroring attributes after editor initializes + this.nodeRefs.editor.mirrorNode( this.nodeRefs.mirror ); } onFocus() { @@ -594,7 +606,6 @@ export default class Editable extends Component { render() { const { tagName: Tagname = 'div', - style, value, focus, wrapperClassname, @@ -606,6 +617,19 @@ export default class Editable extends Component { keepPlaceholderOnFocus = false, } = this.props; + // Assign as props to editor node those not explicitly handled by + // Editable. Since event handlers are proxied by TinyMCE, ignore any + // props prefixed by `on` (see: `proxyPropHandler`). + const editorProps = pickBy( this.props, ( propValue, propKey ) => ( + ! this.constructor.propTypes.hasOwnProperty( propKey ) && + ! /^on[A-Z]/.test( propKey ) + ) ); + + editorProps.className = classnames( 'blocks-editable__tinymce', className ); + if ( ! editorProps[ 'aria-label' ] ) { + editorProps[ 'aria-label' ] = placeholder; + } + // Generating a key that includes `tagName` ensures that if the tag // changes, we unmount and destroy the previous TinyMCE element, then // mount and initialize a new child element in its place. @@ -635,30 +659,49 @@ export default class Editable extends Component { { formatToolbar } </div> } + <div + data-mirror + aria-hidden + ref={ this.bindMirrorRef } + { ...editorProps } /> + { isPlaceholderVisible && + <Tagname data-placeholder aria-hidden { ...editorProps }> + { MultilineTag ? <MultilineTag>{ placeholder }</MultilineTag> : placeholder } + </Tagname> + } <TinyMCE + ref={ this.bindEditorRef } tagName={ Tagname } getSettings={ this.getSettings } onSetup={ this.onSetup } - style={ style } defaultValue={ value } - isPlaceholderVisible={ isPlaceholderVisible } - label={ placeholder } - className={ className } key={ key } + additionalProps={ editorProps } /> - { isPlaceholderVisible && - <Tagname - className={ classnames( 'blocks-editable__tinymce', className ) } - style={ style } - > - { MultilineTag ? <MultilineTag>{ placeholder }</MultilineTag> : placeholder } - </Tagname> - } </div> ); } } +Editable.propTypes = { + className: noop, + focus: noop, + formattingControls: noop, + getSettings: noop, + inlineToolbar: noop, + keepPlaceholderOnFocus: noop, + multiline: noop, + onChange: noop, + onFocus: noop, + onMerge: noop, + onReplace: noop, + onSetup: noop, + placeholder: noop, + tagName: noop, + value: noop, + wrapperClassname: noop, +}; + Editable.contextTypes = { onUndo: noop, }; diff --git a/blocks/editable/style.scss b/blocks/editable/style.scss index 7fa2bf97d4473..365c343a73eb3 100644 --- a/blocks/editable/style.scss +++ b/blocks/editable/style.scss @@ -26,6 +26,26 @@ color: $blue-medium-500; } + &[data-mirror] { + display: none; + } + + &[data-placeholder] { + opacity: 0.5; + pointer-events: none; + + & + .blocks-editable__tinymce { + position: absolute; + top: 0; + width: 100%; + margin-top: 0; + + & > p { + margin-top: 0; + } + } + } + &:focus a[data-mce-selected] { padding: 0 2px; margin: 0 -2px; @@ -46,22 +66,6 @@ &:focus code[data-mce-selected] { background: $light-gray-400; } - - &[data-is-placeholder-visible="true"] { - position: absolute; - top: 0; - width: 100%; - margin-top: 0; - - & > p { - margin-top: 0; - } - } - - & + .blocks-editable__tinymce { - opacity: 0.5; - pointer-events: none; - } } .has-drop-cap .blocks-editable__tinymce:not( :focus ) { diff --git a/blocks/editable/tinymce.js b/blocks/editable/tinymce.js index 415a388c27a4e..0d73b0eb44165 100644 --- a/blocks/editable/tinymce.js +++ b/blocks/editable/tinymce.js @@ -2,8 +2,6 @@ * External dependencies */ import tinymce from 'tinymce'; -import { isEqual } from 'lodash'; -import classnames from 'classnames'; /** * WordPress dependencies @@ -15,6 +13,17 @@ export default class TinyMCE extends Component { this.initialize(); } + componentWillUnmount() { + if ( this.observer ) { + this.observer.disconnect(); + } + + if ( this.editor ) { + this.editor.destroy(); + delete this.editor; + } + } + shouldComponentUpdate() { // We must prevent rerenders because TinyMCE will modify the DOM, thus // breaking React's ability to reconcile changes. @@ -23,31 +32,26 @@ export default class TinyMCE extends Component { return false; } - componentWillReceiveProps( nextProps ) { - const name = 'data-is-placeholder-visible'; - const isPlaceholderVisible = String( !! nextProps.isPlaceholderVisible ); - - if ( this.editorNode.getAttribute( name ) !== isPlaceholderVisible ) { - this.editorNode.setAttribute( name, isPlaceholderVisible ); - } - - if ( ! isEqual( this.props.style, nextProps.style ) ) { - this.editorNode.setAttribute( 'style', '' ); - Object.assign( this.editorNode.style, nextProps.style ); - } - - if ( ! isEqual( this.props.className, nextProps.className ) ) { - this.editorNode.className = classnames( nextProps.className, 'blocks-editable__tinymce' ); - } - } - - componentWillUnmount() { - if ( ! this.editor ) { - return; - } + mirrorNode( node ) { + // Since React reconciliation is disabled for the TinyMCE node, we sync + // attribute changes using a mutation observer on a node within the + // parent Editable which receives reconciliation from Editable props. + this.observer = new window.MutationObserver( ( mutations ) => { + mutations.forEach( ( mutation ) => { + const { attributeName } = mutation; + const nextValue = node.getAttribute( attributeName ); + + if ( null === nextValue ) { + this.editorNode.removeAttribute( attributeName ); + } else { + this.editorNode.setAttribute( attributeName, nextValue ); + } + } ); + } ); - this.editor.destroy(); - delete this.editor; + this.observer.observe( node, { + attributes: true, + } ); } initialize() { @@ -83,7 +87,7 @@ export default class TinyMCE extends Component { } render() { - const { tagName = 'div', style, defaultValue, label, className } = this.props; + const { tagName = 'div', defaultValue, additionalProps } = this.props; // If a default value is provided, render it into the DOM even before // TinyMCE finishes initializing. This avoids a short delay by allowing @@ -97,9 +101,7 @@ export default class TinyMCE extends Component { ref: ( node ) => this.editorNode = node, contentEditable: true, suppressContentEditableWarning: true, - className: classnames( className, 'blocks-editable__tinymce' ), - style, - 'aria-label': label, + ...additionalProps, }, children ); } } From 081fa8be39678b2e948b05c3926d49616a25300b Mon Sep 17 00:00:00 2001 From: Andrew Duthie <andrew@andrewduthie.com> Date: Fri, 22 Sep 2017 17:12:44 -0400 Subject: [PATCH 2/2] Autocomplete: Use combobox role with ARIA attributes --- components/autocomplete/index.js | 19 ++++++++++++++----- components/autocomplete/test/index.js | 14 +++++++++++--- 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/components/autocomplete/index.js b/components/autocomplete/index.js index 9247198130ce7..495ed19b5b831 100644 --- a/components/autocomplete/index.js +++ b/components/autocomplete/index.js @@ -16,10 +16,11 @@ import { keycodes } from '@wordpress/utils'; import './style.scss'; import Button from '../button'; import Popover from '../popover'; +import withInstanceId from '../higher-order/with-instance-id'; const { ENTER, ESCAPE, UP, DOWN } = keycodes; -class Autocomplete extends Component { +export class Autocomplete extends Component { static getInitialState() { return { isOpen: false, @@ -180,10 +181,12 @@ class Autocomplete extends Component { } render() { - const { children, className } = this.props; + const { children, className, instanceId } = this.props; const { isOpen, selectedIndex } = this.state; const classes = classnames( 'components-autocomplete__popover', className ); const filteredOptions = this.getFilteredOptions(); + const listBoxId = `components-autocomplete-listbox-${ instanceId }`; + const activeId = `components-autocomplete-item-${ instanceId }-${ selectedIndex }`; // Blur is applied to the wrapper node, since if the child is Editable, // the event will not have `relatedTarget` assigned. @@ -196,6 +199,10 @@ class Autocomplete extends Component { { cloneElement( Children.only( children ), { onInput: this.search, onKeyDown: this.setSelectedIndex, + role: 'combobox', + 'aria-expanded': isOpen, + 'aria-activedescendant': isOpen ? activeId : null, + 'aria-owns': isOpen ? listBoxId : null, } ) } <Popover isOpen={ isOpen && filteredOptions.length > 0 } @@ -204,13 +211,15 @@ class Autocomplete extends Component { className={ classes } > <ul - role="menu" + id={ listBoxId } + role="listbox" className="components-autocomplete__results" > { filteredOptions.map( ( option, index ) => ( <li key={ option.value } - role="menuitem" + id={ `components-autocomplete-item-${ instanceId }-${ index }` } + role="option" className={ classnames( 'components-autocomplete__result', { 'is-selected': index === selectedIndex, } ) } @@ -227,4 +236,4 @@ class Autocomplete extends Component { } } -export default Autocomplete; +export default withInstanceId( Autocomplete ); diff --git a/components/autocomplete/test/index.js b/components/autocomplete/test/index.js index c4543b7e1b5ea..f937e9f85a4eb 100644 --- a/components/autocomplete/test/index.js +++ b/components/autocomplete/test/index.js @@ -11,7 +11,7 @@ import { keycodes } from '@wordpress/utils'; /** * Internal dependencies */ -import Autocomplete from '../'; +import { Autocomplete } from '../'; const { ENTER, ESCAPE, UP, DOWN, SPACE } = keycodes; @@ -53,13 +53,17 @@ describe( 'Autocomplete', () => { </Autocomplete> ); const popover = wrapper.find( 'Popover' ); + const clone = wrapper.find( '[data-ok]' ); expect( wrapper.state( 'isOpen' ) ).toBe( false ); expect( popover.prop( 'focusOnOpen' ) ).toBe( false ); expect( popover.hasClass( 'my-autocomplete' ) ).toBe( true ); expect( popover.hasClass( 'components-autocomplete__popover' ) ).toBe( true ); expect( wrapper.hasClass( 'components-autocomplete' ) ).toBe( true ); - expect( wrapper.find( '[data-ok]' ) ).toHaveLength( 1 ); + expect( clone ).toHaveLength( 1 ); + expect( clone.prop( 'aria-expanded' ) ).toBe( false ); + expect( clone.prop( 'aria-activedescendant' ) ).toBe( null ); + expect( clone.prop( 'aria-owns' ) ).toBe( null ); } ); it( 'opens on absent trigger prefix search', () => { @@ -68,8 +72,9 @@ describe( 'Autocomplete', () => { <div contentEditable /> </Autocomplete> ); + const clone = wrapper.find( '[contentEditable]' ); - wrapper.find( '[contentEditable]' ).simulate( 'input', { + clone.simulate( 'input', { target: { textContent: 'b', }, @@ -80,6 +85,9 @@ describe( 'Autocomplete', () => { expect( wrapper.state( 'search' ) ).toEqual( /b/i ); expect( wrapper.find( 'Popover' ).prop( 'isOpen' ) ).toBe( true ); expect( wrapper.find( '.components-autocomplete__result' ) ).toHaveLength( 1 ); + expect( clone.prop( 'aria-expanded' ) ).toBe( false ); + expect( clone.prop( 'aria-activedescendant' ) ).toBe( null ); + expect( clone.prop( 'aria-owns' ) ).toBe( null ); } ); it( 'does not render popover as open if no results', () => {