Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
- `ResizableBox`: Update `re-resizable` to `6.11.2`, which supports React 18 and renders the top and left handles before the content so focus order matches visual order ([#83754](https://github.com/WordPress/gutenberg/pull/83754)).
- `PaletteEdit`: Keep an item's slug when it is renamed, so blocks and styles that reference the preset keep working ([#83750](https://github.com/WordPress/gutenberg/pull/83750)).
- `FocalPointPicker`: End a drag when the pointer is released outside the control, including over the editor canvas ([#83348](https://github.com/WordPress/gutenberg/pull/83348)).
- `ContentEditableControl`: Keep a disabled field focusable (out of the tab order), so a field disabled while it has focus keeps it instead of dropping focus on the body ([#84083](https://github.com/WordPress/gutenberg/pull/84083)).

### Internal

Expand Down
5 changes: 2 additions & 3 deletions packages/components/src/content-editable-control/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,8 @@ control in `@wordpress/dataviews` for the canonical assembly).
- Required: No
- Default: `false`

Whether the field is non-editable. A disabled field is not
`contentEditable` (so it is neither focusable nor editable) and exposes
`aria-disabled` to assistive technology.
Whether the field is non-editable. A disabled field exposes
`aria-disabled`, leaves the tab order, and keeps focus if it had it.

### `hideLabelFromVision`

Expand Down
5 changes: 3 additions & 2 deletions packages/components/src/content-editable-control/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,10 @@ function UnforwardedContentEditableControl(
aria-disabled={ disabled || undefined }
aria-required={ required || undefined }
ref={ mergedRefs }
// A disabled field is not `contentEditable`, which also
// removes it from the tab order.
// Not editable and out of the tab order, but still focusable:
// a field disabled while focused keeps focus.
contentEditable={ ! disabled }
tabIndex={ disabled ? -1 : undefined }
suppressContentEditableWarning
{ ...additionalProps }
{ ...controlProps }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,21 @@ describe( 'ContentEditableControl', () => {
expect( textbox ).toHaveAttribute( 'contenteditable', 'false' );
} );

it( 'keeps a disabled field focusable but out of the tab order', () => {
const { rerender } = render( <ContentEditableControl label="Note" /> );

expect( screen.getByRole( 'textbox' ) ).not.toHaveAttribute(
'tabindex'
);

rerender( <ContentEditableControl label="Note" disabled /> );

expect( screen.getByRole( 'textbox' ) ).toHaveAttribute(
'tabindex',
'-1'
);
} );

it( 'exposes `required` via aria-required', () => {
render( <ContentEditableControl label="Note" required /> );

Expand Down
5 changes: 2 additions & 3 deletions packages/components/src/content-editable-control/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,8 @@ export type ContentEditableControlProps = Pick<
*/
label: string;
/**
* Whether the field is non-editable. A disabled field is not
* `contentEditable` (so it is neither focusable nor editable) and exposes
* `aria-disabled` to assistive technology.
* Whether the field is non-editable. A disabled field exposes
* `aria-disabled`, leaves the tab order, and keeps focus if it had it.
*
* @default false
*/
Expand Down
1 change: 1 addition & 0 deletions packages/editor/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
- Notes: Add an Undo action to the "Note deleted." snackbar, and stop the editor's undo from bringing back the highlight of a deleted note ([#83988](https://github.com/WordPress/gutenberg/pull/83988)).
- Notes: Stop the editor's undo from detaching a newly added note from its block, or bringing back the highlight of a resolved note ([#84079](https://github.com/WordPress/gutenberg/pull/84079)).
- Notes: Delete notes permanently when they can't be moved to the trash, instead of failing with an error ([#84102](https://github.com/WordPress/gutenberg/pull/84102)).
- Notes: Disable the note form field while a note or reply is being sent, instead of accepting text the save would not include ([#84083](https://github.com/WordPress/gutenberg/pull/84083)).

### Internal

Expand Down
15 changes: 4 additions & 11 deletions packages/editor/src/components/collab-sidebar/note-form.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -50,23 +50,15 @@ export function NoteForm( { onSubmit, onCancel, note, labels } ) {
return;
}
setIsSubmitting( true );
const submitted = inputComment;

/*
* The note actions resolve with the saved record on success and
* `undefined` on failure (they surface their own error notice),
* so only discard the draft once the save actually succeeded.
* so only discard the content once the save actually succeeded.
*/
const result = await onSubmit( submitted );
const result = await onSubmit( inputComment );
if ( result ) {
/*
* The field stays editable while the request is in flight, so
* keep anything typed since; clearing unconditionally would
* discard it.
*/
setInputComment( ( current ) =>
current === submitted ? '' : current
);
setInputComment( '' );
}

setIsSubmitting( false );
Expand Down Expand Up @@ -102,6 +94,7 @@ export function NoteForm( { onSubmit, onCancel, note, labels } ) {
hideLabelFromVision
value={ inputComment }
onChange={ setInputComment }
disabled={ isSubmitting }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Keeping the disabled field focused still lets rich-text handlers change it: Backspace and Cut can erase the draft, and delayed mention suggestions can insert text that the successful save then clears.

We should probably stop rich-text mutations (deletion, cut, paste, formatting, composition/input handling, and autocomplete) while disabled before removing the draft-preservation guard, especially testing them during a pending submission.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I checked each one. Paste, formatting shortcuts, composition and Enter can't change a disabled field: a non-contentEditable element gets no native input and FormatEdit isn't rendered while disabled.

Backspace/Delete and Cut still can, but only with a selection made before submitting, and a mention suggestion can still be picked. I don't think any of these is a problem:

  • If the save succeeds, the field is cleared anyway, so whatever happened during the request is gone either way.
  • If it fails, the draft only has the user's own deliberate edits. Cut also puts the text on the clipboard.

Before this PR, the whole field was editable during the request, so this is a narrower surface than before. I'd rather not add guards for this edge case. I also fixed the stale comment in RichTextControl that said a disabled field isn't focusable.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For replies, the old guard kept a draft changed during the request, so a mention selected while saving was not cleared either way.

Maybe the safest thing here is to keep that guard until disabled fields also prevent autocomplete from changing the draft?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

For replies, the old guard kept a draft changed during the request, so a mention selected while saving was not cleared either way.

I don't think that's better behavior. Any action while the disabled form is saving should be discarded, which happens now.

It's not that we're triggering autosave while the user types, which can break their UX.

IMO, this is an improvement over the existing behavior. Our local RichTextControl could handle disabling better, but that requires larger refactoring.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fair enough, let's keep it as-is.

We can handle the remaining edits that slip through while disabled separately.

placeholder={ labels?.placeholder }
allowedFormats={ ALLOWED_NOTE_FORMATS }
completers={ NOTE_COMPLETERS }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -291,9 +291,7 @@ export default function RichTextControl( {
*/
const enterRef = useRefEffect< HTMLElement >(
( element ) => {
// A disabled field is not editable; nothing to handle. (Real
// keyboard input cannot reach it either, since a
// non-`contentEditable` div is not focusable.)
// A disabled field is not editable; nothing to handle.
if ( disabled ) {
return;
}
Expand Down
Loading