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

Don't limit block initial focus to input fields #12648

Open
afercia opened this issue Dec 6, 2018 · 5 comments
Open

Don't limit block initial focus to input fields #12648

afercia opened this issue Dec 6, 2018 · 5 comments
Labels
[Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Bug An existing feature does not function as intended

Comments

@afercia
Copy link
Contributor

afercia commented Dec 6, 2018

When inserting a block, if the block has input fields then the initial focus is set by default on the first input field, otherwise it's set on the block wrapper.

See focusTabbable() in BlockListBlock:

// Find all tabbables within node.
const textInputs = focus.tabbable
.find( this.node )
.filter( isTextField )
// Exclude inner blocks
.filter( ( node ) => ! ignoreInnerBlocks || isInsideRootBlock( this.node, node ) );
// If reversed (e.g. merge via backspace), use the last in the set of
// tabbables.
const isReverse = -1 === initialPosition;
const target = ( isReverse ? last : first )( textInputs );
if ( ! target ) {
this.wrapperNode.focus();
return;
}
target.focus();

Note: "input fields" in this context are only:

  • <input> elements that support the text selection API
  • <textarea> elements
  • elements with contenteditable

See isTextField()

export function isTextField( element ) {
try {
const { nodeName, selectionStart, contentEditable } = element;
return (
( nodeName === 'INPUT' && selectionStart !== null ) ||
( nodeName === 'TEXTAREA' ) ||
contentEditable === 'true'
);
} catch ( error ) {
// Safari throws an exception when trying to get `selectionStart`
// on non-text <input> elements (which, understandably, don't
// have the text selection API). We catch this via a try/catch
// block, as opposed to a more explicit check of the element's
// input types, because of Safari's non-standard behavior. This
// also means we don't have to worry about the list of input
// types that support `selectionStart` changing as the HTML spec
// evolves over time.
return false;
}
}

However, the current implementation assumes the first tabbable element is always an "input field". This is true for the Gutenberg default blocks but it's not guaranteed to be true for custom blocks.

For example, what if the first tabbable is a button element?

screenshot 2018-12-06 at 15 39 58

Initial focus is set on the "input field" after the button, thus skipping an important part of the UI.

Same if the first tabbable element is, for example, one of:

  • a select element
  • a checkbox element
  • a radio button element
  • etc.: any tabbable element that is not an "input field" as defined by isTextField()

In all these cases, initial focus will be set on the wrong element.

I'd suggest to consider to remove the filtering by isTextField() to start with. I don't see a reason for it to be there, unless I'm missing something.

Also to consider: a way to give developers more control on initial focus. This could certainly be addressed separately.

@afercia afercia added [Type] Bug An existing feature does not function as intended [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). labels Dec 6, 2018
@afercia
Copy link
Contributor Author

afercia commented Dec 7, 2018

In short: removing the line .filter( isTextField ) would be nice 🙂

@mtias mtias added this to the WordPress 5.0.x Follow Ups milestone Dec 7, 2018
@mtias mtias added the [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... label Dec 7, 2018
@afercia
Copy link
Contributor Author

afercia commented Jan 16, 2019

Tested a bit with nested blocks and seems to me things are not so simple.

When there are inner blocks, the logic should be adjusted, as right now inner blocks are:

  1. taken into account (and focused) when the block mounts
  2. ignored when the block updates

Imagine a block based on inner blocks. The block adds by default two editable fields, for example:

screenshot 2019-01-16 at 17 43 15

Because of 1, removing .filter( isTextField ) will focus the inner blocks container, which is the first focusable thing within the block. Instead, we'd want the first focusable form control / contenteditable to be focused.

Also, when adding a second Q/A pair, I'm seeing focus goes to the answer. Expected: focus to go to the first form field / editable i.e. the question.

@afercia
Copy link
Contributor Author

afercia commented Feb 25, 2019

For the inner blocks, see the original change in #10545

Fix #9212 which is causing the inner blocks to be selected when we select the parent.

@afercia
Copy link
Contributor Author

afercia commented Feb 25, 2019

Looking a bit more into this, seems the case of nested block is not so simple.

Basically, when inserting a block that initially inerts also some nested blocks, focusTabbable() runs when both the parent block and the child block mount.

This leads to unexpected results that actually depend on which elements are initially inside the parent and the child. For example, when inserting a block that has some default inner blocks:

Example 1:

  • parent with no input fields
  • child with 2 input fields
  • the first input of the child gets initial focus: this looks right because it's the first tabbable displayed in the blocks

Example 2:

  • parent with 1 input field
  • child with 2 input fields
  • the first input of the child gets initial focus: this looks wrong: the input of the parent should get focus

Also, we'd need to remove .filter( isTextField ) to get all the tabbables but this would take into account also the entire UI of the children.

The desired behavior would be:

  • get the tabbables only after the parent and all the initial inner blocks have mounted
  • exclude all the blocks UI (appender button, movers, etc.)
  • get all the tabbables within the block, not limited to input fields
  • focus the first one
  • when adding new nested blocks the parent should be ignored and focus should go to the first tabbable of the new nested block
  • as a general fallback, focus the last inserted block wrapper

@florianbrinkmann
Copy link

Would be great if that could be improved.

I have the situation that I use multiple TextControl fields in a custom block, followed by a InnerBlocks component that contains three blocks that contain InnerBlocks again. On inserting the block, the focus jumps to the first block in the last of the three inner block blocks, leading to the situation that the user needs to scroll up to see the top of the block (it is an address block that should give the user the option to add multiple website URLs, email addresses and phone numbers).

@Mamaduka Mamaduka removed this from the WordPress 5.x milestone May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

5 participants