-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Improvements to use-focus-first-element and utils (dom) #39461
Merged
Merged
Changes from all commits
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 33bdfc5
Some code fixes.
alexstine 21b47b1
Merge branch 'trunk' of github.com:wordpress/gutenberg into fix/block…
alexstine e454097
Break off isFormElement function in to utils.js for reuse.
alexstine cbd79e6
Fix element class detection error.
alexstine 4ffec9c
Merge branch 'trunk' of github.com:wordpress/gutenberg into improve/f…
alexstine 14fd4ad
Try to fix test failures.
alexstine 42ddba7
See if tests pass.
alexstine ee84ab2
Merge branch 'trunk' of github.com:wordpress/gutenberg into improve/f…
alexstine cd9df4a
Merge branch 'trunk' of github.com:wordpress/gutenberg into improve/f…
alexstine bb7a855
Now that tests are working again, try again with some fixes. JSDoc im…
alexstine fc14a9b
Merge branch 'trunk' of github.com:wordpress/gutenberg into improve/f…
alexstine c5dae59
Fix failing E2E tests.
alexstine 6ed767d
Move isFormElement to wordpress/dom.
alexstine 007d826
Code cleanup.
alexstine File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,12 @@ import { first, last } from 'lodash'; | |
* WordPress dependencies | ||
*/ | ||
import { useEffect, useRef } from '@wordpress/element'; | ||
import { focus, isTextField, placeCaretAtHorizontalEdge } from '@wordpress/dom'; | ||
import { | ||
focus, | ||
isFormElement, | ||
isTextField, | ||
placeCaretAtHorizontalEdge, | ||
} from '@wordpress/dom'; | ||
import { useSelect } from '@wordpress/data'; | ||
|
||
/** | ||
|
@@ -52,16 +57,6 @@ function useInitialPosition( clientId ) { | |
); | ||
} | ||
|
||
function isFormElement( element ) { | ||
const { tagName } = element; | ||
return ( | ||
tagName === 'INPUT' || | ||
tagName === 'BUTTON' || | ||
tagName === 'SELECT' || | ||
tagName === 'TEXTAREA' | ||
); | ||
} | ||
|
||
/** | ||
* Transitions focus to the block or inner tabbable when the block becomes | ||
* selected and an initial position is set. | ||
|
@@ -107,17 +102,13 @@ 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 ); | ||
// 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' ) ) { | ||
const focusElement = focus.tabbable.findNext( ref.current ); | ||
// Make sure focusElement is valid, contained in the same block, and a form field. | ||
if ( | ||
focusElement && | ||
isFormElement( focusElement ) && | ||
target.contains( focusElement ) && | ||
! focusElement.classList.contains( | ||
'block-editor-button-block-appender' | ||
) | ||
isInsideRootBlock( ref.current, focusElement ) && | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks so much cleaner to me. |
||
isFormElement( focusElement ) | ||
) { | ||
focusElement.focus(); | ||
return; | ||
|
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
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
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
/** | ||
* Internal dependencies | ||
*/ | ||
import isInputOrTextArea from './is-input-or-text-area'; | ||
|
||
/** | ||
* | ||
* Detects if element is a form element. | ||
* | ||
* @param {Element} element The element to check. | ||
* | ||
* @return {boolean} True if form element and false otherwise. | ||
*/ | ||
export default function isFormElement( element ) { | ||
const { tagName } = element; | ||
const checkForInputTextarea = isInputOrTextArea( element ); | ||
return ( | ||
checkForInputTextarea || tagName === 'BUTTON' || tagName === 'SELECT' | ||
); | ||
} |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of interest, why make the change here from
target
toref.current
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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...