Skip to content

♻️ [Story performance] Simplify templates CSS by using attr instead of class #35861

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.

Sign up for GitHub

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

Merged
mszylkowski merged 8 commits into from
Sep 1, 2021
Merged

♻️ [Story performance] Simplify templates CSS by using attr instead of class #35861

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9613a9c
Changed classes for template attr
mszylkowski Aug 30, 2021
a4cbeb4
Simplified rules
mszylkowski Aug 30, 2021
8249c01
Merge branch 'main' of github.com:ampproject/amphtml into template_se…
mszylkowski Aug 30, 2021
8c97a11
Fixed some comments
mszylkowski Aug 30, 2021
6f0558f
Remove unused template
mszylkowski Aug 31, 2021
c5d42fb
Reverted pan demo to image
mszylkowski Aug 31, 2021
f113bfb
Removed unused const
mszylkowski Sep 1, 2021
aec7a89
Merge branch 'main' of github.com:ampproject/amphtml into template_se…
mszylkowski Sep 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion extensions/amp-story-auto-ads/0.1/amp-story-auto-ads.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ amp-story-page amp-ad[type='adsense'] {

/* TODO(ccordry) allow advertisers to opt-in to fullscreen ads. */
.i-amphtml-story-desktop-fullbleed
.i-amphtml-story-grid-template-fill
amp-story-grid-layer[template="fill"]
> amp-ad {
left: 50% !important;
right: auto !important;
Expand Down
3 changes: 1 addition & 2 deletions extensions/amp-story-panning-media/0.1/amp-story-panning-media.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,7 @@ amp-story-panning-media {
amp-story-panning-media amp-img {
backface-visibility: hidden !important;
}

.i-amphtml-story-grid-template-fill amp-story-panning-media amp-img img {
amp-story-grid-layer[template="fill"] amp-story-panning-media amp-img img {
/* override for layout=fill. This centers the image without clipping it */
position: relative !important;
display: block !important;
Expand Down
34 changes: 0 additions & 34 deletions extensions/amp-story/1.0/amp-story-grid-layer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,23 +43,6 @@ const SUPPORTED_CSS_GRID_ATTRIBUTES_SELECTOR = Object.keys(
.map((key) => `[${key}]`)
.join(',');

/**
* The attribute name for grid layer templates.
* @private @const {string}
*/
const TEMPLATE_ATTRIBUTE_NAME = 'template';

/**
* A mapping of template attribute values to CSS class names.
* @const {!Object<string, string>}
*/
export const GRID_LAYER_TEMPLATE_CLASS_NAMES = {
'fill': 'i-amphtml-story-grid-template-fill',
'vertical': 'i-amphtml-story-grid-template-vertical',
'horizontal': 'i-amphtml-story-grid-template-horizontal',
'thirds': 'i-amphtml-story-grid-template-thirds',
};
Comment on lines -56 to -61
Copy link
Copy Markdown
Contributor

@processprocess processprocess Aug 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used by in animation-presets.js. it's setStyleForPreset method might need to be updated to read attributes instead of class names.

Copy link
Copy Markdown
Contributor Author

@mszylkowski mszylkowski Aug 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, will update that. Thanks!

Copy link
Copy Markdown
Contributor Author

@mszylkowski mszylkowski Aug 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the setStyleForPreset, it doesn't seem like those checks are needed there, they are essentially removing the ...-fill class from the template if the parent of the amp-img is template="fill" but this can be done with a CSS selector that's [template="fill"]:not(.i-amphtml-story-grid-template-with-full-bleed-animation) without involving JS. Will commit these changes once I'm sure.


/**
* The attribute name for grid layer presets.
* @private @const {string}
Expand Down Expand Up @@ -112,7 +95,6 @@ export class AmpStoryGridLayer extends AmpStoryBaseLayer {
buildCallback() {
super.buildCallback();
this.applyResponsivenessPresets_();
this.applyTemplateClassName_();
this.setOwnCssGridStyles_();
this.setDescendentCssGridStyles_();
this.initializeListeners_();
Expand Down Expand Up @@ -175,7 +157,6 @@ export class AmpStoryGridLayer extends AmpStoryBaseLayer {
const height = Math.min(vh, (vw * vert) / horiz);
if (width > 0 && height > 0) {
this.getVsync().mutate(() => {
this.element.classList.add('i-amphtml-story-grid-template-aspect');
setStyles(this.element, {
'--i-amphtml-story-layer-width': px(width * this.scalingFactor_),
'--i-amphtml-story-layer-height': px(height * this.scalingFactor_),
Expand All @@ -184,21 +165,6 @@ export class AmpStoryGridLayer extends AmpStoryBaseLayer {
}
}

/**
* Applies internal CSS class names for the template attribute, so that styles
* can use the class name instead of compound
* amp-story-grid-layer[template="..."] selectors, since the latter increases
* CSS specificity and can prevent users from being able to override styles.
* @private
*/
applyTemplateClassName_() {
if (this.element.hasAttribute(TEMPLATE_ATTRIBUTE_NAME)) {
const templateName = this.element.getAttribute(TEMPLATE_ATTRIBUTE_NAME);
const templateClassName = GRID_LAYER_TEMPLATE_CLASS_NAMES[templateName];
this.element.classList.add(templateClassName);
}
}

/**
* Copies the allowlisted CSS grid styles for descendants of the
* <amp-story-grid-layer> element.
Expand Down
12 changes: 6 additions & 6 deletions extensions/amp-story/1.0/amp-story-user-overridable.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,25 +13,25 @@ amp-story-grid-layer * {
margin: 0;
}

.i-amphtml-story-grid-template-fill amp-anim img,
.i-amphtml-story-grid-template-fill amp-img img,
.i-amphtml-story-grid-template-fill amp-video video,
[template="fill"] amp-anim img,
[template="fill"] amp-img img,
[template="fill"] amp-video video,
.i-amphtml-story-grid-template-with-full-bleed-animation amp-img img {
object-fit: cover;
}

.i-amphtml-story-grid-template-vertical {
[template="vertical"] {
align-content: start;
grid-gap: 16px;
justify-content: stretch;
justify-items: start;
}

.i-amphtml-story-grid-template-vertical > * {
[template="vertical"] > * {
width: 100%;
}

.i-amphtml-story-grid-template-horizontal {
[template="horizontal"] {
align-content: stretch;
align-items: start;
grid-gap: 16px;
Expand Down
15 changes: 7 additions & 8 deletions extensions/amp-story/1.0/amp-story.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,11 +359,10 @@ amp-story-grid-layer[anchor*="right"] {
}

/**
* Prevents pre-load placeholders from pushing content offscreen.
* Prevents preload placeholders from pushing content offscreen.
*/
.i-amphtml-story-grid-template-fill amp-img img,
.i-amphtml-story-grid-template-fill amp-video video {
[template="fill"] amp-img img,
[template="fill"] amp-video video {
position: absolute !important;
}

Expand Down Expand Up @@ -416,7 +415,7 @@ amp-story-grid-layer .i-amphtml-embedded-component::after {
padding: 0 !important;
}

.i-amphtml-story-grid-template-fill > * {
[template="fill"]:not(.i-amphtml-story-grid-template-with-full-bleed-animation) > * {
bottom: 0 !important;
height: auto !important;
left: 0 !important;
Expand All @@ -426,25 +425,25 @@ amp-story-grid-layer .i-amphtml-embedded-component::after {
width: auto !important;
}

.i-amphtml-story-grid-template-vertical {
[template="vertical"] {
grid-auto-flow: row !important;
grid-template-columns: 100% !important;
}

.i-amphtml-story-grid-template-horizontal {
[template="horizontal"] {
grid-auto-flow: column !important;
grid-template-rows: 100% !important;
}

.i-amphtml-story-grid-template-thirds {
[template="thirds"] {
height: 100% !important;
grid-template-rows: 33% 33% 33% !important; /* `fr` is broken in Safari. */
grid-template-areas: "upper-third"
"middle-third"
"lower-third" !important;
}

.i-amphtml-story-grid-template-aspect {
[aspect-ratio], [preset] {
margin: auto;
width: var(--i-amphtml-story-layer-width, 100%);
height: var(--i-amphtml-story-layer-height, 100%);
Expand Down
12 changes: 0 additions & 12 deletions extensions/amp-story/1.0/animation-presets.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import {GRID_LAYER_TEMPLATE_CLASS_NAMES} from './amp-story-grid-layer';
import {StoryAnimationPresetDef} from './animation-types';
import {
calculateTargetScalingFactor,
Expand All @@ -12,8 +11,6 @@ import {userAssert} from '../../../src/log';

/** @const {string} */
const FULL_BLEED_CATEGORY = 'full-bleed';
/** @const {string} */
const FILL_TEMPLATE_LAYOUT = 'fill';
/** @const {number} */
const SCALE_HIGH_DEFAULT = 3;
/** @const {number} */
Expand Down Expand Up @@ -69,15 +66,6 @@ export function setStyleForPreset(el, presetName) {
// For full bleed animations.
if (FULL_BLEED_ANIMATION_NAMES.indexOf(presetName) >= 0) {
const parent = el.parentElement;
if (
parent.classList.contains(
GRID_LAYER_TEMPLATE_CLASS_NAMES[FILL_TEMPLATE_LAYOUT]
)
) {
parent.classList.remove(
GRID_LAYER_TEMPLATE_CLASS_NAMES[FILL_TEMPLATE_LAYOUT]
);
}
parent.classList.add(ANIMATION_CSS_CLASS_NAMES[FULL_BLEED_CATEGORY]);
}
}
Expand Down
12 changes: 0 additions & 12 deletions extensions/amp-story/1.0/test/test-amp-story-grid-layer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@ describes.realWin('amp-story-grid-layer', {amp: true}, (env) => {

storeService.dispatch(Action.SET_PAGE_SIZE, {width: 1000, height: 1000});

expect(gridLayerEl).to.have.class('i-amphtml-story-grid-template-aspect');
expect(
parseInt(
gridLayerEl.style.getPropertyValue('--i-amphtml-story-layer-width'),
Expand All @@ -96,7 +95,6 @@ describes.realWin('amp-story-grid-layer', {amp: true}, (env) => {

storeService.dispatch(Action.SET_PAGE_SIZE, {width: 1000, height: 1000});

expect(gridLayerEl).to.have.class('i-amphtml-story-grid-template-aspect');
expect(
parseInt(
gridLayerEl.style.getPropertyValue('--i-amphtml-story-layer-width'),
Expand All @@ -118,7 +116,6 @@ describes.realWin('amp-story-grid-layer', {amp: true}, (env) => {

storeService.dispatch(Action.SET_PAGE_SIZE, {width: 1000, height: 1000});

expect(gridLayerEl).to.have.class('i-amphtml-story-grid-template-aspect');
expect(
parseInt(
gridLayerEl.style.getPropertyValue('--i-amphtml-story-layer-width'),
Expand All @@ -142,15 +139,6 @@ describes.realWin('amp-story-grid-layer', {amp: true}, (env) => {
expect(gridLayerEl.getAttribute('aspect-ratio')).to.equal('69:116');
});

it('should use the responsiveness preset to change the layer aspect', async () => {
gridLayerEl.setAttribute('preset', '2021-foreground');
await buildGridLayer();

storeService.dispatch(Action.SET_PAGE_SIZE, {width: 1000, height: 1000});

expect(gridLayerEl).to.have.class('i-amphtml-story-grid-template-aspect');
Comment thread
mszylkowski marked this conversation as resolved.
});

it('should not add aspect-ratio attribute if preset passed is incorrect', async () => {
gridLayerEl.setAttribute('preset', 'wrong-preset');
await buildGridLayer();
Expand Down
30 changes: 10 additions & 20 deletions test/visual-diff/visual-tests
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,26 +266,23 @@
"name": "amp-story: Grid layer (fill)",
"viewport": {"width": 320, "height": 480},
"loading_complete_selectors": [
".i-amphtml-story-loaded",
".i-amphtml-story-grid-template-fill"
".i-amphtml-story-loaded"
]
},
{
"url": "examples/visual-tests/amp-story/amp-story-grid-layer-template-vertical.html",
"name": "amp-story: Grid layer (vertical)",
"viewport": {"width": 320, "height": 480},
"loading_complete_selectors": [
".i-amphtml-story-loaded",
".i-amphtml-story-grid-template-vertical"
".i-amphtml-story-loaded"
]
},
{
"url": "examples/visual-tests/amp-story/amp-story-grid-layer-template-horizontal.html",
"name": "amp-story: Grid layer (horizontal)",
"viewport": {"width": 320, "height": 480},
"loading_complete_selectors": [
".i-amphtml-story-loaded",
".i-amphtml-story-grid-template-horizontal"
".i-amphtml-story-loaded"
]
},
{
Expand All @@ -296,26 +293,23 @@
"[grid-area]"
],
"loading_complete_selectors": [
".i-amphtml-story-loaded",
".i-amphtml-story-grid-template-thirds"
".i-amphtml-story-loaded"
]
},
{
"url": "examples/visual-tests/amp-story/amp-story-grid-layer-presets.html",
"name": "amp-story: Grid layer anchoring with preset (vertically)",
"viewport": {"width": 250, "height": 500},
"loading_complete_selectors": [
".i-amphtml-story-loaded",
".i-amphtml-story-grid-template-aspect"
".i-amphtml-story-loaded"
]
},
{
"url": "examples/visual-tests/amp-story/amp-story-grid-layer-presets.html",
"name": "amp-story: Grid layer anchoring with preset (horizontally)",
"viewport": {"width": 400, "height": 500},
"loading_complete_selectors": [
".i-amphtml-story-loaded",
".i-amphtml-story-grid-template-aspect"
".i-amphtml-story-loaded"
]
},
{
Expand Down Expand Up @@ -397,26 +391,23 @@
"name": "amp-story: Grid layer (fill) (desktop)",
"viewport": {"width": 1440, "height": 900},
"loading_complete_selectors": [
".i-amphtml-story-loaded",
".i-amphtml-story-grid-template-fill"
".i-amphtml-story-loaded"
]
},
{
"url": "examples/visual-tests/amp-story/amp-story-grid-layer-template-vertical.html",
"name": "amp-story: Grid layer (vertical) (desktop)",
"viewport": {"width": 1440, "height": 900},
"loading_complete_selectors": [
".i-amphtml-story-loaded",
".i-amphtml-story-grid-template-vertical"
".i-amphtml-story-loaded"
]
},
{
"url": "examples/visual-tests/amp-story/amp-story-grid-layer-template-horizontal.html",
"name": "amp-story: Grid layer (horizontal) (desktop)",
"viewport": {"width": 1440, "height": 900},
"loading_complete_selectors": [
".i-amphtml-story-loaded",
".i-amphtml-story-grid-template-horizontal"
".i-amphtml-story-loaded"
]
},
{
Expand All @@ -427,8 +418,7 @@
"[grid-area]"
],
"loading_complete_selectors": [
".i-amphtml-story-loaded",
".i-amphtml-story-grid-template-thirds"
".i-amphtml-story-loaded"
]
},
{
Expand Down