From 004cb8b95a4d7173300bd9142209f2246b7d89e8 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Wed, 11 Apr 2018 15:05:03 -0400 Subject: [PATCH] Components: Improve Disabled component (disabled attribute applicable, tabindex removal, pointer-events) (#5748) * Components: Only apply disabled attribute to eligible nodes * Components: Remove tabindex attribute from disabled nodes * Components: Disable pointer events for disabled content --- components/disabled/index.js | 27 +++++++++++++++++++++++++-- components/disabled/style.scss | 1 + components/disabled/test/index.js | 19 ++++++++++++++----- 3 files changed, 40 insertions(+), 7 deletions(-) diff --git a/components/disabled/index.js b/components/disabled/index.js index 4bd44c9b04c907..15da09d4bcb29a 100644 --- a/components/disabled/index.js +++ b/components/disabled/index.js @@ -1,7 +1,7 @@ /** * External dependencies */ -import { debounce } from 'lodash'; +import { includes, debounce } from 'lodash'; /** * WordPress dependencies @@ -14,6 +14,25 @@ import { focus } from '@wordpress/utils'; */ import './style.scss'; +/** + * Names of control nodes which qualify for disabled behavior. + * + * See WHATWG HTML Standard: 4.10.18.5: "Enabling and disabling form controls: the disabled attribute". + * + * @link https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#enabling-and-disabling-form-controls:-the-disabled-attribute + * + * @type {string[]} + */ +const DISABLED_ELIGIBLE_NODE_NAMES = [ + 'BUTTON', + 'FIELDSET', + 'INPUT', + 'OPTGROUP', + 'OPTION', + 'SELECT', + 'TEXTAREA', +]; + class Disabled extends Component { constructor() { super( ...arguments ); @@ -48,10 +67,14 @@ class Disabled extends Component { disable() { focus.focusable.find( this.node ).forEach( ( focusable ) => { - if ( ! focusable.hasAttribute( 'disabled' ) ) { + if ( includes( DISABLED_ELIGIBLE_NODE_NAMES, focusable.nodeName ) ) { focusable.setAttribute( 'disabled', '' ); } + if ( focusable.hasAttribute( 'tabindex' ) ) { + focusable.removeAttribute( 'tabindex' ); + } + if ( focusable.hasAttribute( 'contenteditable' ) ) { focusable.setAttribute( 'contenteditable', 'false' ); } diff --git a/components/disabled/style.scss b/components/disabled/style.scss index 0312776cd1355d..1e6c51a62ff30c 100644 --- a/components/disabled/style.scss +++ b/components/disabled/style.scss @@ -1,5 +1,6 @@ .components-disabled { position: relative; + pointer-events: none; &:after { content: ''; diff --git a/components/disabled/test/index.js b/components/disabled/test/index.js index 1324a0479230be..c6c6167cf85f3a 100644 --- a/components/disabled/test/index.js +++ b/components/disabled/test/index.js @@ -51,13 +51,18 @@ describe( 'Disabled', () => { window.MutationObserver = MutationObserver; } ); - const Form = () =>
; + const Form = () =>
; it( 'will disable all fields', () => { const wrapper = mount(
); - expect( wrapper.find( 'input' ).getDOMNode().hasAttribute( 'disabled' ) ).toBe( true ); - expect( wrapper.find( '[contentEditable]' ).getDOMNode().getAttribute( 'contenteditable' ) ).toBe( 'false' ); + const input = wrapper.find( 'input' ).getDOMNode(); + const div = wrapper.find( '[contentEditable]' ).getDOMNode(); + + expect( input.hasAttribute( 'disabled' ) ).toBe( true ); + expect( div.getAttribute( 'contenteditable' ) ).toBe( 'false' ); + expect( div.hasAttribute( 'tabindex' ) ).toBe( false ); + expect( div.hasAttribute( 'disabled' ) ).toBe( false ); } ); it( 'should cleanly un-disable via reconciliation', () => { @@ -73,8 +78,12 @@ describe( 'Disabled', () => { const wrapper = mount( ); wrapper.setProps( { isDisabled: false } ); - expect( wrapper.find( 'input' ).getDOMNode().hasAttribute( 'disabled' ) ).toBe( false ); - expect( wrapper.find( '[contentEditable]' ).getDOMNode().getAttribute( 'contenteditable' ) ).toBe( 'true' ); + const input = wrapper.find( 'input' ).getDOMNode(); + const div = wrapper.find( '[contentEditable]' ).getDOMNode(); + + expect( input.hasAttribute( 'disabled' ) ).toBe( false ); + expect( div.getAttribute( 'contenteditable' ) ).toBe( 'true' ); + expect( div.hasAttribute( 'tabindex' ) ).toBe( true ); } ); // Ideally, we'd have two more test cases here: