Lighter block DOM: put sibling inserter in popover #19456

ellatrix · 2020-01-07T10:23:47Z

Description

This results in a performance gain of 26% for typing. It also improves loading time. The big post we're testing loads 20% faster.

This PR moves the sibling inserter out of the block DOM. Currently it is rendered for every block on the page, selected or not, so it can be shown on hover. This PR takes a different approach: it looks at mouse movement between the block and then renders an inserter in the right place. Only one inserter is ever rendered.

Way less inserters rendered, so this solution is more performant.
Much better hover target: the hove target is now exactly as big as the space between the blocks. Previously it has a fixed height.

In order for it to work for keyboard users, an inserter button is visually hidden in the toolbar, but shows when it receives focus. This is similar to other shortcuts in Gutenberg like the "open publish panel" button.

How has this been tested?

Screenshots

Types of changes

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR. .

ellatrix · 2020-01-08T11:11:57Z

This branch seems to perform a lot better than master, because it's avoiding an inserter to be rendered for every block. Average time to type from 68ms to 50ms!

   37e3f31ff..ece5954d7  try/sib-inserter -> try/sib-inserter
Ellas-MacBook-Pro:gutenberg ella$ npm run test-performance

> gutenberg@7.2.0-rc.1 test-performance /Users/ella/gutenberg
> wp-scripts test-e2e --config packages/e2e-tests/jest.performance.config.js

 PASS  packages/e2e-tests/specs/performance/performance.test.js (25.42s)
  Performance
    ✓ 1000 paragraphs (23339ms)

Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        25.525s, estimated 35s
Ran all test suites.

Average time to load: 5144ms
Average time to DOM content load: 5097ms
Average time to type character: 50.385ms
Slowest time to type character: 132ms
Fastest time to type character: 38ms
		
Ellas-MacBook-Pro:gutenberg ella$ git checkout master
M	packages/e2e-tests/config/setup-test-framework.js
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
Ellas-MacBook-Pro:gutenberg ella$ npm run test-performance

> gutenberg@7.2.0-rc.1 test-performance /Users/ella/gutenberg
> wp-scripts test-e2e --config packages/e2e-tests/jest.performance.config.js

 PASS  packages/e2e-tests/specs/performance/performance.test.js (32.718s)
  Performance
    ✓ 1000 paragraphs (29180ms)

Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        32.86s
Ran all test suites.

Average time to load: 6343ms
Average time to DOM content load: 6258ms
Average time to type character: 68.325ms
Slowest time to type character: 143ms
Fastest time to type character: 42ms
		
Ellas-MacBook-Pro:gutenberg ella$ git checkout -
M	packages/e2e-tests/config/setup-test-framework.js
Switched to branch 'try/sib-inserter'
Ellas-MacBook-Pro:gutenberg ella$ npm run test-performance

> gutenberg@7.2.0-rc.1 test-performance /Users/ella/gutenberg
> wp-scripts test-e2e --config packages/e2e-tests/jest.performance.config.js

 PASS  packages/e2e-tests/specs/performance/performance.test.js (27.068s)
  Performance
    ✓ 1000 paragraphs (23470ms)

Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        27.182s, estimated 33s
Ran all test suites.

Average time to load: 5243ms
Average time to DOM content load: 5202ms
Average time to type character: 49.99ms
Slowest time to type character: 145ms
Fastest time to type character: 38ms
		
Ellas-MacBook-Pro:gutenberg ella$ git checkout -
M	packages/e2e-tests/config/setup-test-framework.js
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
Ellas-MacBook-Pro:gutenberg ella$ npm run test-performance

> gutenberg@7.2.0-rc.1 test-performance /Users/ella/gutenberg
> wp-scripts test-e2e --config packages/e2e-tests/jest.performance.config.js

 PASS  packages/e2e-tests/specs/performance/performance.test.js (31.999s)
  Performance
    ✓ 1000 paragraphs (28880ms)

Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        32.122s
Ran all test suites.

Average time to load: 6276ms
Average time to DOM content load: 6193ms
Average time to type character: 67.98ms
Slowest time to type character: 155ms
Fastest time to type character: 53ms

jasmussen · 2020-01-08T11:48:48Z

That speed boost is incredible! Let's get this in as fast as we can.

Overall, this tests really well for me:

I noticed two small issues. The first one you see at the end of that GIF, sometimes the sibling inserter plus seems to be lingering. It seems to work like this:

Hover or select a block.
Now move the mouse to the whitespace left or right of the block.
The plus seems to appear then.

The other issue I'm unsure about, it could be a bad compile on my part, or a ghost of a previous branch, but I clicked a paragraph, then pressed Tab, and I got the following JS error:

