🖍 Cleanup several unresolved amp-layout CSS states by jridgewell · Pull Request #28115 · ampproject/amphtml

jridgewell · 2020-04-29T21:43:59Z

Adds unresolved layout for intrinsic styles
Fixes specificity for unresolved fixed-height styles
Adds unresolved implicit layout responsive styles

css/ampshared.css

dreamofabear · 2020-04-29T22:09:23Z

css/ampshared.css

 /* Hide all children of non-container elements. */
 .i-amphtml-notbuilt:not(.i-amphtml-layout-container) > * ,
-[layout]:not([layout=container]):not(.i-amphtml-element) > *
+[layout]:not([layout=container]):not(.i-amphtml-element) > *,


I realized this also doesn't account for implicit container layout. Is there even a CSS selector that would match?

What defines implicit container? Is it just a lack of width and height and layout?

amphtml/src/layout.js

Lines 429 to 442 in 67815dc

// Calculate effective layout.

if (inputLayout) {

layout = inputLayout;

} else if (!width && !height) {

layout = Layout.CONTAINER;

} else if (height == 'fluid') {

layout = Layout.FLUID;

} else if (height && (!width || width == 'auto')) {

layout = Layout.FIXED_HEIGHT;

} else if (height && width && (sizesAttr || heightsAttr)) {

layout = Layout.RESPONSIVE;

} else {

layout = Layout.FIXED;

}

I don't believe we can do this without :not(:resolved)

css/ampshared.css

- Adds unresolved layout for `intrinsic` styles - Fixes specificity for unresolved `fixed-height` styles - Adds unresolved implicit layout `responsive` styles

jridgewell · 2020-05-05T01:24:30Z

Need a bundle size approval.

dreamofabear · 2020-05-05T02:10:50Z

css/ampshared.css

-[layout]:not(.i-amphtml-element)
+[layout]:not(.i-amphtml-element),
+[width][height][sizes]:not([layout]):not(.i-amphtml-element),
+[width][height][heights]:not([layout]):not(.i-amphtml-element)


Please add a comment that this [width][height][sizes] and [width][height][heights] are "implicit responsive" here and below. Follow-up PR is cool too to avoid the bundle size reapproval.

dreamofabear · 2020-05-05T02:12:36Z

css/ampshared.css

-[layout]:not(.i-amphtml-element)
+[layout]:not(.i-amphtml-element),
+[width][height][sizes]:not([layout]):not(.i-amphtml-element),
+[width][height][heights]:not([layout]):not(.i-amphtml-element)


We're not handling any of the other implicit layouts either. Not sure if it's better to only fix this for implicit responsive or leave them all unfixed.

Consider leaving a note/TODO about this at least.

fixed, fixed-height, fluid and container are the missing ones. fluid isn't documented on https://amp.dev/documentation/guides-and-tutorials/learn/amp-html-layout/#layout, so lets ignore it.

fixed and fixed-height are implicit based on height and width only. Unfortunately, that's very generic, and there are non-amp elmeents that fit that:

width + height

amp-script > canvas

SVG > *

td

th

input

width

table

And container conflicts with basically everything.

I'm not sure we want to apply these styles to those elements.

I'll add a comment.

Ok, thanks for looking into it.

Re: ampproject#28115 (review)

Re: #28115 (review)

* Cleanup several unresolved amp-layout CSS states - Adds unresolved layout for `intrinsic` styles - Fixes specificity for unresolved `fixed-height` styles - Adds unresolved implicit layout `responsive` styles * Fix fixed-height selector * Add in implicit responsive with heights

Re: ampproject#28115 (review)

jridgewell requested review from dreamofabear and dvoytenko April 29, 2020 21:43

googlebot added the cla: yes label Apr 29, 2020

dvoytenko approved these changes Apr 29, 2020

View reviewed changes

dreamofabear reviewed Apr 29, 2020

View reviewed changes

jridgewell added 3 commits May 4, 2020 18:19

Cleanup several unresolved amp-layout CSS states

d8bca82

- Adds unresolved layout for `intrinsic` styles - Fixes specificity for unresolved `fixed-height` styles - Adds unresolved implicit layout `responsive` styles

Fix fixed-height selector

816561e

Add in implicit responsive with heights

7065541

jridgewell force-pushed the intrinsic-css branch from 910c683 to 7065541 Compare May 4, 2020 22:19

dreamofabear approved these changes May 5, 2020

View reviewed changes

jridgewell merged commit 5d859d4 into ampproject:master May 5, 2020

jridgewell deleted the intrinsic-css branch May 5, 2020 03:24

jridgewell added a commit to jridgewell/amphtml that referenced this pull request May 5, 2020

Add implicit layout comments

945fb28

Re: ampproject#28115 (review)

jridgewell mentioned this pull request May 5, 2020

Add implicit layout comments #28190

Merged

ampprojectbot added the PR use: In Beta / Experimental label May 5, 2020

jridgewell added a commit that referenced this pull request May 6, 2020

Add implicit layout comments (#28190)

e35e2fa

Re: #28115 (review)

ldoroshe pushed a commit to ldoroshe/amphtml that referenced this pull request May 8, 2020

Add implicit layout comments (ampproject#28190)

5a8218b

Re: ampproject#28115 (review)

ampprojectbot added the PR use: In Stable label May 14, 2020

westonruter mentioned this pull request May 15, 2021

🖍 Styles from ampshared.css can apply unexpectedly to img elements on “pre-valid” AMP pages #34402

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🖍 Cleanup several unresolved amp-layout CSS states#28115

🖍 Cleanup several unresolved amp-layout CSS states#28115
jridgewell merged 3 commits intoampproject:masterfrom
jridgewell:intrinsic-css

jridgewell commented Apr 29, 2020

Uh oh!

Uh oh!

Uh oh!

dreamofabear Apr 29, 2020

Uh oh!

jridgewell Apr 30, 2020

Uh oh!

dreamofabear May 1, 2020

Uh oh!

jridgewell May 4, 2020

Uh oh!

Uh oh!

jridgewell commented May 5, 2020

Uh oh!

dreamofabear May 5, 2020

Uh oh!

dreamofabear May 5, 2020

Uh oh!

jridgewell May 5, 2020 •

edited

Loading

Uh oh!

dreamofabear May 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	// Calculate effective layout.
	if (inputLayout) {
	layout = inputLayout;
	} else if (!width && !height) {
	layout = Layout.CONTAINER;
	} else if (height == 'fluid') {
	layout = Layout.FLUID;
	} else if (height && (!width \|\| width == 'auto')) {
	layout = Layout.FIXED_HEIGHT;
	} else if (height && width && (sizesAttr \|\| heightsAttr)) {
	layout = Layout.RESPONSIVE;
	} else {
	layout = Layout.FIXED;
	}

Conversation

jridgewell commented Apr 29, 2020

Uh oh!

Uh oh!

Uh oh!

dreamofabear Apr 29, 2020

Choose a reason for hiding this comment

Uh oh!

jridgewell Apr 30, 2020

Choose a reason for hiding this comment

Uh oh!

dreamofabear May 1, 2020

Choose a reason for hiding this comment

Uh oh!

jridgewell May 4, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jridgewell commented May 5, 2020

Uh oh!

dreamofabear May 5, 2020

Choose a reason for hiding this comment

Uh oh!

dreamofabear May 5, 2020

Choose a reason for hiding this comment

Uh oh!

jridgewell May 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dreamofabear May 5, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jridgewell May 5, 2020 •

edited

Loading