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

Implement transformer in optimizer for media, sizes, and heights #4439

Closed
westonruter opened this issue Mar 24, 2020 · 16 comments · Fixed by #4482
Closed

Implement transformer in optimizer for media, sizes, and heights #4439

westonruter opened this issue Mar 24, 2020 · 16 comments · Fixed by #4482
Labels
Changelogged Whether the issue/PR has been added to release notes. Optimizer WS:UX Work stream for UX/Front-end
Milestone

Comments

@westonruter
Copy link
Member

westonruter commented Mar 24, 2020

Feature description

Currently SSR is blocked when an AMP page contains AMP components that make use of the attributes sizes, heights, or media. While the AMP Optimizer does not currently implement a transformer for this, there is a transformation which can be made as noted in Future Work of Server-side rendering for AMP caches:

Handle rewriting of media, sizes and heights attributes into CSS.

Rewriting sizes attribute

<amp-img id="img1" sizes="(min-width: 320px) 320px, 100vw"></amp-img>

Server-side inserted CSS

<style i-amp-layout-style>
#img1 {
  width: 100vw;
}
@media (min-width: 320px) {
  #img1 {
    width: 320px;
  }
}
</style>

Rewriting heights attribute

<amp-img id="img1" heights="(min-width: 320px) 50%, 200px"></amp-img>

Server-side inserted CSS

<style i-amp-layout-style>
#img1 > i-amphtml-sizer {
  height: 200px;
}
@media (min-width: 320px) {
  #img1 > i-amphtml-sizer {
    height: 50%;
  }
}
</style>

Note that media can be transformed just by moving the attribute into a CSS rule. This involved ensuring that the element has an ID to target in a selector.

Related #4213


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation brief

QA testing instructions

Demo

Changelog entry

@westonruter westonruter added this to the v1.6 milestone Mar 24, 2020
@westonruter westonruter changed the title Implement transformer in optimizer for sizes and heights Implement transformer in optimizer for media, sizes, and heights Mar 25, 2020
@westonruter westonruter modified the milestones: v1.6, v1.5.1 Mar 26, 2020
@westonruter
Copy link
Member Author

See ampproject/amphtml#27468 for validator to allow style[i-amp-layout-style].

@schlessera
Copy link
Collaborator

Note that media can be transformed just by moving the attribute into a CSS rule. This involved ensuring that the element has an ID to target in a selector.

Unless I totally misunderstand, this does not make sense as to how CSS works.

The media attribute on an AMP component does the following:

Most AMP elements support the media attribute. The value of media is a media query. If the query does not match, the element is not rendered and its resources and potentially its child resources will not be fetched. If the browser window changes size or orientation, the media queries are re-evaluated and elements are hidden and shown based on the new results.

Just moving the media attribute into CSS provides us with a condition under which to do something, but we still have to define what that do something actually means.

I would assume we'd want to hide the element when the media query is matched. The question then is how to best hide it so that:

  • we minimize side effects and conflicts with other CSS;
  • we actually get performance benefits like not loading the needed image/video resources at all.

I'll experiment with a simple display: none for now to see what the effects will be. However, I might need to actually make that !important to make sure it happens even on elements that have a different display property set in other parts of the CSS.

@schlessera
Copy link
Collaborator

schlessera commented Mar 30, 2020

Given that !important is not allowed in AMP, we might need to use something else to bump specificity in this case. I don't think just having the ID of the element will be enough.

@schlessera
Copy link
Collaborator

Using display: none seems to generally work:

  • The image is only displayed when the media query is satisfied, and
  • only the images that are displayed initially are part of the initial page load
  • when the window is resized to satisfy a different media query, the other image is loaded and shown instead

The question now is what a meaningful specificity would be here to ensure that something like a display: inline-block won't override the image visibility.

Or would it be preferable to use multiple ways of hiding the image at the same time (display, visibility, position, ...) to ensure the "hack" works as expected?

@westonruter, @sebastianbenz thoughts?

@sebastianbenz
Copy link

Could we simply negate the media query?

@media not ($EXISTING_MEDIA_QUERY) {
   display: none;
}

Disclaimer: I'm not a CSS expert so there might be cases I'm missing.

@westonruter
Copy link
Member Author

westonruter commented Mar 31, 2020

Disclaimer: I'm not a CSS expert so there might be cases I'm missing.

You are too humble. I didn't know you could negate media queries in this way 😄

