ampproject / amphtml

The AMP web component framework.
https://amp.dev
Apache License 2.0
14.89k stars 3.89k forks source link

Intent to Implement: CSS @media query validation #18246

Closed Gregable closed 3 years ago

Gregable commented 6 years ago

Current behavior

The AMP Validator currently emits validation parse errors on malformed CSS in custom stylesheets (<style amp-custom> and <style amp-keyframes>). For example, publishers may see an error with attached line/column information such as:

CSS syntax error in tag 'style amp-custom' - invalid declaration.

The validator does not currently parse @media query strings, for example, the string screen and (max-width: 500px) within the CSS block @media screen and (max-width: 500px) {}.

It also does not make any assertions on the valid values for media query types (ex. screen) or media query features (ex. max-width).

Intended change

This proposed change is to invalidate documents containing parse errors in @media query strings, and validate media types and features against a whitelist of allowed values, depending on the context. See the below sections for additional detail.

Parse Errors

If there are errors generated while parsing CSS media queries, browsers will currently discard the entire encountered @media rule. The AMP Validator ignores these cases, but will begin to detect and report on them. The generated validator messages will look like:

CSS syntax error in tag 'style amp-custom' - malformed media query.

And will attach line / column information referencing the start of the media query.

Examples of invalid media queries include:

A sample of 10,000 crawler-accessible pages shows 11 amp documents (0.11%) that would become invalid if media query parsing would report a validator error.

Whitelist of valid media query types and features

The set of media query types (ex. screen) and media query features (ex. max-width) will be whitelisted by the validator.

For normal AMP documents, it is intended that all types and features with any significant use will be whitelisted, so only uncommon typos will become invalid. The current list, which will be extended after wider testing includes:

media_query_type: "all"
media_query_type: "print"
media_query_type: "screen"
media_query_type: "speech"
media_query_type: "tty"
media_query_type: "tv"
media_query_type: "projection"
media_query_type: "handheld"
media_query_type: "braille"
media_query_type: "embossed"
media_query_type: "aural"
media_query_feature: "width"
media_query_feature: "height"
media_query_feature: "aspect-ratio"
media_query_feature: "orientation"
media_query_feature: "resolution"
media_query_feature: "scan"
media_query_feature: "grid"
media_query_feature: "update"
media_query_feature: "overflow-block"
media_query_feature: "overflow-inline"
media_query_feature: "color"
media_query_feature: "color-gamut"
media_query_feature: "color-index"
media_query_feature: "display-mode"
media_query_feature: "monochrome"
media_query_feature: "inverted-colors"
media_query_feature: "pointer"
media_query_feature: "hover"
media_query_feature: "any-pointer"
media_query_feature: "any-hover"
media_query_feature: "light-level"
media_query_feature: "prefers-reduced-motion"
media_query_feature: "prefers-reduced-transparency"
media_query_feature: "prefers-contrast"
media_query_feature: "prefers-color-scheme"
media_query_feature: "scripting"
media_query_feature: "device-width"
media_query_feature: "device-height"
media_query_feature: "device-aspect-ratio"
# Other non-standard media types and features commonly seen:
media_query_type: "-sass-debug-info"
media_query_type: "device-pixel-ratio"
media_query_type: "device-pixel-ratio2"
media_query_feature: "device-pixel-ratio"
media_query_feature: "device-pixel-ratio2"
media_query_feature: "high-contrast"
media_query_feature: "--mod"
media_query_feature: "--md"
media_query_feature: "aspect-ratio"

This list assumes that min-, max- prefix variations, as well as vendor prefix variations (-o-, -moz, -ms-, -webkit-) of the above will also be considered valid.

The validation messages will look like:

CSS syntax error in tag 'style amp-custom' - disallowed media type 'foo'.
CSS syntax error in tag 'style amp-custom' - disallowed media feature 'foo'.

For AMP documents, we will whitelist any commonly observed type or feature, so the impact should be very small and limited to rare typos.

For other types of AMP documents, such as AMP4ADS or AMP4EMAIL, the whitelist will be constrained to a smaller subset.

AMP4ADS:

type: 'all'
type: 'print'
type: 'screen'
type: 'speech'
feature: 'width'
feature: 'height'
feature: 'aspect-ratio'
feature: 'orientation'
feature: 'resolution'

AMP4EMAIL: TBD

Purpose of Change Some media query types and features can be used to fingerprint user browsers representing a security and privacy risk in some contexts. Adding this functionality to manage access to media query parameters allows AMP providers to manage this risk.

Suggested Change Schedule Upon approval of this change, the AMP validator will begin emitting warnings for documents that contain unparseable media queries or use media query types or features not on the proposed whitelist. These warnings will not cause AMP documents to become invalid, but will give developers time to understand the impact of the change. Warnings will appear in all AMP validator interfaces, including Google Search Console.

No sooner than 6 weeks after warnings have been emitted, these warnings will become errors, and documents with these warnings will become invalid.

For process reference, see spec/amp-versioning-policy.md.

dreamofabear commented 6 years ago

Which media query types/features will no longer be valid?

Gregable commented 6 years ago

Anything not in the whitelist. However, any type/feature we find used with any significance on the AMP corpus will be added to the AMP whitelist. Some of the ones in that list fall in that category, such as "-sass-debug-info".

aghassemi commented 6 years ago

few older types to add: tty, tv, projection, handheld, braille, embossed, aural. otherwise LGTM

Gregable commented 6 years ago

@aghassemi I updated the first post to add those 7 to the list.

dreamofabear commented 6 years ago

LGTM

cramforce commented 6 years ago

LGTM

mrjoro commented 5 years ago

Is this I2I still active?

Gregable commented 5 years ago

Yes. However, I think it's still in need of a third LGTM to make progress?

mrjoro commented 5 years ago

We could probably consider this one grandparented in, but it doesn't hurt. :)

cc @ampproject/wg-approvers for a third pre-approval LGTM

aghassemi commented 5 years ago

3rd LGTM

Gregable commented 3 years ago

This has been implemented as a warning only. I don't anticipate we'll want to move to an error for AMP, only email, so closing this.