-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
List View: render a fixed number of items #35230
Conversation
Size Change: +5.72 kB (+1%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
bf2f388
to
2be5be2
Compare
I've modified the spec on this branch to run the typing and focus tests with list view open. To verify visual changes to the performance tests we can run it with: npm run test-performance -- --wordpress-base-url=http://e2e.local/ --wordpress-username=<username>--wordpress-password=<password> --puppeteer-interactive packages/e2e-tests/specs/performance/post-editor.test.js
npm run test-performance -- --wordpress-base-url=http://e2e.local/ --wordpress-username=<username>--wordpress-password=<password> --puppeteer-interactive packages/e2e-tests/specs/performance/site-editor.test.js perf specs need to be written carefully since inserting blocks with the global inserter will hide the list view. Initial results show a modest change for typing and a rather large impact on focus with list view open:
|
c95ba68
to
b26f493
Compare
b268fb5
to
a0d49a1
Compare
Screen readers handle tables differently, but one common aspect will be that most will try to read the number of columns and rows. Since we're exploring using the windowing technique here, the main difference is that the number of rows won't reflect the total number of list items we can reach. I can create labels with the number of total blocks / blocks shown currently, but I do wonder what type of labels we can create that will make this more useful for screen readers in general. Do folks have any thoughts on how we can improve labels in list view when we only render some rows? Maybe @diegohaz or anyone else familiar with screen readers could chime in? For reference, here's what behavior looks like on trunk using voiceover: CleanShot.2021-10-06.at.14.35.47-converted.mp4Here's what it looks like in this branch: CleanShot.2021-10-06.at.15.14.50.mp4 |
Going to put this as ready for review since I'm keen to see if folks can break the branch during testing, or if it needs more stability fixes. I'm also interested in any feedback for more appropriate/useful labels for the List View table. |
await insertBlock( 'Paragraph' ); | ||
await page.click( '.edit-post-header-toolbar__list-view-toggle' ); | ||
await page.click( '.edit-post-visual-editor__post-title-wrapper' ); | ||
await page.keyboard.press( 'Enter' ); |
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.
Note that we can drop the performance spec changes if this PR happens to land. I wanted some concrete numbers though for the potential perf improvements.
See notes in #35230 (comment)
The treegrid items do (or should) already have their own aria labels and that's because screenreader support is currently limited for some of these more advanced patterns. It could be that we have the same kind of thing at the table, replicate the useful information (number of rows, columns) using an aria label. The only problem would be if the screenreader also reads the incorrect windowed number of rows. FYI, this PR would also affect the number of columns in the site editor - #35170. |
@talldan sounds good, I'll give that a try to start.
Thanks for the heads up. If the ellipsis PR lands first, that may be a bigger rebase, but it shouldn't be a problem 😄
@jasmussen it's definitely a technique to keep in mind. Applying windowing to the block list is much harder to get right since the items are of variable height/width, and have nested children. I think it's been tried in the past, but it could be worth trying again in the future. It's also much more likely to break functionality, but would let us relax the need to try and optimize some other pieces since it'd give a consistent number of blocks to render. |
8fd170c
to
541cfb4
Compare
Edit 2: After some further profiling it turns out the issue was how we were querying selectedClientIds. I've isolated a focus fix in #35706 Note that #35706 isn't an alternative but a PR we can potentially land before this one, I'd still love to see a form of windowing to fix List View's slow first render time. |
Going to rebase to pull in changes from #35706 I may also go ahead with the windowing + placeholder approach, since the perf overhead isn't much ~100ms, and rendering time is still ~300ms vs 3-5 seconds. This should also avoid any accessibility regressions. |
f6b9545
to
0dbac74
Compare
New performance baseline, before I modify the algorithm:
See full performance run details https://github.com/WordPress/gutenberg/runs/3979022008:
|
e04d360
to
f9e70e3
Compare
Here are results using the placeholder method. As expected list view open time is ~100ms slower than using padding, but still an order of magnitude faster than trunk for a post that contains ~900 blocks. Since we don't modify total number of table rows changes in this PR shouldn't regress screenreader navigation. This one is set for review 👍
Full performance run results https://github.com/WordPress/gutenberg/runs/3980496064
|
@gwwar Seems to still work fine for accessibility. Tested using NVDA on Windows 10 in Google Chrome. I can't even notice any lag in List View anymore. |
f9e70e3
to
5e2eaca
Compare
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.
Looks good. I think my only concern is the blank areas that you see when scrolling really fast. Not sure if there's a way to make this slightly more visually appealing.
I notice drag and drop performance is really bad in trunk
to the point where it becomes unresponsive. Feels like a regression, I don't recall it being as bad as it is right now.
It is slightly better in this branch, but still very stuttery:
Kapture.2021-10-27.at.17.17.03.mp4
🔥 |
🎉 Much appreciated for the follow up review @talldan, I know this was quite a bit to test! Thanks to @alexstine and everyone else who helped review too!
There's a few options here. I think we're still re-rendering more than we need to on scroll, so there's further optimizations we can profile. Another one might be exploring using a pulsing or loading asset in the placeholder cell.
Oh interesting, I'll try and profile this next to see if I can spot it. 🔍 |
This PR explores using the windowing technique in list view where we render a fixed number of items at a time instead of every block. This change should help stabilize the max time required for block focus (it should no longer grow with the number of blocks in the document) when list view is open, and should unblock other drag experiments in #34022
Performance
I've modified the spec on this branch to run the typing and focus tests with list view open. I also added a new test to toggle list view open and closed. Runs show around a 10x improvement in open time, ~7x improvement in focus time, and a modest improvement in typing speed when list view is open.
To verify visual changes to the performance tests we can run it with:
perf specs need to be written carefully since inserting blocks with the global inserter will hide the list view.
Example with nested items:
CleanShot.2021-09-30.at.15.19.39.mp4
Performance test post.
Note how we show blank padding when scrolling quickly. We can optimize for this by trying to render more items (updating the window overscan) or adding maybe some loading placeholders.
CleanShot.2021-09-30.at.15.20.45.mp4
Keyboard support:
CleanShot.2021-10-04.at.15.16.17.mp4
Example of drag scrolling (shown with list position debug numbers)
CleanShot.2021-10-05.at.14.51.50.mp4
Screen reader should read out the same labels for rows and initial table counts:
Testing Instructions
TODO: