♻️ [Story performance] Simplify templates CSS by using attr instead o…

…f class (#35861) * Changed classes for template attr * Simplified rules * Fixed some comments * Remove unused template * Reverted pan demo to image * Removed unused const
ampproject · Sep 1, 2021 · ca8db68 · ca8db68
1 parent 1318234
commit ca8db68
Show file tree

Hide file tree

Showing 8 changed files with 25 additions and 95 deletions.
diff --git a/extensions/amp-story-auto-ads/0.1/amp-story-auto-ads.css b/extensions/amp-story-auto-ads/0.1/amp-story-auto-ads.css
@@ -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;

diff --git a/extensions/amp-story-panning-media/0.1/amp-story-panning-media.css b/extensions/amp-story-panning-media/0.1/amp-story-panning-media.css
@@ -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;

diff --git a/extensions/amp-story/1.0/amp-story-grid-layer.js b/extensions/amp-story/1.0/amp-story-grid-layer.js
@@ -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',
-};
-
 /**
  * The attribute name for grid layer presets.
  * @private @const {string}
@@ -112,7 +95,6 @@ export class AmpStoryGridLayer extends AmpStoryBaseLayer {
   buildCallback() {
     super.buildCallback();
     this.applyResponsivenessPresets_();
-    this.applyTemplateClassName_();
     this.setOwnCssGridStyles_();
     this.setDescendentCssGridStyles_();
     this.initializeListeners_();
@@ -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_),
@@ -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.

diff --git a/extensions/amp-story/1.0/amp-story-user-overridable.css b/extensions/amp-story/1.0/amp-story-user-overridable.css
@@ -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;

diff --git a/extensions/amp-story/1.0/amp-story.css b/extensions/amp-story/1.0/amp-story.css
@@ -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;
 }
 
@@ -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;
@@ -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%);

diff --git a/extensions/amp-story/1.0/animation-presets.js b/extensions/amp-story/1.0/animation-presets.js
@@ -1,4 +1,3 @@
-import {GRID_LAYER_TEMPLATE_CLASS_NAMES} from './amp-story-grid-layer';
 import {StoryAnimationPresetDef} from './animation-types';
 import {
   calculateTargetScalingFactor,
@@ -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} */
@@ -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]);
   }
 }

diff --git a/extensions/amp-story/1.0/test/test-amp-story-grid-layer.js b/extensions/amp-story/1.0/test/test-amp-story-grid-layer.js
@@ -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'),
@@ -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'),
@@ -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'),
@@ -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');
-  });
-
   it('should not add aspect-ratio attribute if preset passed is incorrect', async () => {
     gridLayerEl.setAttribute('preset', 'wrong-preset');
     await buildGridLayer();

diff --git a/test/visual-diff/visual-tests b/test/visual-diff/visual-tests
@@ -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"
       ]
     },
     {
@@ -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"
       ]
     },
     {
@@ -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"
       ]
     },
     {
@@ -427,8 +418,7 @@
         "[grid-area]"
       ],
       "loading_complete_selectors": [
-        ".i-amphtml-story-loaded",
-        ".i-amphtml-story-grid-template-thirds"
+        ".i-amphtml-story-loaded"
       ]
     },
     {