But apparently the syntax is like so:

@media not all and ($EXISTING_MEDIA_QUERY) {
   display: none;
}

Reasoning by Simon Sapin at https://www.quirksmode.org/blog/archives/2012/11/what_the_hells.html#c15586:

For some obscure reason, not is only allowed in the grammar with a media type:

http://www.w3.org/TR/css3-mediaqueries/#syntax

So the "correct" syntax is @media not all and (grid). Works in current stables of Firefox, Chromium, and Opera:

http://dabblet.com/gist/4081213

We (CSSWG) are aware of this. I hope we can fix it in MQ4 and allow not (grid). Discussion starts here:

http://lists.w3.org/Archives/Public/www-style/2012Oct/0658.html

@sebastianbenz
Copy link

Thanks for working out the correct syntax!

@schlessera would it be possible for you to port back the test cases for the new attributes to AMP Optimizer?

@schlessera
Copy link
Collaborator

@sebastianbenz Yes, sure. The test cases only will create a failing PR, though...

@schlessera
Copy link
Collaborator

Hmm, after the negation, the actual page does not work anymore. Both versions of an image are shown, instead of only the one that matches the media query. The media query doesn't seem to do anything in that form.

Some more investigation needed...

@schlessera
Copy link
Collaborator

The negated media query itself seems to work as expected: https://codepen.io/schlessera/pen/dyoryPx

It must be the generated HTML/CSS the optimizer produces, then...

@schlessera
Copy link
Collaborator

It seems to be the double parentheses around the media query, when it already had parentheses to start with.

So, this works:
@media not all and (min-width: 650px)
This doesn't work (which seems silly):
@media not all and ((min-width: 650px))

It seems that there can't ever be two levels of parentheses, the logic has to be done with and and or only.

I think we should be good with just not adding parentheses here, but I have to admit I'm not a CSS expert either.

So, we'd have the following changes.

media="(min-width: 650px)" => @media not all and (min-width: 650px)
media="screen and (min-width: 650px)" => @media not all and screen and (min-width: 650px)

That still looks correct to me...

@schlessera
Copy link
Collaborator

schlessera commented Mar 31, 2020

Well, that would have been too easy, right?

The following does not work:

@media not all and screen and (min-width: 650px)

So, I'll change the logic now to check whether the trimmed value starts with a parenthesis. If yes, assume we start with a feature and prepend all and. If not, assume we start with a type and don't prepend all and.

In the above case, that would then produce:

@media not screen and (min-width: 650px)

Hopefully that will be correct enough. Next step would be writing a full media query parser, I'm afraid...

@sebastianbenz
Copy link

..and here we go down the rabbit hole...

What about @media not screen and (min-width: 650px)? In that case we'd need to drop the not, no?

I extracted a list of valid media queries from the official test suite. This is probably a good start to check whether we cover all features.

@schlessera
Copy link
Collaborator

From looking at the valid media queries and the spec, it seems that support for an existing not might be the only thing that is still missing.

I'll add a check for that.

@schlessera
Copy link
Collaborator

Okay, I went another route. Using the spec and your test data, I built a regex: https://regex101.com/r/Xctler/1

This looks like it might handle all the cases (at least the ones we know of), and it adds semantic knowledge about the individual building blocks. The logic can then act on these building blocks, instead of doing arbitrary string manipulations.

@schlessera
Copy link
Collaborator

Ah, the test data does not take comma-separated media queries into account.

Because of the specific behavior of the not operator, which acts on the entire query, not a single feature, the entire media query has to be considered.

So, the following query:

@media not screen and (width:50px), printer and (width: 100px)

would be computed in the following way (not acting on the entire thing):

@media not ( screen and (width:50px), printer and (width: 100px) )

@westonruter westonruter modified the milestones: v1.5.1, v1.5.2 Mar 31, 2020
@westonruter westonruter modified the milestones: v1.5.2, v1.6 Apr 1, 2020
@westonruter westonruter modified the milestones: v1.6, Sprint 27 Apr 15, 2020
@kmyram kmyram modified the milestones: Sprint 27, v1.6 May 27, 2020
@westonruter westonruter removed their assignment Jul 13, 2020
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Jul 17, 2020
@kmyram kmyram added the WS:UX Work stream for UX/Front-end label Aug 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes. Optimizer WS:UX Work stream for UX/Front-end
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants