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.

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

Jump to bottom

🚀 🚀 🚀 amp-carousel ssr: allow render without JS 🚀 🚀 🚀 #37646

Merged
merged 10 commits into from
Feb 16, 2022
Merged

🚀 🚀 🚀 amp-carousel ssr: allow render without JS 🚀 🚀 🚀 #37646

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
9b43eca
amp-carousel ssr: allow render without JS
samouri Feb 11, 2022
a2c5200
lint. scrollslide working
samouri Feb 11, 2022
a179c90
rcebulko feedback
samouri Feb 15, 2022
a2351b6
further fixes
samouri Feb 15, 2022
2729c29
hyphen-case setProperty and remove Set
samouri Feb 16, 2022
ac9c39b
fix test
samouri Feb 16, 2022
d8c7703
add an extra test
samouri Feb 16, 2022
6a24d47
moar fix
samouri Feb 16, 2022
4dd1547
solve lowercase vendor bug.
samouri Feb 16, 2022
ebd69a0
even moar test fixes
samouri Feb 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
80 changes: 47 additions & 33 deletions examples/compiler-hydration.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
<title>Hydration Test</title>
<meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1">
<script async custom-element="amp-fit-text" src="https://cdn.ampproject.org/v0/amp-fit-text-0.1.js"></script>
<!-- TODO: make lazy server work with .css files. -->
<link rel="stylesheet" href="/extensions/amp-carousel/0.1/amp-carousel.css" />
<link rel="stylesheet" href="/dist/v0.css" />
<script async custom-element="amp-carousel" src="https://cdn.ampproject.org/v0/amp-carousel-0.1.js"></script>
<style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
<script async src="https://cdn.ampproject.org/v0.js"></script>
Expand All @@ -29,12 +32,20 @@
Unlike most AMPHTML pages, this file's components should all be rendered.
-->

<h1><code>amp-layout</code> with a 4:1 responsive ratio </h1>
<amp-layout layout="responsive" width="4" height="1" class="i-amphtml-layout-responsive i-amphtml-layout-size-defined" i-amphtml-layout="responsive" i-amphtml-ssr="">
<h1><code>client render: amp-layout</code> with a 4:1 responsive ratio </h1>
<amp-layout layout="responsive" width="4" height="1">
<svg viewBox="0 0 100 25">
<circle cx="50%" cy="50%" r="15%" stroke="black" stroke-width="3"></circle>
Sorry, your browser does not support inline SVG.
</svg>
</amp-layout>

<h1><code>server render: amp-layout</code> with a 4:1 responsive ratio </h1>
<amp-layout class="i-amphtml-layout-responsive i-amphtml-layout-size-defined i-amphtml-element" height=1 i-amphtml-layout=responsive i-amphtml-ssr layout=responsive width=4>
<i-amphtml-sizer slot="i-amphtml-svc" style="padding-top: 25%;"></i-amphtml-sizer>
<div class="i-amphtml-fill-content">
<svg viewBox="0 0 100 25">
<circle cx="50%" cy="50%" r="15%" stroke="black" stroke-width="3"></circle>
<circle cx="50%" cy="50%" r="15%" stroke=black stroke-width=3></circle>
Sorry, your browser does not support inline SVG.
</svg>
</div>
Expand All @@ -58,48 +69,51 @@ <h1><code>amp-fit-text</code> with short text</h1>

<h1><code>client render: carousel-0.1 type=slides</code></h1>
<amp-carousel type="slides" width="400" height="300" controls loop>
<amp-img src="https://picsum.photos/400/300?img=1" width="400" height="300"></amp-img>
<amp-img src="https://picsum.photos/400/200?img=2" width="400" height="300"></amp-img>
<amp-img src="https://picsum.photos/400/200?img=3" width="400" height="300"></amp-img>
<amp-img src="https://picsum.photos/400/200?img=4" width="400" height="300"></amp-img>
<amp-img src="https://picsum.photos/400/200?img=5" width="400" height="300"></amp-img>
<img src="https://picsum.photos/400/300?img=1" width="400" height="300"></img>
<img src="https://picsum.photos/400/200?img=2" width="400" height="300"></img>
<img src="https://picsum.photos/400/200?img=3" width="400" height="300"></img>
<img src="https://picsum.photos/400/200?img=4" width="400" height="300"></img>
<img src="https://picsum.photos/400/200?img=5" width="400" height="300"></img>
</amp-carousel>

