-
Notifications
You must be signed in to change notification settings - Fork 2k
Disable zoom for login page #106160
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
Merged
Merged
Disable zoom for login page #106160
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Disabling zoom for webpages is generally frowned upon as zooming is often considered an accessibility feature. I suggest we avoid this.
iOS only performs the zoom-on-input-focus behavior for inputs with font sizes beneath 16px. Previous font size changes were made in #97374 to avoid the zoom as well. I suggest we pursue this approach instead.
From inspecting the current element styles, the font size is surprisingly calculated to be 14px, even though the Sass attempts to set it as 16px. @alshakero do you have ideas as to what is causing this
remmiscalculation?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.
For your average webpage, I agree. For an application interface (especially inside a mobile app), it's unusual to be able to zoom in. If users complain about accessibility issues, we should solve that the same way we would for any other application accessibility problem – increase the font size or fix the layout.
The 16px threshold is an implementation detail that can change at any time – the fix from 9 months ago already doesn't work, so I'm reluctant to rely on something that can break again at any time.
If this is an issue, IMHO we should adopt the system text sizes per https://webkit.org/blog/3709/using-the-system-font-in-web-content/
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.
My personal opinion is that it is not that clear cut. The line between application interface and document can be pretty blurry. Yes, some scenarios could be resolved with better design; other times, accessibility features are truly the most relevant and effective solution. There are a lot of disparate use cases for accessibility features.
Not a "hill I'm doing to die on" for this particular circumstance, but I still think it is misguided if alternatives exist.
Fair. It is unfortunate that (seemingly) a Sass utility intended to calculate
rembased on explicit pixel values is not working as expected.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.
Agreed – I'm going to merge this PR, but if we can get this reliably fixed by other means I'm happy to revert these changes.
This screen is many folks' first interaction with the app, and right now it appears broken so I want to solve that problem as quickly as we're able.
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.
The back and forth continues! While it looks like the intention was to only disable zoom on the login screen, this actually disabled zoom throughout Calypso.
I'm re-enabling zoom here, but only for MSD, which should hopefully continue to satisfy the issue this PR was trying to fix.
#113337
MSD uses the core components, which ensure on mobile devices the font size is >= 16px so the zoom-on-focus thing doesn't happen. But I can only be sure we use core components on MSD, that's why it's enabled there.
Fwiw I think this statement is too sweeping. While we may feel the elements on the page are large enough, users need the ability to zoom in up to 400%. They may be partially sighted, or perhaps only temporarily partially sighted. It's not a black and white thing of either a screen reader user, or a fully sighted user. There's lots in between.