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.

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

Use inert attribute instead of useDisabled #44865

Merged
merged 7 commits into from
Oct 12, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
30 changes: 30 additions & 0 deletions lib/compat/wordpress-6.2/script-loader.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<?php
/**
* Updates the script-loader.php file
*
* @package gutenberg
*/

/**
* Registers vendor JavaScript files to be used as dependencies of the editor
* and plugins.
*
* This function is called from a script during the plugin build process, so it
* should not call any WordPress PHP functions.
*
* @since 13.0
*
* @param WP_Scripts $scripts WP_Scripts instance.
*/
function gutenberg_register_vendor_scripts_62( $scripts ) {
$extension = SCRIPT_DEBUG ? '.js' : '.min.js';

$script = $scripts->query( 'wp-inert-polyfill', 'registered' );
if ( ! $script ) {
$scripts->add( 'wp-inert-polyfill', gutenberg_url( 'build/vendors/inert-polyfill' . $extension ), array() );
}

$script = $scripts->query( 'wp-polyfill', 'registered' );
$script->deps = array_merge( $script->deps, array( 'wp-inert-polyfill' ) );
}
add_action( 'wp_default_scripts', 'gutenberg_register_vendor_scripts_62' );
3 changes: 3 additions & 0 deletions lib/load.php
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,9 @@ function gutenberg_is_experiment_enabled( $name ) {
require __DIR__ . '/compat/wordpress-6.1/template-parts-screen.php';
require __DIR__ . '/compat/wordpress-6.1/theme.php';

// WordPress 6.2 compat.
require __DIR__ . '/compat/wordpress-6.2/script-loader.php';

// Experimental features.
remove_action( 'plugins_loaded', '_wp_theme_json_webfonts_handler' ); // Turns off WP 6.0's stopgap handler for Webfonts API.
require __DIR__ . '/experimental/block-editor-settings-mobile.php';
Expand Down
5 changes: 5 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,8 @@
"@wordpress/viewport": "file:packages/viewport",
"@wordpress/warning": "file:packages/warning",
"@wordpress/widgets": "file:packages/widgets",
"@wordpress/wordcount": "file:packages/wordcount"
"@wordpress/wordcount": "file:packages/wordcount",
"wicg-inert": "3.1.2"
},
"devDependencies": {
"@actions/core": "1.8.0",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
/**
* WordPress dependencies
*/
import { Disabled } from '@wordpress/components';
import { useResizeObserver, pure, useRefEffect } from '@wordpress/compose';
import { useSelect } from '@wordpress/data';
import { useMemo } from '@wordpress/element';
import { Disabled } from '@wordpress/components';

/**
* Internal dependencies
Expand Down
9 changes: 2 additions & 7 deletions packages/block-editor/src/components/block-preview/live.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,3 @@
/**
* WordPress dependencies
*/
import { Disabled } from '@wordpress/components';

/**
* Internal dependencies
*/
Expand All @@ -16,9 +11,9 @@ export default function LiveBlockPreview( { onClick } ) {
onClick={ onClick }
onKeyPress={ onClick }
>
<Disabled>
<div inert="true">
<BlockList />
</Disabled>
</div>
</div>
);
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { render, screen, waitFor } from '@testing-library/react';
import { render, screen } from '@testing-library/react';

/**
* WordPress dependencies
Expand Down Expand Up @@ -99,12 +99,6 @@ describe( 'useBlockPreview', () => {
);
expect( previewedBlockContents ).toBeInTheDocument();

// Test elements within block contents are disabled.
await waitFor( () => {
const button = screen.getByText( 'Button' );
expect( button.hasAttribute( 'disabled' ) ).toBe( true );
} );
Comment on lines -103 to -106
Copy link
Contributor

Choose a reason for hiding this comment

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

Testing that the preview is disabled seemed like a meaningful test — instead of deleting it, we could have replaced it with user actions? (ie. user tries to interact with block preview, nothing happens)


// Ensure the block preview class names are merged with the component's class name.
expect( container.firstChild.className ).toBe(
'test-container-classname block-editor-block-preview__live-content components-disabled'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export default function BlockToolsBackCompat( { children } ) {
deprecated( 'wp.components.Popover.Slot name="block-toolbar"', {
alternative: 'wp.blockEditor.BlockTools',
since: '5.8',
version: '6.3',
} );

return (
Expand Down
8 changes: 1 addition & 7 deletions packages/block-library/src/comments/edit/placeholder.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import { store as blockEditorStore } from '@wordpress/block-editor';
import { __, sprintf } from '@wordpress/i18n';
import { useSelect } from '@wordpress/data';
import { useEntityProp } from '@wordpress/core-data';
import { useDisabled } from '@wordpress/compose';

/**
* Internal dependencies
Expand All @@ -22,13 +21,8 @@ export default function PostCommentsPlaceholder( { postType, postId } ) {
.__experimentalDiscussionSettings
);

const disabledRef = useDisabled();

return (
<div
className="wp-block-comments__legacy-placeholder"
ref={ disabledRef }
>
<div className="wp-block-comments__legacy-placeholder" inert="true">
Copy link
Contributor

Choose a reason for hiding this comment

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

Why exactly are we disabling this?

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 is just mirrors what existed before the PR. Basically we had a useDisabled hook that poly fills what "inert" does. So I'm not sure we changed anything in behavior here. I believe these "inert" within components are there because these blocks are "interactive", people can interact with them in the frontend but they are not meant to be interacted with in the backend (or navigated within). I'm not entirely sure that we really want to keep this visible for screen reader users, they are just mean to offer a visual representation of what the block will look like in the frontend but for interactions purposes, the intent is to hide these entirely.

If we really want to leave a "trace", we may add a label maybe or something like that?

Copy link
Contributor

Choose a reason for hiding this comment

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

@youknowriad That's the problem. We are giving sighted people information keyboard/screen reader users don't have access to. If the content on the back-end is really not important, let's add display: none; to it so it is inaccessible to everyone. Additional context make sense?

Thanks.

Copy link
Contributor Author

@youknowriad youknowriad Sep 11, 2023

Choose a reason for hiding this comment

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

I think I see what you're coming from but the reality is that the editor is also meant to be WYSIWYG and that concept doesn't really translate into screen readers. Sighted users will see something different visually between the backend and frontend and consider this a visual bug.

In terms of interactions, I agree, both sighted and screen reader users should have the exact same thing but the visuals are important for sighted users.

Would it help if we add a label or something saying "Visual Preview of block/whatever we're previewing" or something like that?


Also, this is independent of the use of inert, it has always been this way in the editor.

Copy link
Contributor

Choose a reason for hiding this comment

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

@youknowriad

Also, this is independent of the use of inert, it has always been this way in the editor.

That's got to change but I guess it can go into an issue for further discussion.

Labels don't help here because the preview itself is still not communicated equally.

Why is it not possible to disable interactions with blocks in an accessible way? #54322 accomplishes just that. People with vision can't interact with the block and screen readers can still see the block content but cannot interact with the block.

We've got to quit caring so much about this front-end vs. back-end. Honestly, do you think anyone at all would notice the usage of inert vs. aria-disabled with event listeners to disable a control? Absolutely not. It just makes it more work for developers and that I fear is the real problem here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Below example shows how this can still remain accessible but preventing the form from actually working.

<html>
	<head>
		<title>Test</title>
	</head>
	<body>
		<h1>With inert</h1>
		<form class="form1" action="https://www.google.com" method="GET" inert>
			<input type="search" value="" name="search1" aria-label="Search 1" placeholder="Search 1" />
			<input type="submit" value="Search 1" />
		</form>
		<br />
		<h1>Accessible way but more work</h1>
		<form class="form2" action="https://www.google.com" method="GET">
			<input type="search" value="" name="search2" aria-label="Search 2" placeholder="Search 2" aria-disabled="true" />
			<input type="submit" value="Search 2" aria-disabled="true" />
		</form>
		<script type="text/javascript">
			const form2 = document.querySelector('.form2');
			form2.addEventListener('submit', function(e) {
				e.preventDefault();
			} );
		</script>
	</body>
</html>

Copy link
Member

Choose a reason for hiding this comment

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

I've created an issue where can record discussions and debate alternatives:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that I think about it, all of our usage of "inert" can be seen as "screenshots". In that sense, I think maybe the solution going forward is to use a role="img" on these elements with a label. When you show a preview of a block or a template, it's just like an image.

<h3>
{
/* translators: %s: Post title. */
Expand Down
5 changes: 2 additions & 3 deletions packages/block-library/src/post-comments-form/form.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,17 @@ import {
__experimentalGetElementClassName,
} from '@wordpress/block-editor';
import { Button } from '@wordpress/components';
import { useDisabled, useInstanceId } from '@wordpress/compose';
import { useInstanceId } from '@wordpress/compose';
import { useEntityProp, store as coreStore } from '@wordpress/core-data';
import { useSelect } from '@wordpress/data';

const CommentsFormPlaceholder = () => {
const disabledFormRef = useDisabled();
const instanceId = useInstanceId( CommentsFormPlaceholder );

return (
<div className="comment-respond">
<h3 className="comment-reply-title">{ __( 'Leave a Reply' ) }</h3>
<form noValidate className="comment-form" ref={ disabledFormRef }>
<form noValidate className="comment-form" inert="true">
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
<p>
<label htmlFor={ `comment-${ instanceId }` }>
{ __( 'Comment' ) }
Expand Down
4 changes: 1 addition & 3 deletions packages/block-library/src/table-of-contents/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import {
ToolbarButton,
ToolbarGroup,
} from '@wordpress/components';
import { useDisabled } from '@wordpress/compose';
import { useDispatch, useSelect } from '@wordpress/data';
import { __unstableStripHTML as stripHTML } from '@wordpress/dom';
import { renderToString, useEffect } from '@wordpress/element';
Expand Down Expand Up @@ -55,7 +54,6 @@ export default function TableOfContentsEdit( {
setAttributes,
} ) {
const blockProps = useBlockProps();
const disabledRef = useDisabled();

const canInsertList = useSelect(
( select ) => {
Expand Down Expand Up @@ -295,7 +293,7 @@ export default function TableOfContentsEdit( {
return (
<>
<nav { ...blockProps }>
<ol ref={ disabledRef }>
<ol inert="true">
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
<TableOfContentsList nestedHeadingList={ headingTree } />
</ol>
</nav>
Expand Down
44 changes: 11 additions & 33 deletions packages/components/src/disabled/index.tsx
Original file line number Diff line number Diff line change
@@ -1,13 +1,7 @@
/**
* External dependencies
*/
import type { HTMLProps } from 'react';

/**
* WordPress dependencies
*/
import { useDisabled } from '@wordpress/compose';
import { createContext, forwardRef } from '@wordpress/element';
import { createContext } from '@wordpress/element';

/**
* Internal dependencies
Expand All @@ -20,15 +14,6 @@ import { useCx } from '../utils';
const Context = createContext< boolean >( false );
const { Consumer, Provider } = Context;

// Extracting this ContentWrapper component in order to make it more explicit
// the same 'ContentWrapper' component is needed so that React can reconcile
// the dom correctly when switching between disabled/non-disabled (instead
// of thrashing the previous DOM and therefore losing the form fields values).
const ContentWrapper = forwardRef<
HTMLDivElement,
HTMLProps< HTMLDivElement >
>( ( props, ref ) => <div { ...props } ref={ ref } /> );

/**
* `Disabled` is a component which disables descendant tabbable elements and prevents pointer interaction.
*
Expand Down Expand Up @@ -65,29 +50,22 @@ function Disabled( {
isDisabled = true,
...props
}: WordPressComponentProps< DisabledProps, 'div' > ) {
const ref = useDisabled();
const cx = useCx();
if ( ! isDisabled ) {
return (
<Provider value={ false }>
<ContentWrapper>{ children }</ContentWrapper>
</Provider>
);
}

return (
<Provider value={ true }>
<ContentWrapper
ref={ ref }
className={ cx(
disabledStyles,
className,
'components-disabled'
) }
<Provider value={ isDisabled }>
<div
// @ts-ignore Reason: inert is a recent HTML attribute
inert={ isDisabled ? 'true' : undefined }
className={
isDisabled
? cx( disabledStyles, className, 'components-disabled' )
: undefined
}
{ ...props }
>
{ children }
</ContentWrapper>
</div>
</Provider>
);
}
Expand Down
Loading