Uncaught TypeError: Cannot delete property 'current' of #<Object>
    at onFocus (focus-capture.js:47)
    at HTMLUnknownElement.callCallback (react-dom.8b3dda97.js?ver=16.9.0:341)
    at Object.invokeGuardedCallbackDev (react-dom.8b3dda97.js?ver=16.9.0:391)
    at invokeGuardedCallback (react-dom.8b3dda97.js?ver=16.9.0:448)
    at invokeGuardedCallbackAndCatchFirstError (react-dom.8b3dda97.js?ver=16.9.0:462)
    at executeDispatch (react-dom.8b3dda97.js?ver=16.9.0:594)
    at executeDispatchesInOrder (react-dom.8b3dda97.js?ver=16.9.0:613)
    at executeDispatchesAndRelease (react-dom.8b3dda97.js?ver=16.9.0:719)
    at executeDispatchesAndReleaseTopLevel (react-dom.8b3dda97.js?ver=16.9.0:727)
    at forEachAccumulated (react-dom.8b3dda97.js?ver=16.9.0:701)

Other than those two issues, this one feels great to me!

ellatrix · 2020-01-08T11:50:22Z

@jasmussen Thanks a lot for testing! I'll work on fixes.

ellatrix · 2020-01-08T11:52:30Z

I can't seem to reproduce the error. I click on a paragraph and press tab, but error.

jasmussen · 2020-01-08T11:55:02Z

Probably just some local environment artifact on my side then. I've been jumping from fairly different branches.

ellatrix · 2020-01-08T12:21:17Z

@jasmussen I think I fixed the lingering inserter. Is it better? For the other issue it might help to rebuild the files with npm run dev.

jasmussen · 2020-01-08T12:24:02Z

About to relocate but tried a quick compile, now I can't get the sibling inserter to show up at all, instead I just see this:

Again, it's very possible I'm doing something wrong when compiling, though I am using npm run dev.

Can test again in a bit.

ellatrix · 2020-01-08T12:34:22Z

@jasmussen Hm, that's strange I see this:

jasmussen · 2020-01-08T12:59:27Z

Alright, I relocated home, and everything works as it should, suggesting it was my coworking space build system that was messy.

The one final thing to look at is that the following should not be possible:

That is — the sibling inserter should be available between all blocks when hovering, but never immediately before the selected block. Otherwise this can happen:

That's the only remaining difference I can tell from this branch and master!

ellatrix · 2020-01-08T13:06:05Z

@jasmussen Fixed that too now :)

ellatrix · 2020-01-09T09:46:58Z

I think this is in a good shape now. The interaction is improved (more accurate hover targets), it clearly benefits performance, and there's a tabbable inserter just like before. Let's merge and iterate.

youknowriad · 2020-01-09T10:01:13Z

Average time to type from 68ms to 50ms!

😍

aduth · 2020-01-20T18:29:25Z

packages/block-editor/src/components/block-list/insertion-point.js

+	const [ inserterClientId, setInserterClientId ] = useState( null );
+
+	function onMouseMove( event ) {
+		if ( event.target.className !== className ) {


What is this logic meant to be doing? It's quite unclear to me in reading this. A few inline comments or named utility functions could help clarify here.

FWIW, it raises some curiosity how reliable className is here, whether that might hurt maintainability or interoperability (I know of at least a few Chrome extensions which apply classes to the live DOM, etc). Could be something where event.target.classList.contains could prove more durable.

Yeah, if we need to take into account outside changes to the class name, then checking with contains is better. Even better would be to keep a list of all the reference check if it's included.

aduth · 2020-01-20T18:31:25Z

packages/block-editor/src/components/block-list/insertion-point.js

+		setIsInserterShown( true );
+		setInserterElement( element );
+		setInserterClientId( clientId );


While it might not be too much an issue since setState won't cause a render unless the state actually changes, I note (by placing a logpoint) that this gets called upwards of ~15 times every time a cursor moves within a sibling inserter. Is it something where (even if we don't strictly need to), we could abort the logic when determining that nothing needs to change?

Would it make much difference, you think? I thought the only thing we'd avoid is an extra function call.

Would it make much difference, you think? I thought the only thing we'd avoid is an extra function call.

It's hard to say. I'm not too sure what all React does internally to decide how to handle setState. Depending if we could abort any earlier, it could be an opportunity to avoid reflow from one of the getBoundingClientRect? I'm not sure of the implementation here to know whether that's even possible; I'd guess by the fact there are already some return; that it's probably not?

jasmussen · 2020-03-10T14:00:17Z

It's interesting that we haven't noticed this before now, given that this PR is more than 2 months old. However it appears this PR regressed the behavior where a blue line appears after the selected line, when you hover a block in the toolbar. You can see it after the selected block here:

After this PR, it's gone:

@ellatrix do you think this is difficult to restore?

ellatrix · 2020-03-10T14:53:30Z

I'll have a look.

ellatrix · 2020-03-10T16:53:31Z

I don't know what happened here. I clearly recall working on this during the refactor so the line is preserved. I'll do some digging.

ellatrix added [Type] Performance Related to performance efforts [Type] Code Quality Issues or PRs that relate to code quality labels Jan 7, 2020

ellatrix requested review from talldan and youknowriad as code owners January 7, 2020 10:23

ellatrix requested review from ajitbohra, chrisvanpatten, jorgefilipecosta, nerrad and ntwb as code owners January 7, 2020 12:40

ellatrix force-pushed the try/sib-inserter branch from 5b7678a to 3fa6656 Compare January 8, 2020 10:34

ellatrix added 10 commits January 9, 2020 07:53

Lighter block DOM: put sibling inserter in popover

43f4a7c

Optimise

bc18435

Fix multi select e2e test

0f3ac15

Prevent lingering inserter

8064208

Don't show inserter above selected block

886ca5d

Remove some dead CSS

996b9e0

Add tabbable inserter for multi selected blocks

a70d0ea

Polish

29dfe1b

Simplify

15fd90e

Clean up

c3415fd

Simplify

5aae9ca

ellatrix force-pushed the try/sib-inserter branch from 72cd331 to 25e92b0 Compare January 9, 2020 06:54

Fix nested block inserter

4c12937

ellatrix force-pushed the try/sib-inserter branch from 25e92b0 to 4c12937 Compare January 9, 2020 08:16

ellatrix merged commit 8abebc6 into master Jan 9, 2020

ellatrix deleted the try/sib-inserter branch January 9, 2020 09:47

ellatrix added this to the Gutenberg 7.3 milestone Jan 20, 2020

aduth reviewed Jan 20, 2020

View reviewed changes

This was referenced Mar 16, 2020

reposition of in-between inserter and make it optional #11798

Closed

Classic block partially overlaps the in-between lines inserter #10471

Closed

Overhaul sibling inserter to only appear after a selected block #13571

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lighter block DOM: put sibling inserter in popover #19456

Lighter block DOM: put sibling inserter in popover #19456

ellatrix commented Jan 7, 2020 •

edited

Loading

ellatrix commented Jan 8, 2020 •

edited

Loading

jasmussen commented Jan 8, 2020

ellatrix commented Jan 8, 2020

ellatrix commented Jan 8, 2020

jasmussen commented Jan 8, 2020

ellatrix commented Jan 8, 2020

jasmussen commented Jan 8, 2020

ellatrix commented Jan 8, 2020

jasmussen commented Jan 8, 2020

ellatrix commented Jan 8, 2020

ellatrix commented Jan 9, 2020

youknowriad commented Jan 9, 2020

aduth Jan 20, 2020

ellatrix Jan 21, 2020

aduth Jan 20, 2020

ellatrix Jan 21, 2020

aduth Jan 21, 2020

jasmussen commented Mar 10, 2020

ellatrix commented Mar 10, 2020

ellatrix commented Mar 10, 2020

Lighter block DOM: put sibling inserter in popover #19456

Lighter block DOM: put sibling inserter in popover #19456

Conversation

ellatrix commented Jan 7, 2020 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

ellatrix commented Jan 8, 2020 • edited Loading

jasmussen commented Jan 8, 2020

ellatrix commented Jan 8, 2020

ellatrix commented Jan 8, 2020

jasmussen commented Jan 8, 2020

ellatrix commented Jan 8, 2020

jasmussen commented Jan 8, 2020

ellatrix commented Jan 8, 2020

jasmussen commented Jan 8, 2020

ellatrix commented Jan 8, 2020

ellatrix commented Jan 9, 2020

youknowriad commented Jan 9, 2020

aduth Jan 20, 2020

Choose a reason for hiding this comment

ellatrix Jan 21, 2020

Choose a reason for hiding this comment

aduth Jan 20, 2020

Choose a reason for hiding this comment

ellatrix Jan 21, 2020

Choose a reason for hiding this comment

aduth Jan 21, 2020

Choose a reason for hiding this comment

jasmussen commented Mar 10, 2020

ellatrix commented Mar 10, 2020

ellatrix commented Mar 10, 2020

ellatrix commented Jan 7, 2020 •

edited

Loading

ellatrix commented Jan 8, 2020 •

edited

Loading