Resizable sidebar can overlap the save button on touch enabled device

On an adaptive or mobile device, the save button is located beneath the sidebar. Additionally, a close button is provided to close the sidebar and save your changes.

Indeed, when the screen is below 1024 pixels, there is a close button but not on a desktop with a wider screen.

This issue is not for mobile devices with small screen but for normal desktop with higher resolution with touch enabled screens.

As you can see in my second screenshot, there is no close button on higher resolution when the sidebar overlaps.

By the way, if anybody has this issue, you can always close the sidebar using the Alt-s keyboard combination when it overlaps but this is not intuitive for users.

Thank you for looking into this.

Using the debugger, I was able to observe that when the sidebar overlap occurs, the local storage variable Drupal.gin.sidebarWidth becomes invalid with NaNpx as you can see in this capture :

I'm not able to reproduce the issue! D 10.3.1 and drupal/gin 3.0.0-rc13. Let me know if other specif configuration is needed, thanks!

I am not able to reproduce the issue, as everything appears fine at my end.
Checking on version - 8.x-3.x

Thank you for trying but I'm surprised both of you cannot reproduce this bug. The problem does not show directly when you arrive on the page using the debugger with touch feature enabled. Everything is fine at that point when using a touch screen. But the problem occurs as soon you try to resize the sidebar while being in the mobile view debugger with the touch feature enabled. When resizing the sidebar, it overlaps almost instantly.

I just tested again using all latest versions and the bug is still present unfortunately under Firefox and Chromium. So from my understanding, you did not try to resize the sidebar while being in the mobile debugger with touch enabled in order to reproduce the bug.

Let me know if these details help reproducing the bug?

Thank you

Hi Leducdubleuet

I have reproduced this issue at my end and I am working on it. I will update here ASAP.

Thanks

Hello Amitnar, this is great news, let me know when I can help with testing. Thx

Hi Leducdubleuet,

I have fixed this kindly use the attached patch.

Thanks

Thank you @amitnar!

I agree that disabling the sidebar resizing on touch devices solves the problem but you forgot to keep the following :

document.addEventListener("mouseup", this.resizeEnd)

Also, your patch covers only the sidebar.js inside the "/dist/js" folder and we need to patch the same file inside the "/js" folder as well.

You will find attached a new complete patch "sidebar-resize-overlap-3463177-15.patch" to simply disable the sidebar resizing on touch devices.

I personally was not able to resize the sidebar on a touch device, it only created the current overlap bug so disabling it seems like the way to go for me.

Is disabling the sidebar resizing feature on touch devices the best approach here?

Thank you.

I modified the title and summary to make it clear this bug happens on desktop size devices with touch enabled screen.

Hi leducdubleuet,

I have fixed the resize sidebar issue for the touch-enabled screen.

find the attached patch [sidebar-resize-overlap-15791901-2.patch].

kindly check and confirm.

I tested the patch #17 and can confirm, that it solves the problem on my `browser adaptive screen testing feature`.

Indeed, the patch "sidebar-resize-overlap-15791901-2.patch" fixes the problem on touch enabled desktop devices but then the sidebar is not resizable anymore on normal desktop without touch screen. So, if we go that way, we need a switch in the code to keep it resizable without touch as well.

In the meantime, I simply use my patch in comment #15 which disables the resizing on touch enabled desktop devices.

Thank you for your time.

@leducdubleuet,

I have attached sidebar-resize-overlap-15791901-3.patch for both types of devices, but you have to check on the actual devices. Also, if you want to check on the browser simulator, you have to refresh the page when you go to use the browser simulator on the same page.

Thanks

Hello,

I can confirm the patch *sidebar-resize-overlap-15791901-3.patch* is now working on both type of screens with and without touch enabled, good job!

But while reviewing the patch, I noticed some CSS/SCSS modifications that seem unrelated to the current issue, can you confirm they are needed?

Also, shouldn't we have a check for Android as well? Or is the check needed only for iOS devices?

Thx!

@leducdubleuet,

I have attached the modified sidebar-resize-overlap-15791901-4.patch now removed CSS changes from the patch.

Thanks

@leducdubleuet,

I have attached the modified sidebar-resize-overlap-15791901-4.patch now removed CSS changes from the patch. Also I have added android device in this patch.

Thanks

I'll review this one.

I've tested this both on an iPad and on a GalaxyTab (BrowserStack) and the patch does fix the issue.
Thanks @amitnar for the good work, could you just make a MR iso of a patch?

Marking it as needs work for the MR.

I can also confirm the patch "sidebar-resize-overlap-15791901-4.patch" in comment #24 works well on desktops with or without touch enabled screen as well.

I also reviewed the patch and I suggest to rename the isIOS variable to isTouch or isMobile to reflect that it is not only for iOS devices...

Thank you very much!

@selfirian,

I have raised the MR for this.

Thanks

@amitnar Thank you for your work!

I reviewed the MR and I believe the code in "dist/js/sidebar.js" should be minified for optimization if I am not mistaken?

Also, as I said before, I think we should rename the variable "isIOS" cause it's misleading as it checks more than iOS...

Thx

@leducdubleuet

"I reviewed the MR and I believe the code in "dist/js/sidebar.js" should be minified for optimization if I am not mistaken?"
This was not modified in the originally kindly confirm if this need to minified?

@leducdubleuet,

I have raised the MR for this after the changes. Also, I have not minified "dist/js/sidebar.js" because it was not modified originally. Kindly confirm if this needs to be minified.

Thanks

@leducdubleuet None of the other JS scripts in gin are minified, and IMO that's not needed since they're minified by Drupal once aggregation is turned on, see: https://www.drupal.org/node/3305725 →

As for the name for the variable, let the maintainers decide on that.

I've checked the MR on android and apple devices and it works as expected.

There are errors in the pipeline, so moving this back to needs work 🙇

Testes patch #24, not working for us.
Desktop > 1200px, no touch, Save button and all on the right hand side are covered.