Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[Modal] Touch Scrolling (dropdown, content, focussed input) fixed #273

Merged

Conversation

lubber-de
Copy link
Member

@lubber-de lubber-de commented Nov 30, 2018

Description

This PR fixes lots of issues if touchscroll is not working in modals on mobile devices anymore.
All components within modals which are supposed to be scrollable (for example dropdowns) by touchmove were not working because the touch events were cancelled. This behaviour was invented because of background scrolling issues when a modal was opened. I now simply check if the touch event occurs within the modal (were it is wanted) and only cancel the event, when it is outside.
This PR also fixes lost background scrolling if a modal with focussed input was closed having an open mobile keyboard.

Testcase

To test this, i have prepared a little mobile test suite. Just follow the short mentioned steps there
Open the following links in a real mobile browser to reproduce the issues and fixes

Fixed Version

http://jsfiddle.net/q3ptLkz6/13/show

Unfixed Version

http://jsfiddle.net/q3ptLkz6/12/show

Closes

Semantic-Org/Semantic-UI#6633
Semantic-Org/Semantic-UI#6675
Semantic-Org/Semantic-UI#6611
Semantic-Org/Semantic-UI#6449 (was already partly fixed in FUI)
Semantic-Org/Semantic-UI#6509 (duplicate of 6449)
Semantic-Org/Semantic-UI#6477 (duplicate of 6449)
Semantic-Org/Semantic-UI#6656 (was already working in FUI, but the change fixes it in SUI aswell)

Probably also fixes

Semantic-Org/Semantic-UI#6686 (testcase is missing)
Semantic-Org/Semantic-UI#4089

@lubber-de lubber-de added type/bug Any issue which is a bug or PR which fixes a bug lang/javascript Anything involving JavaScript state/awaiting-reviews Pull requests which are waiting for reviews tag/sui-issue Taken from an existing Issue/PR of SUI labels Nov 30, 2018
@lubber-de lubber-de added this to the 2.7.x milestone Nov 30, 2018
Copy link
Member

@y0hami y0hami left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@ColinFrick ColinFrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@y0hami y0hami merged commit f7c0229 into fomantic:develop Dec 3, 2018
@y0hami y0hami removed state/awaiting-reviews Pull requests which are waiting for reviews labels Dec 3, 2018
@lubber-de lubber-de deleted the fix/6675/mobile_scroll_focussed_input_modal branch December 3, 2018 11:14
@lubber-de lubber-de modified the milestones: 2.7.x, 2.7.0 Dec 6, 2018
@keshmir
Copy link

keshmir commented Dec 14, 2018

Hi @hammy2899, I just tried verifying this with latest FUI and unfortunately the issue is still there. To reproduce using Chrome you can follow these steps:

  1. Use an iPad (or Microsoft surface or Chrome Desktop emulating iPad Pro) and go here: https://fomantic-ui.com/modules/modal.html
  2. Scroll to the bottom to the section titled "Scrolling Content"
  3. Click on "Run Code"
  4. Observe that you are not able to scroll the page using touch. (in chrome your cursor turns into a circle, try to use it to scroll like a finger and it does not work).

The above has been broken since SUI 2.3+. SUI 2.2.14 works well.

@lubber-de
Copy link
Member Author

@keshmir This PR is part of the upcoming 2.7.0 release. The fomantic website still runs 2.6.4.

@keshmir
Copy link

keshmir commented Dec 14, 2018

Oh thanks @lubber-de for pointing this out. I am a newbie! :)

Sorry if this is an obvious question but is there a way for me to get a build with this PR included in order to verify my software? Any pointer appreciated.

@lubber-de
Copy link
Member Author

@keshmir I updated the fiddle with the latest module version containing this PR.
Please try it on a real mobile device:
http://jsfiddle.net/q3ptLkz6/13/show

@lubber-de
Copy link
Member Author

You can also include this in your site (include after semantic.js from FUI and not the old SUI files)
https://cdn.jsdelivr.net/gh/fomantic/fomantic-ui@develop/src/definitions/modules/modal.js

@y0hami
Copy link
Member

y0hami commented Dec 14, 2018

@keshmir as @lubber-de pointed out you could use jsDelivr or you could clone the repo and checkout the develop branch and build it yourself with gulp.

Remember that the develop branch is an early version of the next release so may contain bugs.

@keshmir
Copy link

keshmir commented Dec 14, 2018

Thanks guys for the tips. @lubber-de I have verified your fiddle page on an iPad Pro using both Safari and Chrome and it works as expected.

@y0hami
Copy link
Member

y0hami commented Dec 14, 2018

@keshmir If you want to see if features are released or are coming in the next release you can check what milestone the issue/PR is in. We try to label and milestone everything accordingly 😉

@lubber-de lubber-de added the device/mobile Any issues relating to mobile devices label Dec 18, 2018
This was referenced Dec 21, 2018
@smartm0use
Copy link

smartm0use commented May 2, 2019

Hello,
if you open FIXED jsfiddle, using a mobile device view of the browser, are you able to scroll the page down using the touch simulation?

It seems to be still a problem on touchmove event, as I reported here some months ago.

@y0hami
Copy link
Member

y0hami commented May 2, 2019

@smartm0use What device are you testing this on? I just tried this on my Pixel and it works fine.

@lubber-de
Copy link
Member Author

@smartm0use Chrome indeed seems to have some issues when simulating the touch pointer. But using the mousewheel once (!) activated the touchmove pointer simulation afterwards for me.
But as this fix is working on real mobile devices, i don't think there is something we need to do in FUI, rather than google fixing their touch simulation....

@smartm0use
Copy link

smartm0use commented May 2, 2019

I see and I agree. Anyway this happens to me:

  • Real mobile devices with your jsfiddles: scrolling works;
  • Browser emulated devices with any site: scrolling works;
  • Browser emulated devices with your jsfiddles: scrolling doesn't work;

So, what could be wrong with your jsfiddles that scrolling with emulated touch of browser (both Chrome and Firefox) it doesn't work?

@lubber-de
Copy link
Member Author

lubber-de commented May 2, 2019

I guess it's the extra html which the jsfiddle is rendering on top of our jsfiddle code.(at least the top sticky 'results' menu

@smartm0use
Copy link

I'm asking because I have the same problem with the app I'm developing and I can't figure out why it doesn't work on browser emulation and I'm struggling with building and deploying it to try every time on a real device...

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
device/mobile Any issues relating to mobile devices lang/javascript Anything involving JavaScript tag/sui-issue Taken from an existing Issue/PR of SUI type/bug Any issue which is a bug or PR which fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants