Skip to content
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

Remove maximum-scale=1 and user-scalable=no from Reader template meta viewport #3681

Merged
merged 1 commit into from
Nov 4, 2019

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Nov 4, 2019

The Reader mode templates includes maximum-scale=1 and user-scalable=no in the meta viewport. This was pointed out by @rianrietveld as being a critical accessibility issue since it prevents users from being able to zoom to read text. AMP has no requirement to have these meta viewport properties. The bare minimum AMP requires is just having device-width defined.

Note that this issue only applied to the Reader mode templates; when using Standard or Transitional modes, the meta viewport is whatever the active theme is using, as long as it has the AMP-required width=device-width viewport property (which the AMP plugin will enforce).


It used to be that AMP required these extra properties in the meta viewport. This was no longer the case as of 4 years ago: ampproject/amphtml#592. As a testament to how old the AMP plugin is, the meta viewport for the plugin was added a couple weeks before AMP removed the additional properties.

Update: See #3690 for also removing these meta viewport properties from the old v0.3 back-compat templates.

@westonruter westonruter added the Accessibility Changes that impact accessibility and need corresponding review (e.g. markup changes). label Nov 4, 2019
@westonruter westonruter added this to the v.1.4.1 milestone Nov 4, 2019
@googlebot googlebot added the cla: yes Signed the Google CLA label Nov 4, 2019
@rianrietveld
Copy link

Thanks for the quick fix @westonruter!
cc @goldenapples.

@westonruter westonruter requested a review from kienstra November 4, 2019 21:15
@westonruter westonruter force-pushed the fix/reader-template-meta-viewport-a11y branch from 207c579 to 6438f48 Compare November 4, 2019 21:32
@kienstra
Copy link
Contributor

kienstra commented Nov 4, 2019

Approved

Hi @westonruter,
Like you mentioned, it looks like it's good to avoid maximum-scale=1.

The Reader template still looks good:

read-still-valid

@westonruter
Copy link
Member Author

Merging in spite of the unrelated E2E test failure.

@westonruter westonruter merged commit 7468347 into develop Nov 4, 2019
@westonruter westonruter deleted the fix/reader-template-meta-viewport-a11y branch November 4, 2019 21:59
@westonruter
Copy link
Member Author

Cherry-picked into the 1.4 branch.

@kienstra
Copy link
Contributor

Steps To Test

  1. Select 'Reader' mode
  2. Create a new post, and preview the front-end of the AMP URL
  3. Select 'View Source'
  4. Expected: maximum-scale=1,user-scalable=no" is not present in the HTML of the document, and it's still valid AMP (with the green icon):

read-still-valid

@csossi
Copy link

csossi commented Nov 12, 2019

Verified in QA

@csossi csossi added the QA passed Has passed QA and is done label Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accessibility Changes that impact accessibility and need corresponding review (e.g. markup changes). cla: yes Signed the Google CLA QA passed Has passed QA and is done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants