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.

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

RichText: Do not focus the input unless user is in an editing context #61374

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from
Open

RichText: Do not focus the input unless user is in an editing context #61374

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3c08c2e
Remove hacky focus fix to prevent the canvas from taking focus.
alexstine May 3, 2024
80c2e63
Every time a user types, virtual text tree is created. This could cau…
alexstine May 3, 2024
c3a0213
Better code comment.
alexstine May 3, 2024
1976d51
Refresh and reviewer feedback.
alexstine Jun 4, 2024
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
91 changes: 40 additions & 51 deletions packages/block-library/src/navigation-link/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ function getMissingText( type ) {
* packages/block-library/src/navigation-submenu/edit.js
* Consider reuseing this components for both blocks.
*/
function Controls( { attributes, setAttributes, setIsLabelFieldFocused } ) {
function Controls( { attributes, setAttributes } ) {
const { label, url, description, title, rel } = attributes;
return (
<PanelBody title={ __( 'Settings' ) }>
Expand All @@ -171,8 +171,6 @@ function Controls( { attributes, setAttributes, setIsLabelFieldFocused } ) {
} }
label={ __( 'Text' ) }
autoComplete="off"
onFocus={ () => setIsLabelFieldFocused( true ) }
onBlur={ () => setIsLabelFieldFocused( false ) }
/>
<TextControl
__nextHasNoMarginBottom
Expand Down Expand Up @@ -264,10 +262,6 @@ export default function NavigationLinkEdit( {
const linkUIref = useRef();
const prevUrl = usePrevious( url );

// Change the label using inspector causes rich text to change focus on firefox.
// This is a workaround to keep the focus on the label field when label filed is focused we don't render the rich text.
const [ isLabelFieldFocused, setIsLabelFieldFocused ] = useState( false );

const {
isAtMaxNesting,
isTopLevelLink,
Expand Down Expand Up @@ -484,7 +478,6 @@ export default function NavigationLinkEdit( {
<Controls
attributes={ attributes }
setAttributes={ setAttributes }
setIsLabelFieldFocused={ setIsLabelFieldFocused }
/>
</InspectorControls>
<div { ...blockProps }>
Expand All @@ -499,51 +492,47 @@ export default function NavigationLinkEdit( {
</div>
) : (
<>
{ ! isInvalid &&
! isDraft &&
! isLabelFieldFocused && (
<>
<RichText
ref={ ref }
identifier="label"
className="wp-block-navigation-item__label"
value={ label }
onChange={ ( labelValue ) =>
setAttributes( {
label: labelValue,
} )
}
onMerge={ mergeBlocks }
onReplace={ onReplace }
__unstableOnSplitAtEnd={ () =>
insertBlocksAfter(
createBlock(
'core/navigation-link'
)
{ ! isInvalid && ! isDraft && (
<>
<RichText
ref={ ref }
identifier="label"
className="wp-block-navigation-item__label"
value={ label }
onChange={ ( labelValue ) =>
setAttributes( {
label: labelValue,
} )
}
onMerge={ mergeBlocks }
onReplace={ onReplace }
__unstableOnSplitAtEnd={ () =>
insertBlocksAfter(
createBlock(
'core/navigation-link'
)
}
aria-label={ __(
'Navigation link text'
) }
placeholder={ itemLabelPlaceholder }
withoutInteractiveFormatting
allowedFormats={ [
'core/bold',
'core/italic',
'core/image',
'core/strikethrough',
] }
/>
{ description && (
<span className="wp-block-navigation-item__description">
{ description }
</span>
)
}
aria-label={ __(
'Navigation link text'
) }
</>
) }
{ ( isInvalid ||
isDraft ||
isLabelFieldFocused ) && (
placeholder={ itemLabelPlaceholder }
withoutInteractiveFormatting
allowedFormats={ [
'core/bold',
'core/italic',
'core/image',
'core/strikethrough',
] }
/>
{ description && (
<span className="wp-block-navigation-item__description">
{ description }
</span>
) }
</>
) }
{ ( isInvalid || isDraft ) && (
<div className="wp-block-navigation-link__placeholder-text wp-block-navigation-link__label">
<Tooltip text={ tooltipText }>
<span
Expand Down
9 changes: 8 additions & 1 deletion packages/rich-text/src/to-dom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,10 @@ export function applySelection( { startPath, endPath }, current ) {
range.setEnd( endContainer, endOffset );

const { activeElement } = ownerDocument;
const { frameElement } = activeElement.ownerDocument.defaultView;
const isInsideFrame = frameElement
? frameElement === frameElement.ownerDocument.activeElement
: true;

if ( selection.rangeCount > 0 ) {
// If the to be added range and the live range are the same, there's no
Expand All @@ -287,7 +291,10 @@ export function applySelection( { startPath, endPath }, current ) {
// This function is not intended to cause a shift in focus. Since the above
// selection manipulations may shift focus, ensure that focus is restored to
// its previous state.
if ( activeElement !== ownerDocument.activeElement ) {
// Do not take focus if user is not in iFrame editing canvas.
//
// See: https://github.com/WordPress/gutenberg/issues/61315
if ( isInsideFrame && activeElement !== ownerDocument.activeElement ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We need the activeElement.focus() on line 304 to run for Firefox to move focus after a selection change.

When that change comes from the toolbar (indent/outdent), we want the cursor to get applied to the list item and take focus. When a change comes from the sidebar, we don't want to move focus to the canvas (like a navigation label text change). I can't find a way to differentiate between these two situations. Fixing one breaks the other.

@ellatrix, do you have any insights?

Copy link
Member

@ellatrix ellatrix Jun 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm failing to understand what problem this modification to rich text solves. I remove this change, tried to edit the text in the inspector for a navigation link, and focus seems to remain there. How do I reproduce the problem without this change to rich text?

These lines in rich text are only meant to maintain focus only if the active element right before changing the range is different after changing the range.

Copy link
Contributor

Choose a reason for hiding this comment

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

On Trunk in Chrome:

  • In Chrome
  • Go to a navigation item
  • Edit it in the canvas (add some letters)
  • Tab to the sidebar
  • Continue tabbing and you'll end up in a focus loop
Screen.Recording.2024-06-28.at.9.25.15.AM.mov

On this PR in Firefox

  • In Firefox
  • Add a list block
  • Type something
  • Add another list item
  • Move to the list item toolbar
  • Indent the list item using the Indent button
  • Focus is lost. The active focus should be moved to the list item for typing.
Screen.Recording.2024-06-28.at.9.29.14.AM.mov

This PR without the isInsideFrame check in Firefox

  • In Firefox
  • Go to a navigation item
  • Edit it in the canvas (add some letters)
  • Go to the block settings sidebar
  • Type in the label field
  • Focus will get stolen back to the canvas after one keypress
Screen.Recording.2024-06-28.at.9.30.49.AM.mov

Copy link
Contributor

Choose a reason for hiding this comment

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

@ellatrix Did Jerry's response answer your queries?

// The `instanceof` checks protect against edge cases where the focused
// element is not of the interface HTMLElement (does not have a `focus`
// or `blur` property).
Expand Down
Loading