-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Rich text: try removing store change on focus #48342
Changes from all commits
ff40e15
150f638
5fd5b61
9d84fd6
83119aa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,7 +29,7 @@ function findSelection( blocks ) { | |
blocks[ i ].attributes[ attributeKey ] = blocks[ i ].attributes[ | ||
attributeKey | ||
].replace( START_OF_SELECTED_AREA, '' ); | ||
return blocks[ i ].clientId; | ||
return [ blocks[ i ].clientId, attributeKey, 0, 0 ]; | ||
} | ||
|
||
const nestedSelection = findSelection( blocks[ i ].innerBlocks ); | ||
|
@@ -38,6 +38,8 @@ function findSelection( blocks ) { | |
return nestedSelection; | ||
} | ||
} | ||
|
||
return []; | ||
} | ||
|
||
export function useInputRules( props ) { | ||
|
@@ -86,9 +88,11 @@ export function useInputRules( props ) { | |
} ); | ||
const block = transformation.transform( content ); | ||
|
||
selectionChange( findSelection( [ block ] ) ); | ||
selectionChange( ...findSelection( [ block ] ) ); | ||
onReplace( [ block ] ); | ||
__unstableMarkAutomaticChange(); | ||
|
||
return true; | ||
} | ||
|
||
function onInput( event ) { | ||
|
@@ -106,7 +110,7 @@ export function useInputRules( props ) { | |
} | ||
|
||
if ( __unstableAllowPrefixTransformations && inputRule ) { | ||
inputRule(); | ||
if ( inputRule() ) return; | ||
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 is probably not needed for this PR, but still good to return here. |
||
} | ||
|
||
const value = getValue(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -89,7 +89,7 @@ export default function getRectangleFromRange( range ) { | |
// by adding a temporary text node with zero-width space to the range. | ||
// | ||
// See: https://stackoverflow.com/a/6847328/995445 | ||
if ( ! rect ) { | ||
if ( ! rect || rect.height === 0 ) { | ||
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. Ranges set by Firefox might have 0 height (when it's collapsed on an empty string). This PR probably changes something where we're overriding the range set by Firefox, which is fine. |
||
assertIsDefined( ownerDocument, 'ownerDocument' ); | ||
const padNode = ownerDocument.createTextNode( '\u200b' ); | ||
// Do not modify the live range. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,6 +29,7 @@ import { | |
getHoverEventDurations, | ||
getSelectionEventDurations, | ||
getLoadingDurations, | ||
sum, | ||
} from './utils'; | ||
|
||
jest.setTimeout( 1000000 ); | ||
|
@@ -235,22 +236,26 @@ describe( 'Post Editor Performance', () => { | |
it( 'Selecting blocks', async () => { | ||
await load1000Paragraphs(); | ||
const paragraphs = await canvas().$$( '.wp-block' ); | ||
await page.tracing.start( { | ||
path: traceFile, | ||
screenshots: false, | ||
categories: [ 'devtools.timeline' ], | ||
} ); | ||
await paragraphs[ 0 ].click(); | ||
for ( let j = 1; j <= 10; j++ ) { | ||
// 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.start( { | ||
path: traceFile, | ||
screenshots: false, | ||
categories: [ 'devtools.timeline' ], | ||
} ); | ||
await paragraphs[ j ].click(); | ||
await page.tracing.stop(); | ||
traceResults = JSON.parse( readFile( traceFile ) ); | ||
const allDurations = getSelectionEventDurations( traceResults ); | ||
results.focus.push( | ||
allDurations.reduce( ( acc, eventDurations ) => { | ||
return acc + sum( eventDurations ); | ||
}, 0 ) | ||
Comment on lines
+254
to
+256
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 smells like it should be sum( allDurations.map( sum ) ) No? |
||
); | ||
} | ||
await page.tracing.stop(); | ||
traceResults = JSON.parse( readFile( traceFile ) ); | ||
const [ focusEvents ] = getSelectionEventDurations( traceResults ); | ||
results.focus = focusEvents; | ||
} ); | ||
|
||
it( 'Opening persistent list view', async () => { | ||
|
@@ -292,9 +297,6 @@ describe( 'Post Editor Performance', () => { | |
} ); | ||
|
||
it( 'Searching the inserter', async () => { | ||
function sum( arr ) { | ||
return arr.reduce( ( a, b ) => a + b, 0 ); | ||
} | ||
await load1000Paragraphs(); | ||
await openGlobalBlockInserter(); | ||
for ( let j = 0; j < 10; j++ ) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -287,7 +287,6 @@ export function useInputAndSelection( props ) { | |
end: index, | ||
activeFormats: EMPTY_ACTIVE_FORMATS, | ||
}; | ||
onSelectionChange( index, index ); | ||
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. So this should be a small (maybe not noticeable) perf improvement, right? 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. It's a huge improvement for
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. Maybe we should update the metric though. It feels now that we should be computing "focus + selection change". It seems the time has moved to another event that we don't take into consideration in the metric itself? |
||
} else { | ||
applyRecord( record.current ); | ||
onSelectionChange( record.current.start, record.current.end ); | ||
|
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.
The reason this fixes the failing test is because we preset selection in the store after replacing the input rule. So when the new block is created, the selection is already correct at the time of the focus event.