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

Jump to bottom

RichText: don't set focus when applying format #19536

Merged
merged 8 commits into from
Jan 13, 2020
Merged

RichText: don't set focus when applying format #19536

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
401d949
Return focus to original element
ellatrix Jan 9, 2020
540051f
Fix list e2e tests
ellatrix Sep 26, 2019
d2905e4
Fix annotation e2e tests
ellatrix Sep 27, 2019
8a1b782
Fix some rich text e2e tests
ellatrix Sep 27, 2019
c087194
Fix tests
ellatrix Sep 27, 2019
da86012
Focus after link
ellatrix Jan 9, 2020
daa575e
Add onFocus
ellatrix Jan 9, 2020
e7eeda3
Corrections
ellatrix Jan 9, 2020
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
4 changes: 2 additions & 2 deletions packages/block-editor/src/components/rich-text/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,9 +411,9 @@ class RichTextWrapper extends Component {
start={ start }
reversed={ reversed }
>
{ ( { isSelected, value, onChange, Editable } ) =>
{ ( { isSelected, value, onChange, onFocus, Editable } ) =>
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about the name onFocus, it indicates that it is an event e.g: that we can pass something that is called when something gains focus, while what happens is that onFocus is a function that excitedly puts that focus back on rich text. Maybe we can call it "focus" or "focusRichText"?

Copy link
Member Author

@ellatrix ellatrix Jan 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

What's the difference with onChange? It's also not an event handler but rather an action to change something. I'd prefer to keep it consistent with onChange, which is maybe also named poorly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Cc @aduth since I know you have opinions about this. :)

Copy link
Member

Choose a reason for hiding this comment

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

I'd tend to agree with @jorgefilipecosta that the value of both onChange and onFocus don't conform to my expectations, where my reading of onFoo would indicate some sort of event handler moreso than a "setter" as it seems to be used here. Then again, we don't usually pass around these functions, and in considering it as the function itself, it could be fine to allow it to represent something of a direct access to invoke the handler.

For me, if it's meant to represent some sort of setter, I'd probably name it accordingly: setValue, setFocus.

If we're at a point where it's too late to change, I don't think it's worth stressing too much over. And I do think the consistency here is important, so even if we'd like setFocus, I think it's more important that it align to onChange in at least being consistent with itself.

<>
{ children && children( { value, onChange } ) }
{ children && children( { value, onChange, onFocus } ) }
{ isSelected && hasFormats && ( <FormatToolbarContainer inline={ inlineToolbar } anchorRef={ forwardedRef.current } /> ) }
{ isSelected && <RemoveBrowserShortcuts /> }
<Autocomplete
Expand Down
6 changes: 5 additions & 1 deletion packages/block-library/src/list/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export default function ListEdit( {
const { ordered, values, type, reversed, start } = attributes;
const tagName = ordered ? 'ol' : 'ul';

const controls = ( { value, onChange } ) => (
const controls = ( { value, onChange, onFocus } ) => (
<>
{ ( isSelected && <>
<RichTextShortcut
Expand Down Expand Up @@ -79,6 +79,7 @@ export default function ListEdit( {
isActive: isActiveListType( value, 'ul', tagName ),
onClick() {
onChange( changeListType( value, { type: 'ul' } ) );
onFocus();

if ( isListRootSelected( value ) ) {
setAttributes( { ordered: false } );
Expand All @@ -91,6 +92,7 @@ export default function ListEdit( {
isActive: isActiveListType( value, 'ol', tagName ),
onClick() {
onChange( changeListType( value, { type: 'ol' } ) );
onFocus();

if ( isListRootSelected( value ) ) {
setAttributes( { ordered: true } );
Expand All @@ -104,6 +106,7 @@ export default function ListEdit( {
isDisabled: ! canOutdentListItems( value ),
onClick() {
onChange( outdentListItems( value ) );
onFocus();
},
},
{
Expand All @@ -113,6 +116,7 @@ export default function ListEdit( {
isDisabled: ! canIndentListItems( value ),
onClick() {
onChange( indentListItems( value, { type: tagName } ) );
onFocus();
},
},
] }
Expand Down
2 changes: 2 additions & 0 deletions packages/e2e-tests/specs/editor/plugins/annotations.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ describe( 'Using Plugins API', () => {
// Click add annotation button.
const addAnnotationButton = ( await page.$x( "//button[contains(text(), 'Add annotation')]" ) )[ 0 ];
await addAnnotationButton.click();
await page.evaluate( () => document.querySelector( '[contenteditable]' ).focus() );
}

/**
Expand All @@ -60,6 +61,7 @@ describe( 'Using Plugins API', () => {
// Click remove annotations button.
const addAnnotationButton = ( await page.$x( "//button[contains(text(), 'Remove annotations')]" ) )[ 0 ];
await addAnnotationButton.click();
await page.evaluate( () => document.querySelector( '[contenteditable]' ).focus() );
}

/**
Expand Down
13 changes: 10 additions & 3 deletions packages/format-library/src/bold/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,15 @@ export const bold = {
title,
tagName: 'strong',
className: null,
edit( { isActive, value, onChange } ) {
const onToggle = () => onChange( toggleFormat( value, { type: name } ) );
edit( { isActive, value, onChange, onFocus } ) {
function onToggle() {
onChange( toggleFormat( value, { type: name } ) );
}

function onClick() {
onToggle();
onFocus();
Copy link
Member

Choose a reason for hiding this comment

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

This change is something that will impact third-party developers creating their own formats, I guess we should refer this PR in a dev note.

Copy link
Member Author

@ellatrix ellatrix Jan 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Added the label. It won't break anything, but yes it's a change in behaviour. :)

}

return (
<>
Expand All @@ -27,7 +34,7 @@ export const bold = {
name="bold"
icon="editor-bold"
title={ title }
onClick={ onToggle }
onClick={ onClick }
isActive={ isActive }
shortcutType="primary"
shortcutCharacter="b"
Expand Down
9 changes: 6 additions & 3 deletions packages/format-library/src/code/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,17 @@ export const code = {

return value;
},
edit( { value, onChange, isActive } ) {
const onToggle = () => onChange( toggleFormat( value, { type: name } ) );
edit( { value, onChange, onFocus, isActive } ) {
function onClick() {
onChange( toggleFormat( value, { type: name } ) );
onFocus();
}

return (
<RichTextToolbarButton
icon="editor-code"
title={ title }
onClick={ onToggle }
onClick={ onClick }
isActive={ isActive }
/>
);
Expand Down
3 changes: 2 additions & 1 deletion packages/format-library/src/image/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ export const image = {
}

render() {
const { value, onChange, isObjectActive, activeObjectAttributes } = this.props;
const { value, onChange, onFocus, isObjectActive, activeObjectAttributes } = this.props;

return (
<MediaUploadCheck>
Expand All @@ -108,6 +108,7 @@ export const image = {
alt,
},
} ) );
onFocus();
} }
onClose={ this.closeModal }
render={ ( { open } ) => {
Expand Down
13 changes: 10 additions & 3 deletions packages/format-library/src/italic/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,15 @@ export const italic = {
title,
tagName: 'em',
className: null,
edit( { isActive, value, onChange } ) {
const onToggle = () => onChange( toggleFormat( value, { type: name } ) );
edit( { isActive, value, onChange, onFocus } ) {
function onToggle() {
onChange( toggleFormat( value, { type: name } ) );
}

function onClick() {
onToggle();
onFocus();
}

return (
<>
Expand All @@ -27,7 +34,7 @@ export const italic = {
name="italic"
icon="editor-italic"
title={ title }
onClick={ onToggle }
onClick={ onClick }
isActive={ isActive }
shortcutType="primary"
shortcutCharacter="i"
Expand Down
3 changes: 2 additions & 1 deletion packages/format-library/src/link/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ export const link = {
}

render() {
const { isActive, activeAttributes, value, onChange } = this.props;
const { isActive, activeAttributes, value, onChange, onFocus } = this.props;

return (
<>
Expand Down Expand Up @@ -131,6 +131,7 @@ export const link = {
activeAttributes={ activeAttributes }
value={ value }
onChange={ onChange }
onFocus={ onFocus }
/>
</>
);
Expand Down
4 changes: 3 additions & 1 deletion packages/format-library/src/link/inline.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ class InlineLinkUI extends Component {
}

submitLink( event ) {
const { isActive, value, onChange, speak } = this.props;
const { isActive, value, onChange, onFocus, speak } = this.props;
const { inputValue, opensInNewWindow } = this.state;
const url = prependHTTP( inputValue );
const selectedText = getTextContent( slice( value ) );
Expand All @@ -154,6 +154,8 @@ class InlineLinkUI extends Component {
onChange( applyFormat( value, format ) );
}

onFocus();

this.resetState();

if ( ! isValidHref( url ) ) {
Expand Down
9 changes: 6 additions & 3 deletions packages/format-library/src/strikethrough/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,17 @@ export const strikethrough = {
title,
tagName: 's',
className: null,
edit( { isActive, value, onChange } ) {
const onToggle = () => onChange( toggleFormat( value, { type: name } ) );
edit( { isActive, value, onChange, onFocus } ) {
function onClick() {
onChange( toggleFormat( value, { type: name } ) );
onFocus();
}

return (
<RichTextToolbarButton
icon="editor-strikethrough"
title={ title }
onClick={ onToggle }
onClick={ onClick }
isActive={ isActive }
/>
);
Expand Down
2 changes: 2 additions & 0 deletions packages/rich-text/src/component/format-edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ const interactiveContentTags = new Set( [
export default function FormatEdit( {
formatTypes,
onChange,
onFocus,
value,
allowedFormats,
withoutInteractiveFormatting,
Expand Down Expand Up @@ -68,6 +69,7 @@ export default function FormatEdit( {
}
value={ value }
onChange={ onChange }
onFocus={ onFocus }
/>
);
} );
Expand Down
43 changes: 30 additions & 13 deletions packages/rich-text/src/component/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,19 +358,29 @@ class RichText extends Component {
unstableOnFocus();
}

// We know for certain that on focus, the old selection is invalid. It
// will be recalculated on the next mouseup, keyup, or touchend event.
const index = undefined;
const activeFormats = EMPTY_ACTIVE_FORMATS;

this.record = {
...this.record,
start: index,
end: index,
activeFormats,
};
this.props.onSelectionChange( index, index );
this.setState( { activeFormats } );
if ( ! this.props.__unstableIsSelected ) {
// We know for certain that on focus, the old selection is invalid. It
// will be recalculated on the next mouseup, keyup, or touchend event.
const index = undefined;
const activeFormats = EMPTY_ACTIVE_FORMATS;

this.record = {
...this.record,
start: index,
end: index,
activeFormats,
};
this.props.onSelectionChange( index, index );
this.setState( { activeFormats } );
} else {
this.props.onSelectionChange( this.record.start, this.record.end );
this.setState( {
activeFormats: getActiveFormats( {
...this.record,
activeFormats: undefined,
}, EMPTY_ACTIVE_FORMATS ),
} );
}

// Update selection as soon as possible, which is at the next animation
// frame. The event listener for selection changes may be added too late
Expand Down Expand Up @@ -1067,6 +1077,11 @@ class RichText extends Component {
} = this.props;
const { activeFormats } = this.state;

const onFocus = () => {
forwardedRef.current.focus();
this.applyRecord( this.record );
};

return (
<>
<BoundaryStyle
Expand All @@ -1078,12 +1093,14 @@ class RichText extends Component {
withoutInteractiveFormatting={ withoutInteractiveFormatting }
value={ this.record }
onChange={ this.onChange }
onFocus={ onFocus }
formatTypes={ formatTypes }
/> }
{ children && children( {
isSelected,
value: this.record,
onChange: this.onChange,
onFocus,
Editable: this.Editable,
} ) }
{ ! children && <this.Editable /> }
Expand Down
6 changes: 2 additions & 4 deletions packages/rich-text/src/to-dom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,10 +278,7 @@ export function applySelection( { startPath, endPath }, current ) {
range.setStart( startContainer, startOffset );
range.setEnd( endContainer, endOffset );

// Set back focus if focus is lost.
if ( ownerDocument.activeElement !== current ) {
current.focus();
}
const { activeElement } = ownerDocument;

if ( selection.rangeCount > 0 ) {
// If the to be added range and the live range are the same, there's no
Expand All @@ -294,4 +291,5 @@ export function applySelection( { startPath, endPath }, current ) {
}

selection.addRange( range );
activeElement.focus();
}