List View: Update with scrolling and custom scrollbar. #49793
Conversation
jasmussen
requested review from
scruffian,
jameskoster,
SaxonF and
andrewserong
|
Size Change: +727 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks for opening this alternate PR @jasmussen!
Unfortunately I ran into a few issues while testing. The most concerning one was what appears to be buggy behaviour in Safari, that I couldn't replicate on Chrome or other browsers. The custom scrollbars seem to result in odd rendering issues for me while interacting with it, and sometimes the scrollbars don't appear to work at all. Here's a screengrab:
2023-04-14.09.42.19.mp4
Can anyone else replicate that happening, or it just on my machine? I'm wondering if the custom scrollbars are affecting the windowing logic or rendering of the list view 🤔
Less severe in Chrome where it was working correctly, I noticed a couple of other smaller issues:
- There's now a gutter at the right hand edge of the list items. It's most visible when a block is selected. Is that intentional?
- The site editor isn't scrolling horizontally as the
width: 350pxrule from Site Editor List View: Fix the width of the list view to 350px #49508 hasn't been ported over:
Should we add back in the following to the site edit list view panel content?
@include break-medium() {
// Same width as the Inserter.
// @see packages/block-editor/src/components/inserter/style.scss
width: 350px;
}
Aside from the weird behaviour in Safari, I otherwise really like the look of the custom scrollbars now that they're styled for light mode.
|
Oh good feedback, thank you. I personally don't mind the reserved space for the scrollbar as it avoids a layout shift, but let me see if there are a couple of tricks I can do. Same with the Safari windowing issue. |
|
I think I may have threaded the needle. I was able to reproduce the Safari issue, but will-change: transform; seems to fix it:
Note the slight jump if you scroll to the rightmost edge, or the bottom of the list view, and then mouse-out. With the removal of the stable scrollbar gutter, that jump happens. But I find it to be acceptable, because I think separately we should probably follow up with a PR to automatically scroll to the left side of the list view when you mouse out. This will need to be done programmatically and is probably not that urgent, what do you think? Note that I updated the mixin still a bit more, so it needs good testing if we want to land this, both in post and site editor, speaking of which here it feels pretty good:
Note I restored the stable scrollbar gutter in the case of the templates list in the stie view. Without it:
With it:
|
|
Nice this seems to work well. I don't think it's a blocker, but it's a shame how the ellipsis button feels out-of-reach when there's horizontal scrolling. A right-click affordance would probably solve this, but that might be a while away. Alternatively, we could try something with sticky.mp4What do you think? |
There was a problem hiding this comment.
Thanks for the updates! This feels a little more stable in testing, but I'm still running into issues with the windowing logic. For example, with a very long post, the list view isn't updating between placeholder vs real list view items when scrolling down the list.
To reproduce
- Create a post with a very large number of blocks
- Save and/or update the post
- Reload the editor, and scroll to the bottom of the editor canvas and select the bottom-most block
- Scroll the list view to the bottom.
- Note that there's empty space in the list view (the invisible placeholder rows have not been swapped out for real list view items):
Note the slight jump if you scroll to the rightmost edge, or the bottom of the list view, and then mouse-out. With the removal of the stable scrollbar gutter, that jump happens.
This feels like a bit of a blocker to me, personally, as it results in a few issues:
- It's hard to use the settings menu on selected blocks as the click position jumps
- In longer block titles the bump is a bit distracting
- It bumps the popover for the settings menu
Here's how it's looking in my testing environment:
| Hard to click on the settings icon | Popover position jumps |
|---|---|
 |
 |
