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

Components: Add isFocusable state to Button #19337

Merged
merged 8 commits into from
Jan 13, 2020
Merged

Components: Add isFocusable state to Button #19337

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
21e3c9c
Components: Add isFocusable state to Button
diegohaz Dec 27, 2019
9fb891f
Make isFocusable true by default
diegohaz Jan 6, 2020
b1957ac
Update styles
diegohaz Jan 6, 2020
452926c
Remove stopImmediatePropagation call
diegohaz Jan 6, 2020
25a0fb3
Fix unit tests
diegohaz Jan 6, 2020
3bc6257
Fix e2e tests
diegohaz Jan 6, 2020
db17a04
Merge branch 'master' into update/button-is-focusable
diegohaz Jan 6, 2020
8d9bb96
Make isFocusable prop experimental and false by default
diegohaz Jan 10, 2020
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
2 changes: 1 addition & 1 deletion packages/block-editor/src/components/inserter/test/menu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ describe( 'InserterMenu', () => {
TestUtils.Simulate.click( layoutTab );

const disabledBlocks = element.querySelectorAll(
'.block-editor-block-types-list__item[disabled]'
'.block-editor-block-types-list__item[disabled], .block-editor-block-types-list__item[aria-disabled="true"]'
);

expect( disabledBlocks ).toHaveLength( 1 );
Expand Down
26 changes: 23 additions & 3 deletions packages/components/src/button/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ import { forwardRef } from '@wordpress/element';
import Tooltip from '../tooltip';
import Icon from '../icon';

const disabledEventsOnDisabledButton = [
'onMouseDown',
'onClick',
];

export function Button( props, ref ) {
const {
href,
Expand All @@ -39,6 +44,7 @@ export function Button( props, ref ) {
shortcut,
label,
children,
__experimentalIsFocusable: isFocusable,
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the story of this prop, should we make it stable @gziolo ? Recently I found a use-case where we have a button that switches from disabled to enabled and back and this causes focus loss unless this prop is true. So I'm going to use it but since it's now 2 years old, I was wondering if we should mark it stable? What's the reason for the experimental state.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we can promote it to stable after checking how the same functionality has evolved in Reakit.

Copy link
Member Author

Choose a reason for hiding this comment

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

In Reakit it was named focusable. That caused a bit of confusion. It wasn't obvious that the prop would make the element focusable when it was disabled. Some people were setting this prop to false thinking it would disable focus on the element.

We're renaming it to accessibleWhenDisabled in v2 (this name may still change though).

...additionalProps
} = props;

Expand All @@ -62,13 +68,27 @@ export function Button( props, ref ) {
'has-icon': !! icon,
} );

const Tag = href !== undefined && ! disabled ? 'a' : 'button';
const trulyDisabled = disabled && ! isFocusable;
const Tag = href !== undefined && ! trulyDisabled ? 'a' : 'button';
const tagProps = Tag === 'a' ?
{ href, target } :
{ type: 'button', disabled, 'aria-pressed': isPressed };
{ type: 'button', disabled: trulyDisabled, 'aria-pressed': isPressed };

if ( disabled && isFocusable ) {
// In this case, the button will be disabled, but still focusable and
// perceivable by screen reader users.
tagProps[ 'aria-disabled' ] = true;

for ( const disabledEvent of disabledEventsOnDisabledButton ) {
additionalProps[ disabledEvent ] = ( event ) => {
event.stopPropagation();
event.preventDefault();
};
}
}

// Should show the tooltip if...
const shouldShowTooltip = ! disabled && (
const shouldShowTooltip = ! trulyDisabled && (
// an explicit tooltip is passed or...
( showTooltip && label ) ||
// there's a shortcut or...
Expand Down
26 changes: 26 additions & 0 deletions packages/components/src/button/stories/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,16 @@ export const disabled = () => {
);
};

export const disabledFocusable = () => {
const label = text( 'Label', 'Disabled Button' );

return (
<Button disabled __experimentalIsFocusable>
{ label }
</Button>
);
};

export const link = () => {
const label = text( 'Label', 'Link Button' );

Expand Down Expand Up @@ -101,6 +111,22 @@ export const icon = () => {
);
};

export const disabledFocusableIcon = () => {
const usedIcon = text( 'Icon', 'ellipsis' );
const label = text( 'Label', 'More' );
const size = number( 'Size' );

return (
<Button
icon={ usedIcon }
label={ label }
iconSize={ size }
disabled
__experimentalIsFocusable
/>
);
};

export const groupedIcons = () => {
const GroupContainer = ( { children } ) => (
<div style={ { display: 'inline-flex' } }>{ children }</div>
Expand Down
10 changes: 5 additions & 5 deletions packages/components/src/button/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@

}

&:active:enabled {
&:not([aria-disabled="true"]):active:enabled {
background: #f3f5f6;
color: color(theme(button) shade(5%));
border-color: #7e8993;
Expand Down Expand Up @@ -87,7 +87,7 @@
0 0 0 3px color(theme(button));
}

&:active:enabled {
&:not([aria-disabled="true"]):active:enabled {
background: color(theme(button) shade(20%));
border-color: color(theme(button) shade(20%));
color: $white;
Expand Down Expand Up @@ -149,7 +149,7 @@
height: auto;

&:not(:disabled):not([aria-disabled="true"]):hover,
&:active {
&:not([aria-disabled="true"]):active {
color: #00a0d2;
}

Expand All @@ -166,7 +166,7 @@
color: $alert-red;
}

&:active {
&:not([aria-disabled="true"]):active {
color: inherit;
}

Expand Down Expand Up @@ -212,7 +212,7 @@
outline: none;
}

&:active:focus:enabled {
&:not([aria-disabled="true"]):active:focus:enabled {
box-shadow: none;
}
Copy link
Contributor

@youknowriad youknowriad Dec 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The styles changes make me a bit nervous :P because we have some many button styles ovverrides in the UI that I know this can trigger specificity issues and should be tested properly.


Expand Down
7 changes: 7 additions & 0 deletions packages/components/src/button/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ describe( 'Button', () => {
expect( button.hasClass( 'is-primary' ) ).toBe( false );
expect( button.hasClass( 'is-pressed' ) ).toBe( false );
expect( button.prop( 'disabled' ) ).toBeUndefined();
expect( button.prop( 'aria-disabled' ) ).toBeUndefined();
expect( button.prop( 'type' ) ).toBe( 'button' );
expect( button.type() ).toBe( 'button' );
} );
Expand Down Expand Up @@ -58,6 +59,12 @@ describe( 'Button', () => {
expect( button.prop( 'disabled' ) ).toBe( true );
} );

it( 'should add only aria-disabled attribute when disabled and isFocusable are true', () => {
const button = shallow( <Button disabled __experimentalIsFocusable /> );
expect( button.prop( 'disabled' ) ).toBe( false );
expect( button.prop( 'aria-disabled' ) ).toBe( true );
} );

it( 'should not poss the prop target into the element', () => {
const button = shallow( <Button target="_blank" /> );
expect( button.prop( 'target' ) ).toBeUndefined();
Expand Down
2 changes: 1 addition & 1 deletion packages/e2e-tests/specs/editor/various/editor-modes.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ describe( 'Editing modes (visual/HTML)', () => {
expect( noBlocksElement ).not.toBeNull();

// The inserter is disabled
const disabledInserter = await page.$( '.block-editor-inserter > button:disabled' );
const disabledInserter = await page.$( '.block-editor-inserter > button:disabled, .block-editor-inserter > button[aria-disabled="true"]' );
expect( disabledInserter ).not.toBeNull();
} );
} );
2 changes: 1 addition & 1 deletion packages/e2e-tests/specs/editor/various/preview.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ describe( 'Preview', () => {

// Disabled until content present.
const isPreviewDisabled = await editorPage.$$eval(
'.editor-post-preview:not( :disabled )',
'.editor-post-preview:not( :disabled ):not( [aria-disabled="true"] )',
( enabledButtons ) => ! enabledButtons.length,
);
expect( isPreviewDisabled ).toBe( true );
Expand Down
44 changes: 44 additions & 0 deletions storybook/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,6 +563,50 @@ exports[`Storyshots Components|Button Disabled 1`] = `
</button>
`;

exports[`Storyshots Components|Button Disabled Focusable 1`] = `
<button
aria-disabled={true}
className="components-button"
disabled={false}
onClick={[Function]}
onMouseDown={[Function]}
type="button"
>
Disabled Button
</button>
`;

exports[`Storyshots Components|Button Disabled Focusable Icon 1`] = `
<button
aria-disabled={true}
aria-label="More"
className="components-button has-icon"
disabled={false}
onBlur={[Function]}
onClick={[Function]}
onFocus={[Function]}
onMouseDown={[Function]}
onMouseEnter={[Function]}
onMouseLeave={[Function]}
type="button"
>
<svg
aria-hidden="true"
className="dashicon dashicons-ellipsis"
focusable="false"
height={20}
role="img"
viewBox="0 0 20 20"
width={20}
xmlns="http://www.w3.org/2000/svg"
>
<path
d="M5 10c0 1.1-.9 2-2 2s-2-.9-2-2 .9-2 2-2 2 .9 2 2zm12-2c-1.1 0-2 .9-2 2s.9 2 2 2 2-.9 2-2-.9-2-2-2zm-7 0c-1.1 0-2 .9-2 2s.9 2 2 2 2-.9 2-2-.9-2-2-2z"
/>
</svg>
</button>
`;

exports[`Storyshots Components|Button Disabled Link 1`] = `
<button
className="components-button"
Expand Down