octobercms / october

Self-hosted CMS platform based on the Laravel PHP Framework.
https://octobercms.com/
Other
11.03k stars 2.21k forks source link

Backend tabs and tables no longer horizontally scrollable #4037

Closed joseph-d closed 5 years ago

joseph-d commented 5 years ago

Description:

Since the latest 444-446 build update, it is no longer possible to horizontally scroll backend tabs and tables on mobile view.

Steps To Reproduce:

Go to and backend view with tabs or tables that extend beyond the mobile viewport and try to swipe to scroll them horizontally as was previously possible prior to the Build 444 update. Swiping no longer results in the tabs or tables being scrolled.

LukeTowers commented 5 years ago

@ayumihamsaki can you look into this? @joseph-d could you provide more information on the browser used?

joseph-d commented 5 years ago

The issue is present on the following browsers that I use:

Mobile: Chrome 71.0.3578.99 on Android 8.0.0; SM-G955F Build/R16NW Firefox 64.0.1 on Android 8.0.0; SM-G955F Build/R16NW

Desktop in mobile emulation: Chrome Version 71.0.3578.98 (Official Build) (64-bit) on Ubuntu Linux 18.10

I have also just tested it on Firefox Quantum 64.0 on Ubuntu 18.10 Desktop using a mobile responsive width and tabs and tables do still appear to scroll horizontally in Build 446 but I suspect this is just because Firefox on Desktop isn't doing mobile emulation properly.

If I change the storm files in /modules/system/assets/ui/ back to Build 443 versions, horizontal scrolling of tabs and tables works properly on all browsers listed above.

w20k commented 5 years ago

I also confirm bug, tested it on the:

I think it could be reproduced with the chrome device mode.

LukeTowers commented 5 years ago

@ayumihamsaki @ayumihamsaki2 @Teranode could you guys look into this and see if you can find the issue? @joseph-d are you able to try incrementally applying changes from the 446 version of storm.css to find the issue?

w20k commented 5 years ago

@LukeTowers found the cause, will make PR tomorrow.

ghost commented 5 years ago

Sorry for late reply, only now checking my github. I will let w20k do this one as found the cause.

w20k commented 5 years ago

@LukeTowers, will wait for CSS minifier revert, and then push.

w20k commented 5 years ago

@joseph-d, @ayumihamsaki, I'd need your help with testing on mobile devices. I've tested tables with Chrome Emulator, only. Didn't have tabs examples.

So, In a few days, hopefully, will make a PR. Still, dealing with CssMinifier.

joseph-d commented 5 years ago

@w20k Sure, I'm ready to test as soon as the code is ready.

AugmentBLU commented 5 years ago

Is it possible to disable the tab switching when swiping if tabs are now scrollable? Reason is that if we use something like responsive tables in a tab and attempt to scroll horizontally, the tab switches which makes things very difficult to edit.

joseph-d commented 5 years ago

Tabs were previously scrollable before the latest update stopped them from scrolling. At that time (< build 444) scrolling tables within scrollable tabs worked perfectly fine without making the tabs scroll too. The two things worked independently. Therefore, I think that the issue mentioned by @AugmentBLU must've been introduced by the recent fix, although it doesn't seem to be happening for me.

Just a thought: Scrolling of tables was only previously possible by swiping the table header. Swiping the actual table didn't do anything, which made sense. If this fix is allowing the whole table to be swiped on some browsers then that could signify a difference in the way it worked prior to build 444.

w20k commented 5 years ago

@AugmentBLU could you give us more details on browser type/version/device & etc?

As, @joseph-d, I haven't had the same issue with table swipe while testing.

AugmentBLU commented 5 years ago

@joseph-d The swiping has been in place for a while I believe.

@w20k On my Samsung S9, if viewing a tab and the tab has any overflowing content or responsive elements that allow horizontal scrolling, the JavaScript swipe is being triggered and switching tabs so without great care, I cannot scroll horizontally.

It's due to data-slidable within the tabs. Removing this stops the issue but it's part of the core and would be best if we could some how disable this or take it out as it's counter intuitive if tabs are scrollable. Why would we need swiping between tabs if they're scrollable?

https://github.com/octobercms/october/blob/fb2aa1730cc08e8563908b54b70e942bd2988f5e/modules/backend/widgets/form/partials/_section.htm#L26

I don't see any option to disable this on the backend, even though there is an option available within the JS itself.

https://github.com/octobercms/october/blob/7e6800ea6e00637619a0d10eef3eaa82d5c4b255/modules/system/assets/ui/js/tab.js#L30

joseph-d commented 5 years ago

Strangely enough, I have never experienced the tabs swiping in this way. Perhaps it isn't supported by my browser.

But, yes, I agree that there's really no point in having swipable tabs if the tabs themselves are scrollable. In my opinion, it's much more intuitive to scroll and select a tab than it is to have tabs (which might not even be visible) being swipable, especially when tables (which often appear within tabs) need to be scrollable with a swipe gesture!

AugmentBLU commented 5 years ago

That's exactly it @joseph-d - I have a table within a tab and has quite a few columns so using responsive tables allows for horizontal scrolling but with the swiping JS being active, as soon as you attempt to scroll the table, it switches tab.

w20k commented 5 years ago

I guess we have to disable it by default and add an option to enable it if needed. Partially already done)

LukeTowers commented 5 years ago

I'm fine with disabling it by default and just leaving it like that for now until someone complains.

w20k commented 5 years ago

@AugmentBLU could you verify will swiping be disabled if you set data-slidable="false"? Will make a PR.

LukeTowers commented 5 years ago

Or if we just remove data-slidable?

w20k commented 5 years ago

@LukeTowers that's an option, but for me, it feels like this option would be lost in the wild 😄

LukeTowers commented 5 years ago

What do you mean?

w20k commented 5 years ago

@LukeTowers my bad forgot about tab.md that it's already documented. Thought it wasn't. Will make PR to address this issue.

w20k commented 5 years ago

@AugmentBLU made a PR. Could you re-test and comment on PR. Thanks! :)