Skip to content

Improvements to use-focus-first-element and utils (dom) #39461

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

Merged
talldan merged 15 commits into from
Mar 28, 2022
Merged

Improvements to use-focus-first-element and utils (dom) #39461

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
fac5b03
Improve the way focus first element works for non-content editable bl…
alexstine Mar 15, 2022
33bdfc5
Some code fixes.
alexstine Mar 15, 2022
21b47b1
Merge branch 'trunk' of github.com:wordpress/gutenberg into fix/block…
alexstine Mar 15, 2022
e454097
Break off isFormElement function in to utils.js for reuse.
alexstine Mar 15, 2022
cbd79e6
Fix element class detection error.
alexstine Mar 15, 2022
4ffec9c
Merge branch 'trunk' of github.com:wordpress/gutenberg into improve/f…
alexstine Mar 15, 2022
14fd4ad
Try to fix test failures.
alexstine Mar 15, 2022
42ddba7
See if tests pass.
alexstine Mar 15, 2022
ee84ab2
Merge branch 'trunk' of github.com:wordpress/gutenberg into improve/f…
alexstine Mar 15, 2022
cd9df4a
Merge branch 'trunk' of github.com:wordpress/gutenberg into improve/f…
alexstine Mar 16, 2022
bb7a855
Now that tests are working again, try again with some fixes. JSDoc im…
alexstine Mar 16, 2022
fc14a9b
Merge branch 'trunk' of github.com:wordpress/gutenberg into improve/f…
alexstine Mar 16, 2022
c5dae59
Fix failing E2E tests.
alexstine Mar 17, 2022
6ed767d
Move isFormElement to wordpress/dom.
alexstine Mar 17, 2022
007d826
Code cleanup.
alexstine Mar 18, 2022
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
Prev Previous commit
Next Next commit
Now that tests are working again, try again with some fixes. JSDoc im… 
…provements. Type improvements. Add back the ability to skip block specific inserter. Add custom option to toggle this ability.
  • Loading branch information
@alexstine
alexstine committed Mar 16, 2022
commit bb7a855d2fc4c457d02f51fc5184b30a21a7e2fb
9 changes: 8 additions & 1 deletion packages/block-editor/src/components/block-list/use-block-props/use-focus-first-element.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,10 +100,17 @@ export function useFocusFirstElement( clientId ) {
// Check to see if element is focussable before a generic caret insert.
if ( ! target.getAttribute( 'contenteditable' ) ) {
const focusElement = focus.tabbable.findNext( target );
const rootBlockOptions = {
skipBlockInserter: true,
};
// Make sure focusElement is valid, contained in the same block, and a form field.
if (
focusElement &&
isInsideRootBlock( ref.current, focusElement ) &&
isInsideRootBlock(
ref.current,
focusElement,
rootBlockOptions
) &&
isFormElement( focusElement )
) {
focusElement.focus();
Expand Down
4 changes: 1 addition & 3 deletions packages/block-editor/src/components/writing-flow/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
/** @typedef {import('@wordpress/element').RefObject} RefObject */

/**
*
* Detects if element is a form element.
*
* @param {RefObject} element The element to check.
* @param {Element} element The element to check.
*
* @return {boolean} True if form element and false otherwise.
Copy link
Contributor Author

@alexstine alexstine Mar 15, 2022

Choose a reason for hiding this comment

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

I am also fairly certain this returns true/false.

*/
Expand Down
20 changes: 16 additions & 4 deletions packages/block-editor/src/utils/dom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
const BLOCK_SELECTOR = '.block-editor-block-list__block';
const APPENDER_SELECTOR = '.block-list-appender';
//const BLOCK_APPENDER_CLASS = '.block-editor-button-block-appender';
const BLOCK_APPENDER_CLASS = '.block-editor-button-block-appender';

/**
* Returns true if two elements are contained within the same block.
Expand All @@ -18,16 +18,28 @@ export function isInSameBlock( a, b ) {
* Returns true if an element is considered part of the block and not its inner
* blocks or appender.
*
* @param {Element} blockElement Block container element.
* @param {Element} element Element.
* @param {Element} blockElement Block container element.
* @param {Element} element Element.
* @param {{ skipBlockInserter: boolean }} options Custom options to set.
*
* @return {boolean} Whether an element is considered part of the block and not
* its inner blocks or appender.
*/
export function isInsideRootBlock( blockElement, element ) {
export function isInsideRootBlock(
blockElement,
element,
options = { skipBlockInserter: false }
) {
Copy link
Contributor

@talldan talldan Mar 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm having a hard time understanding the isInsideRootBlock function (not your fault), but to me the description ('Returns true if an element is considered part of the block and not its inner blocks or appender') makes it sound like it should already return false for the group block's button block appender.

This probably wasn't updated when the button appender was introduced and is now buggy. I think it'd be fine to remove the options param and instead make it return false always for the button appender.

Maybe the implementation could be something like:

export function isInsideRootBlock( blockElement, element ) {
	const parentBlock = element.closest(
		[ BLOCK_SELECTOR, APPENDER_SELECTOR, BUTTON_APPENDER_SELECTOR ].join( ',' )
	);
	return parentBlock === blockElement;
}

To be honest, using the appender classnames is probably not very reliable, as blocks can use custom appenders that may not even have a classname.

Copy link
Contributor Author

@alexstine alexstine Mar 18, 2022

Choose a reason for hiding this comment

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

To be honest, using the appender classnames is probably not very reliable, as blocks can use custom appenders that may not even have a classname.

The problem is, very little thought was put in to inner blocks from an accessibility perspective. Having all these child divs and elements inside parent block elements has made it a nightmare to work with when it comes down to controlling what screen readers see. I would have much rather represented inner blocks with CSS positioning than quite literally in the DOM. Even adding things such as aria-description can be super problematic because the descriptions are read from inner element to outer element instead of just for the current element. Some type of conditional checking would have also been nice that way I could detect if I was in parent block vs. child block, but as you can see, it's all kind of hacking at the end of the day.

As far as the classes are concerned, it goes back to my previous point. I have no idea how to actually get around it. I don't think it's in the store anywhere and I don't think any custom reliable attributes are outputted. My goal was to make this work with blocks we control and then figure out the rest later.

I'm getting at, previous design choices have made things really tough for us now.

As far as what the function should do.

  1. Compare the block wrapper and element to see if it is in the block.
  2. Ensure the element is not in an inner block.
  3. Return false if appender. The reason being is the group block is already technically an inner block but if I excluded inner blocks even if it was the parent block, focus would not work. Take the columns block for example. You have columns, column, and inner blocks of column. If I select say a image block in column 1, this is technically an inner block that needs to be focused.

Sorry this is so hard to explain. 😞 We can't change what happened before, just need to improve it going forward. 👍

Copy link
Contributor Author

@alexstine alexstine Mar 18, 2022

Choose a reason for hiding this comment

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

@talldan I was unsure if your suggestion would work as I almost took that approach at first. Seems to work fine. Cleaned up the code and committed.

I also didn't know if this was used elsewhere so I decided the option approach was safer. If you think it's fine, I removed it. Can always revert that commit if someone else feels differently.

I don't have enough history to know if it's safe or not... 🙂

const parentBlock = element.closest(
[ BLOCK_SELECTOR, APPENDER_SELECTOR ].join( ',' )
);
// Skip inserter for blocks such as group block.
if (
options.skipBlockInserter &&
parentBlock?.classList.contains( BLOCK_APPENDER_CLASS )
) {
return false;
}
return parentBlock === blockElement;
}

Expand Down