Improvements to use-focus-first-element and utils (dom) #39461
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…-quick-remove-after-insert-support
alexstine
added
[Type] Enhancement
|
Size Change: +2.32 kB (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
alexstine
commented
Mar 15, 2022
| ! focusElement.classList.contains( | ||
| 'block-editor-button-block-appender' | ||
| ) | ||
| isInsideRootBlock( ref.current, focusElement ) && |
There was a problem hiding this comment.
This looks so much cleaner to me.
| * | ||
| * Detects if element is a form element. | ||
| * | ||
| * @param {RefObject} element The element to check. |
There was a problem hiding this comment.
I am pretty sure this type is correct.
| * | ||
| * @param {RefObject} element The element to check. | ||
| * | ||
| * @return {boolean} True if form element and false otherwise. |
There was a problem hiding this comment.
I am also fairly certain this returns true/false.
| return parentBlock === blockElement; | ||
| return ( | ||
| parentBlock === blockElement && | ||
| ! element.classList.contains( BLOCK_APPENDER_CLASS ) |
There was a problem hiding this comment.
Added this here to not focus block appender that is inside blocks such as group block. This helps us stay within the block itself.
…ocus-first-element
…ocus-first-element
|
Back to draft until the tests get fixed and I can continue. |
…ocus-first-element
…provements. Type improvements. Add back the ability to skip block specific inserter. Add custom option to toggle this ability.
talldan
reviewed
Mar 17, 2022
talldan
reviewed
Mar 18, 2022
| blockElement, | ||
| element, | ||
| options = { skipBlockInserter: false } | ||
| ) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
- Compare the block wrapper and element to see if it is in the block.
- Ensure the element is not in an inner block.
- 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. 👍
There was a problem hiding this comment.
@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 focusElement = focus.tabbable.findNext( target ); | ||
| // Make sure focusElement is valid, form field, and within the current target element. | ||
| // Ensure is not block inserter trigger, don't want to focus that in the event of the group block which doesn't contain any other focussable elements. | ||
| if ( ! ref.current.getAttribute( 'contenteditable' ) ) { |
There was a problem hiding this comment.
Out of interest, why make the change here from target to ref.current?
There was a problem hiding this comment.
@talldan E2E tests caught a bug in the embed block and I confirmed it. Seems like using ref.current is more stable in this case.
It would focus the button of the embed block vs. the field to enter a URL. Odd issue but easy enough fix. Not sure if it was the IFrame or what...
talldan
approved these changes
Mar 28, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
This PR comes with some follow-up improvements from #37934.
Why?
Good to keep our code clean.
How?
Code is cleaned up.
Testing Instructions
Screenshots or screencast