because I think separately we should probably follow up with a PR to automatically scroll to the left side of the list view when you mouse out
I'm not sure if that would be desirable, as it'd make it harder for folks to work with switching between the editor canvas and the list view when working on adjacent deeply nested blocks. E.g. you open the list view deeply to look at a couple of headings and a paragraph, and you click through each to make adjustments to colors. With this behaviour, each time you go back to the list view, you'd have to scroll to the right again. I think it'd probably be better to keep the list view position static / not changing it based on hover.
Alternatively, we could try something with position: sticky; on block-editor-list-view-block__menu-cell. A very quick Inspector mockup
This idea looks pretty cool, though — perhaps with that, we wouldn't need to automatically scroll the list view?
Overall, I'm liking the direction, but it's a bit concerning that the on-hover behaviour results in issues for the windowing logic, and the introducing of some jumpiness. I was wondering if at this stage, would it be easier to split these changes out into two separate PRs — one for the width changes + use normal scrollbars, then look at the custom scrollbars / on hover behaviour in a separate PR if it looks like it's going to be challenging to get it to behave smoothly in the list view?
|
Update: I just noticed there's another bug introduced by hiding the scrollbars when the list view is not hovered. It then appears to break the behaviour in |
There was a problem hiding this comment.
Second update: I think I see why this PR results in a fair few confusing bugs. We use getScrollContainer in a number of places for windowing logic, scroll logic, and soon to calculate the width of the drop indicator (#49786). This works by walking up the DOM tree to find the scrollable container, to then do various calculations that we might need for these features.
On balance, how important is it that we restrict the scrollbars to only appear on hover for the list view via updating the overflow setting? At first glance, it might not be simple to update the affected features to still work when overflow: hidden is in use, so it could be good to explore alternatives or weigh up the options 🤔
packages/base-styles/_mixins.scss
Outdated
| &:hover, | ||
| &:focus, | ||
| & > * { | ||
| visibility: visible; | ||
| } | ||
|
|
||
| // Scrolling behavior. | ||
| overflow: hidden; |
There was a problem hiding this comment.
I think this line could be causing a few issues with windowing and other logic in the list view.
|
Thanks for all the reviews. I need to pause work on it for a bit to focus on other things, so folks should feel welcome to either fork the PR or push directly to it. The main motivation remains: make the scrollbars feel out of sight, out of mind, but still be there when you need them. The auto-hiding helps with that as they don't add visual weight until you interact with it, the collapsing scroll gutter furthermore makes it so things like up when no scrollbars are needed.
For the site editor in the dark material on the left, I consider it somewhat important. But in general, and perhaps moreso for the list view, if it continues to cause trouble there are probably other ways we can address this, like changing the appearance of the scrollbar to be more opaque when hovering/focusing, less opaque when not. But I'd still consider that plan B territory, as the auto-hiding scrollbars does allow us to have things line up, when a scrollbar is needed, but the list view is not hovered. Worth pushing this a bit more for, IMO, but not be all, end-all. That said, I actually moved the overflow definition to the mixin assuming it to be useful there, but it doesn't have to be there. I believe that we can actually refactor out the |
|
Also, thanks so much for looking and reviewing 🙏 |
packages/base-styles/_mixins.scss
Outdated
| @@ -489,12 +489,9 @@ | |||
| /* stylelint-enable function-comma-space-after */ | |||
| } | |||
|
|
|||
| @mixin custom-scrollbars-on-hover() { | |||
| @mixin custom-scrollbars-on-hover($handle-color, $track-color) { | |||
There was a problem hiding this comment.
I moved this change to a separate PR as it will enable us to share this mixin elsewhere: #49892
scruffian
force-pushed
the
try/list-view-scrolling
branch
from
80388dc
to
e0d04f9
Compare
|
@scruffian thank you for the rebase. Were you going to take a look at this one? Otherwise I'm happy to try and pick it up and see if I can move the overflow values around a bit. |
|
I don't think I'll have time in the next couple of weeks, sorry. |
|
No worries, I'll see if I can't pick this up again, just wanted to make sure I didn't push over code you had somewhere locally. |
jasmussen
force-pushed
the
try/list-view-scrolling
branch
from
e0d04f9
to
65ea3ba
Compare
|
Okay, I think I've actually threaded the needle with a refactor. What seems most likely to have caused flickering in Safari was the visibility hiding and unhiding. The primary purpose of this was to ensure the scrollbars were visible only when needed, and also to have those scrollbars not take up space until hovering. In the latter case, this was effectively cancelled out by This seems to be working well. Shown here, Chrome:
Safari: Both of those use webkit specific properties to style the scrollbar, which makes it always be present despite your OS setting on whether to show or auto-hide scrollbars or not. Firefox on the other hand leverages a new web standard that changes depending on always show or onscroll-only scrollbars. So testing twice here, with scrollbars always on:
Auto hide:
We discussed briefly whether to reserve space for the scrollbars or not, and how I liked the idea of not reserving space so that focused items in the list view could have equidistant padding all around, like this:
In part, this is still present in this branch as is, when there is not enough content to scroll. So the space for the scrollbar appears with the scrollbar. I think this is acceptable as a starting point. If we want to revisit this, the following CSS appended in context of where the mixin is used, appears to allow the previous behavior I was exploring: What do you think? |
|
For posterity, I also validated that scrolling still works as intended on a mobile device:
|
There was a problem hiding this comment.
This is terrific work @jasmussen, thank you for figuring out how to advance the design while preserving all the current technical details!
In part, this is still present in this branch as is, when there is not enough content to scroll. So the space for the scrollbar appears with the scrollbar. I think this is acceptable as a starting point.
I think that gives us the best balance of behaviour for now as it gets us closer to the ideal design, while not introducing the overflow: hidden rule, which appears to be what caused other aspects of the list view to stop working, as well as the jump when hovering over list view items.
In the current state of the PR, the jump is only really visible when resizing the window vertically, so I imagine it won't be noticed by anyone (and in this context it isn't at all intrusive):
This is testing great for me, here's an overview of what I tested:
✅ Scroll list view to selected block behaviour is preserved
✅ Windowing logic works correctly
✅ Width of drop indicator logic still works correctly
✅ Safari bug is resolved
✅ height: 100% for the list view content means you can hover over the space at the bottom to show scrollbars
✅ Scrollbar gutter is only introduced when there are enough list items to require it
✅ Scrollbar gutter when present looks good to me
✅ Tests well in Chrome, FF, Safari, and Edge on Mac
✅ Tested post, site, and template editors
✅ Mobile breakpoints look consistent between editors, and allows full-width list view on mobile
This LGTM! ✨
|
Thanks for the thorough testing. Given that we have styled scrollbars shipping in both the site view of the site editor, as well as unstyled horizontally in the list view, I'm going to land this one as I think it's a solid step forward for those scrollbars. Let's generally keep an eye on any scrollbar related issues that may emerge from this. |
bph
added
[Feature] List View
What?
Alternative to #49508, followup to #49611.
Keeps the recently enabled horizontal scrolling in the list view, but updates it with an on-hover scrollbar. It also refactors the same scrollbar mixin to support colors.
Why?
By leveraging on-hover/focus scrollbars, scrolling is available when it needs to be but doesn't weigh the interface when you're not interacting with it.
How?
Uses standard CSS and scrollbar properties.
Testing Instructions
Please test post editor, site editor, and the site editor template view to verify that the onhover scrollbars work as intended.
Screenshots or screencast
Site editor:
Post editor:
Templates list: