-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ensure the page can be pinch to zoomed #155
Conversation
I tested this with browserstack, which allows pinch to zoom gestures via a trackpad. @36degrees @hannalaakso I would appreciate you double checking my findings if you have the time. |
Results of my testing in the lab: |
@hannalaakso can you give more detail on 1. what URL you were testing 2. what happened to make it seem broken. This change uses the same meta tag as the Design System so I'm confused why pinch to zoom wouldnt work with this change. |
Sorry for the terseness, I was rushing off into a meeting 😞 I tested the pinch to zoom on https://tdt-documentation.london.cloudapps.digital. I've since managed to install Chrome on the new iOS devices in the lab and pinch to zoom works on those so the issue on iPhone 5s could be a hardware issue on older iPhones or something to do with iOS 12 (although @NickColley's findings above suggest this shouldn't be an issue > iOS 10 - I'll try to find another iOS device to test on if we want to dig into this further). ✅ Safari on iPhone 5s 12.1 * Pinch to zoom works on https://design-system.service.gov.uk/ |
@hannalaakso I think those findings confirm what I found, since this pull request has not been deployed yet I would expect some devices to not allow pinch to zoom right now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NickColley I'm clearly having an off day. Now tested the latest code with the following and it looks good:
✅ Safari on iPhone 5s 12.1
✅ Chrome on iPhone 5s 12.1
✅ Safari on iPhone 8 13.2
✅ Chrome on iPhone 8 13.2
✅ Chrome on iPad 3 13.2
✅ Safari on iPad 3 13.2
✅ Samsung Internet 10.1 on Galaxy S8
✅ Firefox 68 on on Galaxy S8
I checked that pinch to zoom works and that the navigation fixed element doesn't end up overlapping when you pinch to zoom.
These are the same meta tags that we use on the Design System website itself and on the GOV.UK Frontend page template so I think we're fairly confident that this will work correctly for our users 👍
OK I'll add a changelog entry and we can get this merged, thanks for the extensive testing Hanna |
I've tested in latest versions of iOS and Android. - iOS v10 onwards ignores disabling pinch to zoom - Android does not ignore disabling pinch to zoom, although the user can override this. I have not been able to introduce any negative impact for removing pinch to zoom, the interface scales proportionally and does not obstruct anything. So given that iOS already ignores this I think we should allow this for all users. This is also a WCAG accessibility requirement. Fixes #110
6512627
to
4abcc49
Compare
We have intentionally tried to disable pinch to zoom in the past based around the sticky navigation, but this is an accessibility issue.
This pull request updates this decision to pinch to zooming, which for the most part is how it's working at the moment anyway since a lot of devices ignore blocking pinch to zoom.
I've tested in latest versions of iOS and Android.
I have not been able to introduce any negative impact for allowing pinch to zoom, the interface scales proportionally and does not obstruct anything. So given that iOS already ignores this I think we should allow this for all users.
This is also a WCAG accessibility requirement.
Fixes #110