Add mechanism to avoid forced child selection on blocks with templates. #10696

jorgefilipecosta · 2018-10-17T16:58:45Z

Description

Currently, if a block contains a template when we insert that block the block that gets focused is one of the child blocks specified in the template. We don't offer a way to disable this behavior.

This current behavior makes sense in most cases (e.g: columns) but for some cases,
we may want to keep the focus on the parent. E.g. In PR #9416 @afercia pointed some a11y concerns in automatically selecting the child block on the InnerBlocks area.

Blocks may have their own edition area and an InnerBlocks are at the end. Automatically selecting a block in the InnerBlocks area, has some accessible concerns has the parent block edition area may be unnoticeable for screen reader users.

This PR adds a flag to the InnerBlocks component that allows blocks to disable the behavior of automatically selecting the children. In order for this flag to be possible another flag was added in insertBlock(s) action that allows for blocks to be inserted without a selection update happening.

How has this been tested?

I created some tests blocks available in https://gist.github.com/jorgefilipecosta/a99dcbaedd35ec88c37b142679bc0dc1.
I checked that when I insert 'Test No Template Selection' the parent block is the block selected after the insert.
I checked that when I insert 'Test Template Selection' the first child is the block that gets inserted.
Columns continue to work exactly as before (the first paragraph gets selected) when a column is inserted.

youknowriad · 2018-10-17T17:04:41Z

packages/editor/src/store/reducer.js

