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

Fix disabled publish/update button still clickable #11760

Closed
wants to merge 2 commits into from
Closed

Fix disabled publish/update button still clickable #11760

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df33b59
Fix disabled publish/update button still clickable
Nov 12, 2018
bb141f3
Adjusting e2e test
Nov 12, 2018
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
4 changes: 2 additions & 2 deletions packages/editor/src/components/post-publish-button/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ export class PostPublishButton extends Component {
};

const buttonProps = {
'aria-disabled': isButtonDisabled,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the reason we prefer 'aria-disabled' is to avoid focus loss when the status of the button changes. So instead we do 'aria-disabled' and return early on the onClick handler. Maybe we're missing the early return here.

Is this right @afercia ?

Copy link
Member

Choose a reason for hiding this comment

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

I am not Andrea but yes that's right. We should be using aria-disabled and returning early from the onClick handler.

Copy link
Member

@noisysocks noisysocks Nov 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

From #12073:

From an accessibility perspective, using aria-disabled="true" and "noop'ing" an UI control is equivalent to disabling it with a disable attribute, as long as it is perceived as disabled by all users, i.e. also visually. The relevant difference is that a control with aria-disabled="true" is still focusable. Removing focusability from disabled elements can offer users both advantages and disadvantages. A must read recommendation can be found in the ARIA authoring practices:

5.7 Focusability of disabled controls
https://www.w3.org/TR/wai-aria-practices-1.1/#kbd_disabled_controls

Although those recommendations apply to ARIA widgets, the Gutenberg top bar can be considered a "toolbar", where the ability to discover the functionalities within the toolbar is a primary concern. I'd recommend to adopt a consistent convention for the focusability of these controls and always use aria-disabled="true" / "noop". The Block navigation menu should not open its panel when there are no blocks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Late reply but yes: when there's the need to avoid a focus loss (as when a focused button gets disabled while it's focused), it's possible to use aria-disabled and noop it. At that point, the only difference is that the button is still focusable. This can offer both advantages and disadvantages, to be evaluated on a case by case basis. For more details:

5.7 Focusability of disabled controls
https://www.w3.org/TR/wai-aria-practices-1.1/#kbd_disabled_controls

disabled: isButtonDisabled,
className: 'editor-post-publish-button',
isBusy: isSaving && isPublished,
isLarge: true,
Expand All @@ -88,7 +88,7 @@ export class PostPublishButton extends Component {
};

const toggleProps = {
'aria-disabled': isToggleDisabled,
disabled: isToggleDisabled,
'aria-expanded': isOpen,
className: 'editor-post-publish-panel__toggle',
isBusy: isSaving && isPublished,
Expand Down
16 changes: 8 additions & 8 deletions packages/editor/src/components/post-publish-button/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { PostPublishButton } from '../';
jest.mock( '../../../../../components/src/button' );

describe( 'PostPublishButton', () => {
describe( 'aria-disabled', () => {
describe( 'disabled', () => {
it( 'should be true if post is currently saving', () => {
const wrapper = shallow(
<PostPublishButton
Expand All @@ -21,7 +21,7 @@ describe( 'PostPublishButton', () => {
/>
);

expect( wrapper.prop( 'aria-disabled' ) ).toBe( true );
expect( wrapper.prop( 'disabled' ) ).toBe( true );
} );

it( 'should be true if forceIsSaving is true', () => {
Expand All @@ -33,7 +33,7 @@ describe( 'PostPublishButton', () => {
/>
);

expect( wrapper.prop( 'aria-disabled' ) ).toBe( true );
expect( wrapper.prop( 'disabled' ) ).toBe( true );
} );

it( 'should be true if post is not publishable and not forceIsDirty', () => {
Expand All @@ -45,7 +45,7 @@ describe( 'PostPublishButton', () => {
/>
);

expect( wrapper.prop( 'aria-disabled' ) ).toBe( true );
expect( wrapper.prop( 'disabled' ) ).toBe( true );
} );

it( 'should be true if post is not saveable', () => {
Expand All @@ -56,7 +56,7 @@ describe( 'PostPublishButton', () => {
/>
);

expect( wrapper.prop( 'aria-disabled' ) ).toBe( true );
expect( wrapper.prop( 'disabled' ) ).toBe( true );
} );

it( 'should be true if post saving is locked', () => {
Expand All @@ -68,7 +68,7 @@ describe( 'PostPublishButton', () => {
/>
);

expect( wrapper.prop( 'aria-disabled' ) ).toBe( true );
expect( wrapper.prop( 'disabled' ) ).toBe( true );
} );

it( 'should be false if post is saveable but not publishable and forceIsDirty is true', () => {
Expand All @@ -80,7 +80,7 @@ describe( 'PostPublishButton', () => {
/>
);

expect( wrapper.prop( 'aria-disabled' ) ).toBe( false );
expect( wrapper.prop( 'disabled' ) ).toBe( false );
} );

it( 'should be false if post is publishave and saveable', () => {
Expand All @@ -91,7 +91,7 @@ describe( 'PostPublishButton', () => {
/>
);

expect( wrapper.prop( 'aria-disabled' ) ).toBe( false );
expect( wrapper.prop( 'disabled' ) ).toBe( false );
} );
} );

Expand Down
10 changes: 5 additions & 5 deletions test/e2e/specs/publish-button.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,23 +22,23 @@ describe( 'PostPublishButton', () => {
} );

it( 'should be disabled when post is not saveable', async () => {
const publishButton = await page.$( '.editor-post-publish-button[aria-disabled="true"]' );
const publishButton = await page.$( '.editor-post-publish-button:disabled' );
expect( publishButton ).not.toBeNull();
} );

it( 'should be disabled when post is being saved', async () => {
await page.type( '.editor-post-title__input', 'E2E Test Post' ); // Make it saveable
expect( await page.$( '.editor-post-publish-button[aria-disabled="true"]' ) ).toBeNull();
expect( await page.$( '.editor-post-publish-button:disabled' ) ).toBeNull();

await page.click( '.editor-post-save-draft' );
expect( await page.$( '.editor-post-publish-button[aria-disabled="true"]' ) ).not.toBeNull();
expect( await page.$( '.editor-post-publish-button:disabled' ) ).not.toBeNull();
} );

it( 'should be disabled when metabox is being saved', async () => {
await page.type( '.editor-post-title__input', 'E2E Test Post' ); // Make it saveable
expect( await page.$( '.editor-post-publish-button[aria-disabled="true"]' ) ).toBeNull();
expect( await page.$( '.editor-post-publish-button:disabled' ) ).toBeNull();

await page.evaluate( () => window.wp.data.dispatch( 'core/edit-post' ).requestMetaBoxUpdates() );
expect( await page.$( '.editor-post-publish-button[aria-disabled="true"]' ) ).not.toBeNull();
expect( await page.$( '.editor-post-publish-button:disabled' ) ).not.toBeNull();
} );
} );