<h1><code>server render: carousel-0.1 type=slides</code></h1>
<amp-carousel class="i-amphtml-layout-fixed i-amphtml-layout-size-defined i-amphtml-slidescroll i-amphtml-carousel-has-controls" controls height=300 i-amphtml-layout=fixed i-amphtml-ssr loop style="width: 400px; height: 300px;" type=slides width=400>
<div aria-live=polite class="i-amphtml-slides-container i-amphtml-slidescroll-no-snap" tabindex="-1">
<div class="i-amphtml-slide-item"><amp-img class="amp-carousel-slide" height=300 src="https://picsum.photos/400/300?img=1" width=400></amp-img></div>
<div class="i-amphtml-slide-item"><amp-img class="amp-carousel-slide" height=300 src="https://picsum.photos/400/200?img=2" width=400></amp-img></div>
<div class="i-amphtml-slide-item"><amp-img class="amp-carousel-slide" height=300 src="https://picsum.photos/400/200?img=3" width=400></amp-img></div>
<div class="i-amphtml-slide-item"><amp-img class="amp-carousel-slide" height=300 src="https://picsum.photos/400/200?img=4" width=400></amp-img></div>
<div class="i-amphtml-slide-item"><amp-img class="amp-carousel-slide" height=300 src="https://picsum.photos/400/200?img=5" width=400></amp-img></div>
<amp-carousel class="i-amphtml-layout-fixed i-amphtml-layout-size-defined i-amphtml-slidescroll i-amphtml-carousel-has-controls i-amphtml-element" controls height="300" i-amphtml-layout="fixed" i-amphtml-ssr loop style="width: 400px; height: 300px" type="slides" width="400">
<div aria-live="polite" class="i-amphtml-slides-container i-amphtml-slidescroll-no-snap" tabindex="-1">
<div class="i-amphtml-slide-item i-amphtml-slide-item-show">
<img class="amp-carousel-slide" height="300" src="https://picsum.photos/400/300?img=1" width="400" />
</div>
<div class="i-amphtml-slide-item"><img class="amp-carousel-slide" height="300" src="https://picsum.photos/400/200?img=2" width="400"></div>
<div class="i-amphtml-slide-item"><img class="amp-carousel-slide" height="300" src="https://picsum.photos/400/200?img=3" width="400"></div>
<div class="i-amphtml-slide-item"><img class="amp-carousel-slide" height="300" src="https://picsum.photos/400/200?img=4" width="400"></div>
<div class="i-amphtml-slide-item"><img class="amp-carousel-slide" height="300" src="https://picsum.photos/400/200?img=5" width="400"></div>
</div>
<div aria-disabled=false class="amp-carousel-button amp-carousel-button-prev" role=button tabindex=0 title="Previous item in carousel (5 of 5)"></div>
<div aria-disabled=false class="amp-carousel-button amp-carousel-button-next" role=button tabindex=0 title="Next item in carousel (2 of 5)"></div>
<div aria-disabled="false" class="amp-carousel-button amp-carousel-button-prev" role="button" tabindex="0" title="Previous item in carousel (5 of 5)"></div>
<div aria-disabled="false" class="amp-carousel-button amp-carousel-button-next" role="button" tabindex="0" title="Next item in carousel (2 of 5)"></div>
</amp-carousel>

<h1><code>client render: carousel-0.1 type=scrollable</code></h1>
<amp-carousel width="300" height="100">
<amp-img src="https://picsum.photos/300/400?img=1" width="120" height="100"></amp-img>
<amp-img src="https://picsum.photos/300/400?img=2" width="120" height="100"></amp-img>
<amp-img src="https://picsum.photos/300/400?img=3" width="120" height="100"></amp-img>
<amp-img src="https://picsum.photos/300/400?img=4" width="120" height="100"></amp-img>
<amp-img src="https://picsum.photos/300/400?img=5" width="120" height="100"></amp-img>
<amp-img src="https://picsum.photos/300/400?img=6" width="120" height="100"></amp-img>
<amp-img src="https://picsum.photos/300/400?img=7" width="120" height="100"></amp-img>
<img src="https://picsum.photos/300/400?img=1" width="120" height="100">
<img src="https://picsum.photos/300/400?img=2" width="120" height="100">
<img src="https://picsum.photos/300/400?img=3" width="120" height="100">
<img src="https://picsum.photos/300/400?img=4" width="120" height="100">
<img src="https://picsum.photos/300/400?img=5" width="120" height="100">
<img src="https://picsum.photos/300/400?img=6" width="120" height="100">
<img src="https://picsum.photos/300/400?img=7" width="120" height="100">
</amp-carousel>

<h1><code>server render: carousel-0.1 type=scrollable</code></h1>
<amp-carousel class="i-amphtml-layout-fixed i-amphtml-layout-size-defined" height=100 i-amphtml-layout=fixed i-amphtml-ssr style="width: 300px; height: 100px;" width=300>
<amp-carousel class="i-amphtml-layout-fixed i-amphtml-layout-size-defined i-amphtml-element" height="100" i-amphtml-layout="fixed" i-amphtml-ssr style="width: 300px; height: 100px" width="300">
<div class="i-amphtml-scrollable-carousel-container" tabindex="-1">
<amp-img class="amp-carousel-slide amp-scrollable-carousel-slide" height=100 src="https://picsum.photos/300/400?img=1" width=120></amp-img>
<amp-img class="amp-carousel-slide amp-scrollable-carousel-slide" height=100 src="https://picsum.photos/300/400?img=2" width=120></amp-img>
<amp-img class="amp-carousel-slide amp-scrollable-carousel-slide" height=100 src="https://picsum.photos/300/400?img=3" width=120></amp-img>
<amp-img class="amp-carousel-slide amp-scrollable-carousel-slide" height=100 src="https://picsum.photos/300/400?img=4" width=120></amp-img>
<amp-img class="amp-carousel-slide amp-scrollable-carousel-slide" height=100 src="https://picsum.photos/300/400?img=5" width=120></amp-img>
<amp-img class="amp-carousel-slide amp-scrollable-carousel-slide" height=100 src="https://picsum.photos/300/400?img=6" width=120></amp-img><amp-img class="amp-carousel-slide amp-scrollable-carousel-slide" height=100 src="https://picsum.photos/300/400?img=7" width=120></amp-img>
<img class="amp-carousel-slide amp-scrollable-carousel-slide" height="100" src="https://picsum.photos/300/400?img=1" width="120" />
<img class="amp-carousel-slide amp-scrollable-carousel-slide" height="100" src="https://picsum.photos/300/400?img=2" width="120" />
<img class="amp-carousel-slide amp-scrollable-carousel-slide" height="100" src="https://picsum.photos/300/400?img=3" width="120" />
<img class="amp-carousel-slide amp-scrollable-carousel-slide" height="100" src="https://picsum.photos/300/400?img=4" width="120" />
<img class="amp-carousel-slide amp-scrollable-carousel-slide" height="100" src="https://picsum.photos/300/400?img=5" width="120" />
<img class="amp-carousel-slide amp-scrollable-carousel-slide" height="100" src="https://picsum.photos/300/400?img=6" width="120" />
<img class="amp-carousel-slide amp-scrollable-carousel-slide" height="100" src="https://picsum.photos/300/400?img=7" width="120" />
</div>
<div aria-disabled=true class="amp-carousel-button amp-carousel-button-prev amp-disabled" role=presentation tabindex="-1" title="Previous item in carousel"></div>
<div aria-disabled=false class="amp-carousel-button amp-carousel-button-next" role=presentation tabindex=0 title="Next item in carousel"></div>
<div aria-disabled="true" class="amp-carousel-button amp-carousel-button-prev amp-disabled" role="presentation" tabindex="-1" title="Previous item in carousel"></div>
<div aria-disabled="false" class="amp-carousel-button amp-carousel-button-next" role="presentation" tabindex="0" title="Next item in carousel"></div>
</amp-carousel>
</html>
8 changes: 7 additions & 1 deletion extensions/amp-ad-network-adsense-impl/0.1/test/test-amp-ad-network-adsense-impl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
CONSENT_STRING_TYPE,
} from '#core/constants/consent-state';
import {addAttributesToElement, createElementWithAttributes} from '#core/dom';
import {camelCaseToHyphenCase} from '#core/dom/style';
import {utf8Decode, utf8Encode} from '#core/types/string/bytes';
import {toWin} from '#core/window';

