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

Remove focus event from RichText #44304

Closed
wants to merge 4 commits into from
Closed

Remove focus event from RichText #44304

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
799672b
Remove focus event from RichText
youknowriad Sep 20, 2022
6520952
Fix e2e tests
youknowriad Sep 20, 2022
f183e10
check restoring the handleSelectionChange call
youknowriad Sep 21, 2022
ba8f297
Improve the focus metric
youknowriad Sep 22, 2022
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/e2e-tests/specs/editor/various/block-deletion.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ const addThreeParagraphsToNewPost = async () => {
await page.keyboard.press( 'Enter' );
await page.keyboard.type( 'Second paragraph' );
await page.keyboard.press( 'Enter' );
await page.evaluate( () => new Promise( window.requestIdleCallback ) );
};

/**
Expand Down
1 change: 1 addition & 0 deletions packages/e2e-tests/specs/editor/various/undo.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,7 @@ describe( 'undo', () => {
// Issue is demonstrated by forcing state merges (multiple inputs) on
// an existing text after a fresh reload.
await selectBlockByClientId( ( await getAllBlocks() )[ 0 ].clientId );
await page.evaluate( () => new Promise( window.requestIdleCallback ) );
await page.keyboard.type( 'modified' );

// The issue is demonstrated after the one second delay to trigger the
Expand Down
30 changes: 17 additions & 13 deletions packages/e2e-tests/specs/performance/post-editor.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ import {

jest.setTimeout( 1000000 );

function sum( arr ) {
return arr.reduce( ( a, b ) => a + b, 0 );
}

describe( 'Post Editor Performance', () => {
const results = {
serverResponse: [],
Expand Down Expand Up @@ -163,22 +167,26 @@ describe( 'Post Editor Performance', () => {
dispatch( 'core/block-editor' ).resetBlocks( blocks );
} );
const paragraphs = await page.$$( '.wp-block' );
await page.tracing.start( {
path: traceFile,
screenshots: false,
categories: [ 'devtools.timeline' ],
} );
await paragraphs[ 0 ].click();
results.focus = [];
for ( let j = 1; j <= 10; j++ ) {
await page.tracing.start( {
path: traceFile,
screenshots: false,
categories: [ 'devtools.timeline' ],
} );
// Wait for the browser to be idle before starting the monitoring.
// eslint-disable-next-line no-restricted-syntax
await page.waitForTimeout( 1000 );
await paragraphs[ j ].click();
// Wait for the browser to be idle before starting the monitoring.
// eslint-disable-next-line no-restricted-syntax
await page.waitForTimeout( 1000 );
await page.tracing.stop();
traceResults = JSON.parse( readFile( traceFile ) );
const [ focusEvents ] = getSelectionEventDurations( traceResults );
results.focus.push( sum( focusEvents ) );
}
await page.tracing.stop();
traceResults = JSON.parse( readFile( traceFile ) );
const [ focusEvents ] = getSelectionEventDurations( traceResults );
results.focus = focusEvents;
} );

it( 'Opening persistent list view', async () => {
Expand Down Expand Up @@ -222,10 +230,6 @@ describe( 'Post Editor Performance', () => {
} );

it( 'Searching the inserter', async () => {
function sum( arr ) {
return arr.reduce( ( a, b ) => a + b, 0 );
}

// Measure time to search the inserter and get results.
await openGlobalBlockInserter();
for ( let j = 0; j < 10; j++ ) {
Expand Down
11 changes: 8 additions & 3 deletions packages/e2e-tests/specs/performance/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,13 @@ function isKeyUpEvent( item ) {
return isEvent( item ) && item.args.data.type === 'keyup';
}

function isFocusEvent( item ) {
return isEvent( item ) && item.args.data.type === 'focus';
function isFocusOrSelectionEvent( item ) {
return (
isEvent( item ) &&
[ 'focus', 'focusin', 'selectionchange', 'focusout' ].includes(
item.args.data.type
)
);
}

function isClickEvent( item ) {
Expand Down Expand Up @@ -68,7 +73,7 @@ export function getTypingEventDurations( trace ) {
}

export function getSelectionEventDurations( trace ) {
return [ getEventDurationsForType( trace, isFocusEvent ) ];
return [ getEventDurationsForType( trace, isFocusOrSelectionEvent ) ];
}

export function getClickEventDurations( trace ) {
Expand Down
19 changes: 1 addition & 18 deletions packages/rich-text/src/component/use-input-and-selection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,28 +275,11 @@ export function useInputAndSelection( props ) {
return;
}

if ( ! isSelected ) {
// 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;

record.current = {
...record.current,
start: index,
end: index,
activeFormats: EMPTY_ACTIVE_FORMATS,
};
onSelectionChange( index, index );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main goal for my explorations on this PR is to get rid of this, which was causing onSelectionChange to be called twice on focus, I'm assuming it's already done in the "selectionchange" event.

Copy link
Member

Choose a reason for hiding this comment

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

If it's called twice, it will be called first here, then in the "selectionchange" event.

} else {
if ( isSelected ) {
applyRecord( record.current );
onSelectionChange( record.current.start, record.current.end );
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It turns out, this is the only "useful" code here that doesn't duplicate selectionchange event. It's useful when applying formats (like links), that way the selection is restored at the right position after inserting links. Maybe we could even move this code elsewhere (when we actually update record.current) and avoid the event handler entirely but I'm not familiar enough with this code to try it.


// Update selection as soon as possible, which is at the next animation
// frame. The event listener for selection changes may be added too late
// at this point, but this focus event is still too early to calculate
// the selection.
rafId = defaultView.requestAnimationFrame( handleSelectionChange );
Copy link
Member

Choose a reason for hiding this comment

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

Should we first try to remove it without removing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not clear to me why we need this since when "focus" happens, the selection changes so the callback is called no?

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 just curious which of these changes makes which tests fail, so we can address it more easily.

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 just curious which of these changes makes which tests fail, so we can address it more easily.

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 just curious which of these changes makes which tests fail, so we can address it more easily.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like everything passes now that I restored that part

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 say let's look into that separately then.

}

Expand Down