Fix collapsed sidebar icons when scrollbars are present

[archilex]

Currently when 1) sidebars are collapsed and 2) scrollbars are always turned on, the icons will shrink due to there not being enough space for them expand. This PR fixes this by adding items-center to the nav container which allows them to keep their width.

Before:

After:

After with tenancy:

The following fixed has been tested and is working as expected in these scenarios:

1. Sidebar normal, automatic scrollbars
2. Sidebar normal, scrollbars always on
3. Sidebar collapased, automatic scrollbars
4. Sidebar collapased, scrollbars always on
5. The above with and without tenancy
6. The above with and without iconless nav items (showing just the dot)
7. Mobile (but that always hides and when it expands it is full width
8. Tested on Chrome, Safari, Firefox

The only issue, as you can see in the screenshot, is that when scrollbars are always on and the sidebar is collapsed the nav icons do not align with the collapse button at the top. Unfortunately, I don't think there is a way in css alone to know if scrollbars are turned on or not so I don't see a way to conditionally add/remove padding from the collapse button so they align.

Finally, I HAVE NOT TESTED ON WINDOWS/EDGE...Hopefully someone else can confirm on windows.

For additional background, at first I added flex-shrink-0 to the icons which fixed the icon issue, but the button hover:bg-gray-100 was trying to fit within the padding so it was smaller than the expected 40px. Adding flex-shrink-0 to the button has no effect since it's the icon is shrinking. Then I tried setting w-10 for the button, which works when collapsed in conjunction with items-center, but then w-10 needs to be conditionally removed when the sidebar isn't collapased.

Then I realized that just using items-center on the nav fixes everything...which honestly was a little surprising to me, but I'll take it.

• Changes have been thoroughly tested to not break existing functionality.
• New functionality has been documented or existing documentation has been updated to reflect changes.
• Visual changes are explained in the PR description using a screenshot/recording of before and after.

[what-the-diff]

PR Summary

• Enhancement of Sidebar Width Adjustment
  Added an operation that allows us to manipulate the width of the sidebar in our web interface, giving users more control over their interface experiences.

• Removal of Sidebar Width Limitation
  Removed a restriction that capped the sidebar's maximum width. This allows the sidebar to use more space when needed, allowing for more content to be visible and better usability.

• Automatic Alignment Update in Navigation Bar
  Introduced a new feature that realigns the items in the navigation bar depending on whether the sidebar is open or not. This change enhances user interaction and accessibility.

[archilex]

Forgot to mention I went ahead and included overflow-x-hidden is also addressed in PR #8098

[zepfietje]

Forgot to mention I went ahead and included overflow-x-hidden is also addressed in PR #8098

Could you undo that change? I'd like to address that issue separately, as that's probably not the right fix (I recall removing that overflow-x-hidden class to fix another bug.

[zepfietje]

Thanks for taking the time to fix this issue, @archilex, appreciate it.

Could you mark this PR as ready for review after you've implemented the changes I requested?
Then I'll do a final thorough review to make sure this is actually the best fix and doesn't break any other cases (as you noticed already, this is a tricky one).

[archilex]

My pleasure....I hope!...I was very hesitant to touch it since I know there are a lot of special cases to handle. Keep up the great work!

[zepfietje]

So this issue is actually caused by the specific sidebar width we set?
I'd prefer determining the width based on the content by default (instead of setting a hard-coded width), which would account for this case if I'm correct. What do you think, @archilex?

[archilex]

That sounds like good approach to me...Hopefully that would better handle alignment with the collapse button as well.

Feel free to take it from here. I'm off to bed 🥱

[zepfietje]

Good night!

I'm very busy as well (look at that long list of issues and PRs 😅).
Going to mark this as draft again, and let me know if you find the time to work on it in the meantime before I get to this. 🙂

[archilex]

@zepfietje Morning! I'll work on this today when I get out of some meetings.

[archilex]

@zepfietje

Hopefully I implemented the alternative version the way you were thinking, but here's a detailed 🤣 summary of what I came up with.

Basically, by going with the second approach of removing --collapsed-sidebar-width, however this has it's own set of tradeoffs. First, here's what it looks like in Chrome/Safari:

The first thing you'll notice above is that the icons are now aligned with the the collapse button above. That's progress.

Unfortunately, this does NOT carry over to Firefox:

The reason for this seems to be the way Chrome/Safari overlay scrollbars compared to Firefox. In Chrome and Safari when a width isn't set, scrollbars will expand the right padding allowing the container to expand:

But Firefox eats into the original padding.

In fact, because of the way Firefox renders the scrollbar, it also means you have to keep the original fix of conditionally applying items-center on nav because without it, the small icon issue comes back in Firefox:

Something else to be aware of in Safari/Chrome, is that since the scrollbar expands the padding, it expands it into the <main> container.

Safari/Chrome (notice the smaller padding on the left side of main:

Firefox (since it eats into the sidebar padding the main padding is maintained):

That said, this is really only an issue in Chrome/Safari at widths less than max-w-7xl since beyond that width the main padding increases such that the difference is barely visible. It's there, but not as noticable:

And the reason this happens at all is that since <aside> needs to stay fixed so that it doesn't scroll with the main container, we have to maintain lg:ps-[--collapsed-sidebar-width] on the main so it has the correct spacing. What this mean is that in Chrome/Safari <aside> is bascially overlapping <main> by the width of the scrollbar. Unfortuantely, I don't think you can calculate the padding of <main> based on the width of <aside> without JS.

In summary, the advantage of this version is that in Chrome/Safari you get aligned collapse and nav buttons.

Hopefully this research helps out a bit.

[zepfietje]

Thanks for the detailed findings, @archilex!

Have you found a way to make the collapsed sidebar expand on Firefox?

The <main> padding issue could be solved by making the sidebar sticky instead of fixed and letting CSS handle spacing instead of manually applying the sidebar width as start-padding on <main>. We would then lose the ability to animate the sidebar when opening/closing though. However, since these animations are quite glitchy in some spaces still, I'd consider this a legit option. Do you know if the Firefox issue would disappear too when going this route?

[archilex]

@zepfietje

Sounds good. I'll test out using sticky and see how it goes.

What do you mean by "Have you found a way to make the collapsed sidebar expand on Firefox?" It was expanding for me in Firefox...was it not before?

[archilex]

@zepfietje I've looked into this and I don't think sticky is going to work.

The good news is, as you said, using sticky and removing start-padding does fix the issue with the padding on <main>

But there are several downsides:

1. Using sticky doesn't change anything on Firefox. Even with sticky, firefox is not expanding like chrome/safari. You would still have to use items-center to make it expand past its container. Basically firefox always overlaps the container and never expands it even if there's nothing stopping it from expanding. (using flex-shrink-0 on the icon doesn't help either)

2. On mobile we are hiding the sidebar with -translate-x-full. This of course moves the sidebar off-screen, but with sticky it seems that the width of the off-screen sidebar container is always maintained and now main doesnt flow to fill the width of the screen. There are ways to mitgate this like conditionally changing sticky to fixed when it's off screen or hiding it but that doesn't seem ideal.

3. There is an issue when debugbar is open that when scrolling vertically, it pushes the sticky sidebar off the page. This is only a problem when debugbar is open, so in production this won't be an issue, BUT you will receive SOOOOO many discord helps and github issues because of this:

4. And related to that, chrome, safari, and firefox all behave differently when scrolling past the window:

sticky.mov

5. And of course you lose all your animations.

In summary,

1. Firefox is horrible,
2. the original fix of items-center is always needed

Look forward to your insight and ideas.

[zepfietje]

Thanks again, @archilex!

1. This is what I meant with "expanding on Firefox". I feel like there should be a way to work around it though... I'd have to look into it a little deeper.
2. Sidebar should probably be fixed on mobile?
3. Hmm, isn't this an issue with Debugbar rather than Filament..? But yeah, agree we'd receive many questions about it.
4. This is caused by the overscroll-behavior we explicitly set to prevent the main content from overscrolling while the sidebar doesn't when scrolling the main content on Chrome. Yet another reason to move to sticky (as we can then remove that property and it should overscroll just fine). That's actually the original reason why I investigated using sticky during the v3 redesign.

[archilex]

@zepfietje

1. Figured out that Firefox needs overflow-y-scroll to behave like Chrome/Safari. overflow-y-auto has a bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1800438. This fixes that issue, and also means we can get rid of items-center

1a. However, this obviously mean that now ugly scroll lines get rendered on all browsers even when the content doesn't need to be scrolled.

2. Fixed works for me.
3. Yes, this is a debugbar problem, but you're going to get a lof of feedback on this probably. Your call.
4. Removing overscroll unfortunately has no effect on Firefox. The sidebar continues to scroll past the container.

[zepfietje]

1. Ow wow, good catch, @archilex! Yeah 1a is the ugly side effect that came to mind immediately... Any other way to work around it?
2. Perfect.
3. Since you've encountered the Debugbar problem, have you found an easy way to fix it by changing some styling on the debug bar? If so, that would be a quick PR (hopefully).
4. That's intended. What I meant is that the main content should also overscroll. Could you test unsetting overscroll-y-none in base.blade.php and see if the sidebar overscrolls evenly with the main content?

[archilex]

@zepfietje

Update: We can use overflow-y-auto together with scrollbar-gutter: stable to make this work without the scroll lines when the content doesn't need to scroll. 🎉

I believe that resolves the major firefox issues.

3. I'd have to take a look at debugbar in the near future, but I can add to my todo list.

4. unsetting overscroll-y-none has no effect on firefox. It still breaks and scrolls vertically past the top bar

If you're ok with the debugbar problem and the vertical scrolling problem on firefox then I can push these changes

[zepfietje]

Nice!

Does scrollbar-gutter: stable have any side effects?

Also, do we still need to move to sticky then? If not, we might still want to do it in the future but at least it's not urgent.

Could you share a screen recording of the overscroll issue on Firefox with sticky?

[archilex]

@zepfietje

Unfortunately, just using scrollbar-gutter: stable alone doesn't solve the issue since it sidebar has a fixed width. In fact, it triggers the small icons automatically since the fixed width means the scrollbar can't expand out.

Therefore if you want to go that route we'll have to use sticky (fixed for mobile).

As for side effects, besides the debugbar and firefox vertical scroll issue (video below) I don't see any other issues at all. I've tested this:

1. Safari/Chrome/Firefox
2. sidebarCollapsibleOnDesktop, sidebarFullyCollapsibleOnDesktop, topNavigation, normal
3. Open and closed configurations on all the above situations
4. Responsive and desktop on all the above situations

I have not tested on a mobile device.

I'm going to go ahead and push these changes so you can see what's involved and we can refactor as needed since the lg:sticky can't get applied in some cases so I had to add it manually.

Overscroll

overscroll.mov

[zepfietje]

Thanks again, @archilex.

I think I'm happy to move to sticky, though I'd like to get @danharrin's and @awcodes' thoughts on this too as we'll lose the transition (which was still buggy in some cases).

As for the Firefox overscroll issue that's pretty interesting, like, why does the main content not overscroll when you scroll past the bottom while it does for the sidebar? 🤔

[danharrin]

Im fine to ditch the transition if it means this is fixed

[awcodes]

I'm fine with no transitions anywhere. LOL.

[archilex]

@zepfietje

Doing some comparissons, what I'm seeing is that in Firefox, when sticky is used, a container will overscroll on any side that reaches the boundaries of the viewport. So in the case of the sidebar, since aside has h-screen and touches both the top and bottom boundaries, you get overscrolling on both sides. In fact, if you change h-screen to some set height less than the viewport, then the overscroll will go away when scrolling towards the bottom. This is also the reason why the topbar doesn't overscroll when you scroll to the bottom - it's bottom edge doesn't reach the bottom viewport boundary.

I can even replicate this by with the top bar by making the browser very short so that the viewport is smaller than the height of the top bar. It will make the topbar overscroll just like the sidebar.

The only issue I've found on bugzilla is this, but I'm not sure it's related: https://bugzilla.mozilla.org/show_bug.cgi?id=1772626

The problem is that this behavior is different than chrome/safari. In Chrome/safari overscroll applies to the entire page. Unles you have overriden then overscroll behavior, chrome/safari will just overscroll the entire page.

I'm not sure what solution there is. I will say however that currently, the opposite is happening on our demo site. If you go there in firefox and scroll up (like pull to refresh), the body will overscroll but the sidebar will stay fixed. This is a separate issue, but I bring it up because if no one has brought this up as a bug, then they might not bring this up either.

As for the transitions, I added them back on mobile...so should I remove them there too?

Update: so I was playing around with calc(100vh - 1px) to see if backing off the bottom boundary would be enough to fix it, but Firefox actually seems to be doing some sort of internal calculation depending on 1) how much you try to scroll up 2) how FAR you try to scroll up, and 3) how HARD you scroll up as well. 🤦‍♂️ So calc(100vh - 30px) seems to prevent this on just a normal gentle scroll, but you need calc(100vh - 60px) to prevent it on a FAR scroll, and then calc(100vh - 100px) to prevent it on a forcefull scroll!!! Obviously, trying to hack around it like this won't work.

[zepfietje]

Oh damn, I didn't notice the issue that was solved with the overscroll class on Chromium actually still exists in some way on Firefox... Honestly, I'm fine with the remaining issue after this PR then, because it isn't any more broken than it currently is.

Transitions on mobile are fine.

Obviously, trying to hack around it like this won't work.

Yeah... let's just give up on trying to fix that issue; should be fixed in the browser.

[archilex]

@zepfietje

Works for me. Anything else I need to do? Suggestions on a better way to add those lg:sticky classes?

[danharrin]

Reverting, this is causing issues with flickering between page loads

Screen.Recording.2023-09-10.at.00.07.03.mov

[archilex]

Hmmm…I’ll take a look

[archilex]

Which browser did you see that flickering on? I'm not getting it on chrome/firefox/safari on Mac.

flickering.mov

[archilex]

Nevermind...it happens when ->sidebarCollapsibleOnDesktop() isn't enabled. I'll take a look.