Expand Down Expand Up @@ -1536,7 +1537,12 @@ describes.realWin(
};
impl.iframe = {
contentWindow: window,
style: {'visibility': 'hidden'},
style: {
'visibility': 'hidden',
setProperty: (name, value) => {
impl.iframe.style[camelCaseToHyphenCase(name)] = value;
},
},
};
win.postMessage('fill_sticky', '*');
return renderPromise.then(() => {
Expand Down
4 changes: 4 additions & 0 deletions extensions/amp-carousel/0.1/build-dom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,14 @@ export const ClassNames = {

// Generic
SLIDE: 'amp-carousel-slide',
SHOW_SLIDE: 'amp-carousel-slide',

// SlideScroll Carousel
SLIDESCROLL_CAROUSEL: 'i-amphtml-slidescroll',
SLIDE_WRAPPER: 'i-amphtml-slide-item',
SLIDES_CONTAINER: 'i-amphtml-slides-container',
SLIDES_CONTAINER_NOSNAP: 'i-amphtml-slidescroll-no-snap',
SLIDES_ITEM_SHOW: 'i-amphtml-slide-item-show',

// Scrollable Carousel
SCROLLABLE_CONTAINER: 'i-amphtml-scrollable-carousel-container',
Expand Down Expand Up @@ -229,6 +231,8 @@ function buildSlideScrollCarousel(element) {
slidesContainer.appendChild(slideWrapper);
slideWrappers.push(slideWrapper);
});
// Initialize the first slide to be shown.
slideWrappers[0]?.classList.add(ClassNames.SLIDES_ITEM_SHOW);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Move this into the forEach above? Also, why isn't it a map…

Copy link
Member Author

@samouri samouri Feb 16, 2022 •
edited

Choose a reason for hiding this comment

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

I'm confused. Why should this be part of the forEach (it should only occur to a single elem) and what should be a map?

Copy link
Member Author

@samouri samouri Feb 16, 2022 •
edited

Choose a reason for hiding this comment

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

Oh I see, instead of arr.forEach it could be arr.map.
Agreed!

Mind if I punt on that to a small PR tomorrow? (unless you have more feedback and I need to rerun CI anyway)


return {slidesContainer, slides, slideWrappers};
}
Expand Down
11 changes: 5 additions & 6 deletions extensions/amp-carousel/0.1/slidescroll.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ import {
import {CarouselControls} from './carousel-controls';

/** @const {string} */
const SHOWN_CSS_CLASS = 'i-amphtml-slide-item-show';

/** @const {number} */
const NATIVE_SNAP_TIMEOUT = 200;
Expand Down Expand Up @@ -842,7 +841,7 @@ export class AmpSlideScroll extends AMP.BaseElement {
if (this.shouldLoop_) {
setStyle(this.slideWrappers_[showIndex], 'order', loopIndex + 1);
}
this.slideWrappers_[showIndex].classList.add(SHOWN_CSS_CLASS);
this.slideWrappers_[showIndex].classList.add(ClassNames.SLIDES_ITEM_SHOW);
const owners = Services.ownersForDoc(this.element);
if (showIndex == newIndex) {
owners.scheduleLayout(this.element, this.slides_[showIndex]);
Expand Down Expand Up @@ -929,17 +928,17 @@ export class AmpSlideScroll extends AMP.BaseElement {
hideRestOfTheSlides_(indexArr) {
const {noOfSlides_} = this;
for (let i = 0; i < noOfSlides_; i++) {
if (!this.slideWrappers_[i].classList.contains(SHOWN_CSS_CLASS)) {
if (
!this.slideWrappers_[i].classList.contains(ClassNames.SLIDES_ITEM_SHOW)
) {
continue;
}
// Hide if not shown anymore
if (!indexArr.includes(i)) {
if (this.shouldLoop_) {
setStyle(this.slideWrappers_[i], 'order', '');
}
dev()
.assertElement(this.slideWrappers_[i])
.classList.remove(SHOWN_CSS_CLASS);
this.slideWrappers_[i].classList.remove(ClassNames.SLIDES_ITEM_SHOW);
this.slides_[i].removeAttribute('aria-hidden');
}
// Pause if not the current slide
Expand Down
11 changes: 9 additions & 2 deletions extensions/amp-fit-text/0.1/test/test-amp-fit-text.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@ import {createDocument as createWorkerDomDoc} from '@ampproject/worker-dom/dist/

import {createElementWithAttributes} from '#core/dom';

import {getDeterministicOuterHTML} from '#testing/helpers';
import {
getDeterministicOuterHTML,
hypenCaseToCamelCase,
} from '#testing/helpers';

import {AmpFitText, calculateFontSize_, updateOverflow_} from '../amp-fit-text';
import {buildDom} from '../build-dom';
Expand Down Expand Up @@ -244,7 +247,11 @@ describes.realWin('amp-fit-text updateOverflow', {}, (env) => {
doc = win.document;
classToggles = {};
content = {
style: {},
style: {
setProperty(name, value) {
content.style[hypenCaseToCamelCase(name)] = value;
},
},
classList: {
toggle: (className, on) => {
classToggles[className] = on;
Expand Down
1 change: 1 addition & 0 deletions src/compiler/builders.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ function wrap(buildDom: BuildDom): BuildDom {
applyStaticLayout(element as AmpElement);
buildDom(element);
element.setAttribute('i-amphtml-ssr', '');
element.classList.add('i-amphtml-element');
};
}

Expand Down
34 changes: 19 additions & 15 deletions src/core/dom/style.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,23 @@ export function camelCaseToTitleCase(camelCase) {
return camelCase.charAt(0).toUpperCase() + camelCase.slice(1);
}

/**
* @param {string} camelCase camel cased string
* @return {string} hyphen-cased string
*/
export function camelCaseToHyphenCase(camelCase) {
const hyphenated = camelCase.replace(
/[A-Z]/g,
(match) => '-' + match.toLowerCase()
);

// For o-foo or ms-foo, we need to convert to -o-foo and ms-foo
if (vendorPrefixes.some((prefix) => hyphenated.startsWith(prefix + '-'))) {
return `-${hyphenated}`;
}
return hyphenated;
}

/**
Checks the style if a prefixed version of a property exists and returns
* it or returns an empty string.
Expand All @@ -45,15 +62,6 @@ function getVendorJsPropertyName_(style, titleCase) {
return '';
}

/**
* A small set of curated properties that are valid with setProperty.
* Within worker-dom, only `.setProperty` properly reflects into the attribute.
* If we solve that, or decide to always use setProperty, this kludge can be removed.
*
* TODO(https://github.com/ampproject/worker-dom/issues/1134)
*/
const SHOULD_USE_SET_PROPERTY = new Set(['width', 'height', 'order', 'hidden']);

/**
* Returns the possibly prefixed JavaScript property name of a style property
* (ex. WebkitTransitionDuration) given a camelCase'd version of the property
Expand Down Expand Up @@ -101,7 +109,7 @@ export function setImportantStyles(element, styles) {
const {style} = element;
for (const k in styles) {
style.setProperty(
getVendorJsPropertyName(style, k),
camelCaseToHyphenCase(getVendorJsPropertyName(style, k)),
String(styles[k]),
'important'
);
Expand All @@ -126,11 +134,7 @@ export function setStyle(element, property, value, opt_units, opt_bypassCache) {
return;
}
const styleValue = opt_units ? value + opt_units : value;
if (isVar(propertyName) || SHOULD_USE_SET_PROPERTY.has(propertyName)) {
element.style.setProperty(propertyName, styleValue);
} else {
/** @type {*} */ (element.style)[propertyName] = styleValue;
}
element.style.setProperty(camelCaseToHyphenCase(propertyName), styleValue);
samouri marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand Down
25 changes: 22 additions & 3 deletions test/unit/core/dom/test-style.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,17 @@ describes.sandboxed('DOM - style helpers', {}, (env) => {
});

it('setStyle with vendor prefix', () => {
const element = {style: {WebkitTransitionDuration: ''}};
const element = {
style: {
WebkitTransitionDuration: '',
setProperty: (name, value) => {
element.style[name] = value;
},
},
};

st.setStyle(element, 'transitionDuration', '1s', undefined, true);
expect(element.style.WebkitTransitionDuration).to.equal('1s');
expect(element.style['-webkit-transition-duration']).to.equal('1s');
});

it('setStyle with custom var', () => {
Expand Down Expand Up @@ -67,7 +75,7 @@ describes.sandboxed('DOM - style helpers', {}, (env) => {
transitionDurationImportant: '1s',
});
expect(spy).to.have.been.calledWith(
'WebkitTransitionDurationImportant',
'-webkit-transition-duration-important',
'1s',
'important'
);
Expand Down Expand Up @@ -95,6 +103,17 @@ describes.sandboxed('DOM - style helpers', {}, (env) => {
expect(st.camelCaseToTitleCase(str)).to.equal('TheQuickBrownFox');
});

it('camelCaseToHyphenCase', () => {
expect(st.camelCaseToHyphenCase('paddingTop')).to.equal('padding-top');
expect(st.camelCaseToHyphenCase('WebkitTransition')).to.equal(
'-webkit-transition'
);
expect(st.camelCaseToHyphenCase('webkitTransition')).to.equal(
'-webkit-transition'
);
expect(st.camelCaseToHyphenCase('oTransition')).to.equal('-o-transition');
});

it('removeAlphaFromColor', () => {
expect(st.removeAlphaFromColor('rgba(1, 1, 1, 0)')).to.equal(
'rgba(1, 1, 1, 1)'
Expand Down