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

Discontinue excluding excessive CSS by default on Native mode sites #2326

Open
westonruter opened this issue May 15, 2019 · 3 comments

Comments

Projects
None yet
3 participants
@westonruter
Copy link
Member

commented May 15, 2019

Currently when there is too much CSS, the first stylesheet that exceeds the 50KB limit is excluded from being included on the page. This leads to pages that appear broken. While prioritizing the stylesheets to concatenate will help a lot (#2322), we should also consider whether we should exclude stylesheets by default. This is particularly important when activating AMP on a theme that has more than 50KB of CSS even without the admin bar, or when a certain combination of blocks causes more than 50KB of CSS to be added to the page after tree shaking.

We currently have this toggle to automatically-accept validation errors, which results in excessive CSS being excluded:

image

What if an excessive_css validation error was a special case which instead defaulted to “New Rejected” status? On a Native mode site, this would result in the amp attribute being removed from the html element, and essentially serving an unmarked AMP page which would be able to take advantage of all the performance benefits of AMP except for being served from the AMP cache.

Should this auto-reject for excessive_css apply to Transitional mode as well or just Native mode? I'm thinking no, as the non-AMP version would be a better default that includes all the CSS. The primary problem today is Native mode sites that exclude CSS.

If we do this, it will be important to show a notice that too much CSS is on the page: #1801.

@westonruter westonruter added this to the v1.2 milestone May 15, 2019

@westonruter westonruter added the CSS label May 15, 2019

@felixarntz

This comment has been minimized.

Copy link
Collaborator

commented May 15, 2019

I'm not sure on this. I think #2322 should definitely have more priority and be implemented (and even released) first so that we can get some real-world usage on whether it's sufficient for most cases or not.

The issue here would help mitigate an initial "everything is broken" reaction, but prioritization may already cover that sufficiently. Given the lack of consistency of treating excessive_css differently from other validation errors, I'd prefer to only do that if we still feel a need for it after the other improvements.

@amedina

This comment has been minimized.

Copy link
Member

commented May 16, 2019

This is important for any case where there is an excessive CSS validation error in Native mode. In Transitional, the site has the fall back of redirection to canonical in the presence of errors. In Native, with the assumption of auto sanitization by default, the only option in such cases is a broken site. If such case ever happens, however unlikely, this approach would provide a much better experience.

@westonruter

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2019

Also, in Transitional mode, I think redirection to the non-AMP version should be prevented when the user is logged-in. Otherwise, it would be annoying to see what the AMP version looks like, even though it has too much CSS.

@westonruter westonruter modified the milestones: v1.2, v1.3 Jun 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.