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

Writing flow: allow select all from empty selection #33446

Merged
merged 6 commits into from
Jul 15, 2021
Merged

Writing flow: allow select all from empty selection #33446

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7dd849b
Writing flow: allow select all from empty selection
ellatrix Jul 15, 2021
dd7f1f6
Simpler tax index
ellatrix Jul 15, 2021
894848d
Forward ref
ellatrix Jul 15, 2021
c8b3a2d
Fix e2e test: enter nav mode when tabbing inside the canvas and no bl…
ellatrix Jul 15, 2021
e72dabb
Add e2e test
ellatrix Jul 15, 2021
ee244e9
Writing flow: prevent default on Esc keydown for nav mode
ellatrix Jul 15, 2021
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
33 changes: 23 additions & 10 deletions packages/block-editor/src/components/writing-flow/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
/**
* External dependencies
*/
import classNames from 'classnames';

/**
* WordPress dependencies
*/
import { useSelect } from '@wordpress/data';
import { __ } from '@wordpress/i18n';
import { useMergeRefs } from '@wordpress/compose';
import { forwardRef } from '@wordpress/element';

/**
* Internal dependencies
Expand All @@ -14,14 +20,7 @@ import useArrowNav from './use-arrow-nav';
import useSelectAll from './use-select-all';
import { store as blockEditorStore } from '../../store';

/**
* Handles selection and navigation across blocks. This component should be
* wrapped around BlockList.
*
* @param {Object} props Component properties.
* @param {WPElement} props.children Children to be rendered.
*/
export default function WritingFlow( { children } ) {
function WritingFlow( { children, ...props }, forwardedRef ) {
const [ before, ref, after ] = useTabNav();
const hasMultiSelection = useSelect(
( select ) => select( blockEditorStore ).hasMultiSelection(),
Expand All @@ -31,14 +30,19 @@ export default function WritingFlow( { children } ) {
<>
{ before }
<div
{ ...props }
ref={ useMergeRefs( [
forwardedRef,
ref,
useMultiSelection(),
useSelectAll(),
useArrowNav(),
] ) }
className="block-editor-writing-flow"
tabIndex={ hasMultiSelection ? '0' : undefined }
className={ classNames(
props.className,
'block-editor-writing-flow'
) }
tabIndex={ -1 }
aria-label={
hasMultiSelection
? __( 'Multiple selected blocks' )
Expand All @@ -51,3 +55,12 @@ export default function WritingFlow( { children } ) {
</>
);
}

/**
* Handles selection and navigation across blocks. This component should be
* wrapped around BlockList.
*
* @param {Object} props Component properties.
* @param {WPElement} props.children Children to be rendered.
*/
export default forwardRef( WritingFlow );
8 changes: 2 additions & 6 deletions packages/block-editor/src/components/writing-flow/use-select-all.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,12 @@ export default function useSelectAll() {

return useRefEffect( ( node ) => {
function onKeyDown( event ) {
const selectedClientIds = getSelectedBlockClientIds();

if ( ! selectedClientIds.length ) {
return;
}

if ( ! isMatch( 'core/block-editor/select-all', event ) ) {
return;
}

const selectedClientIds = getSelectedBlockClientIds();

if (
selectedClientIds.length === 1 &&
! isEntirelySelected( event.target )
Expand Down
2 changes: 2 additions & 0 deletions packages/block-editor/src/components/writing-flow/use-tab-nav.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ export default function useTabNav() {
function onKeyDown( event ) {
if ( event.keyCode === ESCAPE && ! hasMultiSelection() ) {
event.stopPropagation();
event.preventDefault();
setNavigationMode( true );
return;
}
Expand All @@ -102,6 +103,7 @@ export default function useTabNav() {
const direction = isShift ? 'findPrevious' : 'findNext';

if ( ! hasMultiSelection() && ! getSelectedBlockClientId() ) {
setNavigationMode( true );
Copy link
Contributor

Choose a reason for hiding this comment

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

How is this related to this PR? What's the purpose of this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is needed to make e2e tests pass. We enable navigation mode when the user tabs into the writing flow focus capture, but with tab index set to -1 and when clearing block selection, focus will no longer move to the interface content element, but rather to the editor styles wrapper. When tabbing, it will not go through to focus capture to enable navigation mode, so we have to enable navigation mode when the user tabs in the content and there is no block selection.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think I follow here, why do we need to auto-enable navigation mode implicitly (not hitting "Escape").

If there's no block selected and we tab into the content, can't we just select the first one?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could, but that's not how the behaviour was. I want to preserve the current behaviour. This was discussed once with the some people from the accessibility team that if no block is selected, and you tab into the content, the editor should be in navigation mode to select a block. Supposedly it wouldn't make sense to decide for the user and pick the first one from which they then have to press Esc to pick another one. But all of this is a bit more opinionated, so for this PR I just want to make the tests pass and not change the current behaviour. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 I didn't know this was the current behavior, that's fine by me.

return;
}

Expand Down
12 changes: 12 additions & 0 deletions packages/e2e-tests/specs/editor/various/__snapshots__/multi-block-selection.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,18 @@ exports[`Multi-block selection should return original focus after failed multi s
<!-- /wp:paragraph -->"
`;

exports[`Multi-block selection should select all from empty selection 1`] = `
"<!-- wp:paragraph -->
<p>1</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph -->
<p>2</p>
<!-- /wp:paragraph -->"
`;

exports[`Multi-block selection should select all from empty selection 2`] = `""`;

exports[`Multi-block selection should set attributes for multiple paragraphs 1`] = `
"<!-- wp:paragraph {\\"align\\":\\"center\\"} -->
<p class=\\"has-text-align-center\\">1</p>
Expand Down
23 changes: 23 additions & 0 deletions packages/e2e-tests/specs/editor/various/multi-block-selection.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -632,4 +632,27 @@ describe( 'Multi-block selection', () => {
'[data-type="core/paragraph"].is-multi-selected'
);
} );

it( 'should select all from empty selection', async () => {
await clickBlockAppender();

await page.keyboard.type( '1' );
await page.keyboard.press( 'Enter' );
await page.keyboard.type( '2' );

// Confirm setup.
expect( await getEditedPostContent() ).toMatchSnapshot();

// Clear the selected block.
const paragraph = await page.$( '[data-type="core/paragraph"]' );
const box = await paragraph.boundingBox();
await page.mouse.click( box.x - 1, box.y );

await pressKeyWithModifier( 'primary', 'a' );

await page.keyboard.press( 'Backspace' );

// Expect both paragraphs to be deleted.
expect( await getEditedPostContent() ).toMatchSnapshot();
} );
} );
27 changes: 12 additions & 15 deletions packages/edit-post/src/components/visual-editor/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,13 +57,14 @@ function MaybeIframe( {
return (
<>
<EditorStyles styles={ styles } />
<div
<WritingFlow
ref={ contentRef }
className="editor-styles-wrapper"
style={ { flex: '1', ...style } }
tabIndex={ -1 }
>
{ children }
</div>
</WritingFlow>
</>
);
}
Expand All @@ -75,7 +76,7 @@ function MaybeIframe( {
contentRef={ contentRef }
style={ { width: '100%', height: '100%', display: 'block' } }
>
{ children }
<WritingFlow>{ children }</WritingFlow>
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 the future, writing flow behaviour will be set on the iframe body element.

</Iframe>
);
}
Expand Down Expand Up @@ -234,18 +235,14 @@ export default function VisualEditor( { styles } ) {
layout={ defaultLayout }
/>
) }
<WritingFlow>
{ ! isTemplateMode && (
<div className="edit-post-visual-editor__post-title-wrapper">
<PostTitle />
</div>
) }
<RecursionProvider>
<BlockList
__experimentalLayout={ layout }
/>
</RecursionProvider>
</WritingFlow>
{ ! isTemplateMode && (
<div className="edit-post-visual-editor__post-title-wrapper">
<PostTitle />
</div>
) }
<RecursionProvider>
<BlockList __experimentalLayout={ layout } />
</RecursionProvider>
</MaybeIframe>
</motion.div>
</motion.div>
Expand Down