-				isMultiSelecting: false,
-			};
+		case 'INSERT_BLOCKS': {
+			if ( action.updateSelection ) {


For me, this is not what's causing the innerBlocks to be selected, it's the focusTabbable function inside block.js. This on ensures the inserted block (parent) is selected.

But the children are inserted after the parent, so without this changes, the selection gets updated when the child blocks are inserted.

Oh makes sense. Seems like a decent approach.

Hi @youknowriad thank you for looking into this PR. Do you think it is ready to merge?

chrisvanpatten · 2018-10-19T12:19:48Z

OH MY GOSH this is so exciting. 😁🤗

In my custom Columns block we dispatch an insertBlock action to insert blocks from our own UI and it works but causes the paragraph inside the column to get selected. Being able to disable this behavior would be just the most incredible thing.

💟

youknowriad

Looks good to me overall. @aduth Thoughts on this new argument?

youknowriad · 2018-10-25T11:01:01Z

docs/data/data-core-editor.md

@@ -1514,7 +1514,10 @@ inserted, optionally at a specific index respective a root block list.
 * block: Block object to insert.
 * index: Index at which block should be inserted.
 * rootClientId: Optional root client ID of block list on which
-                              to insert.
+                                   to insert.


Maybe we should just not add a line break at all for arguments with long lines (to avoid messing with the markdown output)

youknowriad · 2018-10-25T11:01:51Z

packages/editor/src/components/inner-blocks/README.md

@@ -92,6 +92,13 @@ const TEMPLATE = [ [ 'core/columns', {}, [

 The previous example creates an InnerBlocks area containing two columns one with an image and the other with a paragraph.

+### `templateInsertUpdatesSelection`


I wonder if this is really needed or if it should always be "false"?

I think it needs to be conditional. If you're building a nested block with a flow optimised for writing you would want the selection to be updated for keyboard use.

Making it always false would also be a change in the current behaviour, which updates the selection automatically.

I agree for some blocks it may make sense to use a different setting.

Would this be clearer if it's named: updateSelectionOnTemplateInsert ?

afercia · 2018-11-06T18:22:06Z

Right now, the "Media & Text" block accessibility is less than ideal, see #9416 (comment)

In that PR it was mentioned there are higher order limitations with templates / nested blocks that, at that time, prevented to fully address the accessibility concerns. This PR is meant to solve one of those issues. However, there's no milestone set and the "Media & Text" block is still not accessible. We're now at less than a week from the scheduled Release Candidate.

Are there any plans to help this PR move on? /Cc @mtias @youknowriad @jorgefilipecosta @tofumatt

Adding the label "bug" as "Media & Text" is not accessible.

jorgefilipecosta · 2018-11-06T19:52:36Z

I rebased this PR and retested and things looked fine, it should be ready, I added it to 4.3 milestone.

chrisvanpatten

I'm not comfortable commenting on the specific implementation, but I tested the feature with our custom columns block and it works beautifully. The user experience is dramatically improved by this.

Hope to see it make it into 4.3 :)

youknowriad

LGTM 👍

…rnmobile/fix-merge-content-not-refreshed-UI * 'master' of https://github.com/WordPress/gutenberg: Fix the isBeingScheduled Selector. (#11572) Slot/Fill pattern with Toolbar #199 (#11115) Add mechanism to avoid forced child selection on blocks with templates. (#10696) Allow a block to disable being converted into a reusable block; Fix: Column block (#11550)

jorgefilipecosta added [Type] Enhancement A suggestion for improvement. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). labels Oct 17, 2018

youknowriad reviewed Oct 17, 2018

View reviewed changes

jorgefilipecosta mentioned this pull request Oct 17, 2018

Add "Text & Image" Block #9416

Merged

jorgefilipecosta force-pushed the add/mechanism-to-avoid-force-child-selection-on-blocks-with-templates branch 5 times, most recently from c21c801 to a54d5b7 Compare October 24, 2018 19:26

youknowriad reviewed Oct 25, 2018

View reviewed changes

afercia added [Type] Bug An existing feature does not function as intended and removed [Type] Enhancement A suggestion for improvement. labels Nov 6, 2018

jorgefilipecosta force-pushed the add/mechanism-to-avoid-force-child-selection-on-blocks-with-templates branch from a54d5b7 to 7ec29d1 Compare November 6, 2018 19:47

jorgefilipecosta added this to the 4.3 milestone Nov 6, 2018

chrisvanpatten approved these changes Nov 6, 2018

View reviewed changes

Add mechanism to avoid forced child selection on blocks with templates.

696f26c

jorgefilipecosta force-pushed the add/mechanism-to-avoid-force-child-selection-on-blocks-with-templates branch from 7ec29d1 to 696f26c Compare November 6, 2018 22:03

youknowriad approved these changes Nov 7, 2018

View reviewed changes

jorgefilipecosta merged commit 16a718a into master Nov 7, 2018

jorgefilipecosta deleted the add/mechanism-to-avoid-force-child-selection-on-blocks-with-templates branch November 7, 2018 12:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add mechanism to avoid forced child selection on blocks with templates. #10696

Add mechanism to avoid forced child selection on blocks with templates. #10696

jorgefilipecosta commented Oct 17, 2018 •

edited

Loading

youknowriad Oct 17, 2018

jorgefilipecosta Oct 17, 2018 •

edited

Loading

youknowriad Oct 17, 2018

jorgefilipecosta Oct 24, 2018

chrisvanpatten commented Oct 19, 2018

youknowriad left a comment

youknowriad Oct 25, 2018

youknowriad Oct 25, 2018

chrisvanpatten Oct 25, 2018

jorgefilipecosta Nov 6, 2018

youknowriad Nov 7, 2018

afercia commented Nov 6, 2018

jorgefilipecosta commented Nov 6, 2018 •

edited

Loading

chrisvanpatten left a comment

youknowriad left a comment

		@@ -92,6 +92,13 @@ const TEMPLATE = [ [ 'core/columns', {}, [

		The previous example creates an InnerBlocks area containing two columns one with an image and the other with a paragraph.

		### `templateInsertUpdatesSelection`

Add mechanism to avoid forced child selection on blocks with templates. #10696

Add mechanism to avoid forced child selection on blocks with templates. #10696

Conversation

jorgefilipecosta commented Oct 17, 2018 • edited Loading

Description

How has this been tested?

Choose a reason for hiding this comment

jorgefilipecosta Oct 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisvanpatten commented Oct 19, 2018

youknowriad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afercia commented Nov 6, 2018

jorgefilipecosta commented Nov 6, 2018 • edited Loading

chrisvanpatten left a comment

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

jorgefilipecosta commented Oct 17, 2018 •

edited

Loading

jorgefilipecosta Oct 17, 2018 •

edited

Loading

jorgefilipecosta commented Nov 6, 2018 •

edited

Loading