From f42b1e3ecf0adff1168d3caffab1cf187665a4a5 Mon Sep 17 00:00:00 2001 From: Dima Voytenko Date: Thu, 25 Mar 2021 16:32:51 -0700 Subject: [PATCH] Bento: add container to the lightbox 1.0 (#33488) --- extensions/amp-lightbox/0.1/amp-lightbox.js | 2 +- extensions/amp-lightbox/1.0/base-element.js | 46 ++++++++++---- extensions/amp-lightbox/1.0/component.js | 12 ++-- extensions/amp-lightbox/1.0/component.type.js | 1 + .../1.0/test/test-amp-lightbox.js | 15 +++++ .../amp-lightbox/1.0/test/test-component.js | 4 ++ extensions/amp-video/1.0/video-wrapper.js | 2 +- src/preact/base-element.js | 8 ++- src/preact/slot.js | 21 +++++-- src/service/scheduler.js | 2 - test/unit/preact/test-base-element-mapping.js | 20 +++--- test/unit/preact/test-slot.js | 62 +++++++++++++++++++ 12 files changed, 158 insertions(+), 37 deletions(-) diff --git a/extensions/amp-lightbox/0.1/amp-lightbox.js b/extensions/amp-lightbox/0.1/amp-lightbox.js index cd83cc9e7554..a8090cd5ae58 100644 --- a/extensions/amp-lightbox/0.1/amp-lightbox.js +++ b/extensions/amp-lightbox/0.1/amp-lightbox.js @@ -623,7 +623,7 @@ class AmpLightbox extends AMP.BaseElement { // Unmount all children when the lightbox is closed. They will automatically // remount when the lightbox is opened again. - unmountAll(this.element); + unmountAll(this.element, /* includeSelf */ false); Services.ownersForDoc(this.element).schedulePause( this.element, diff --git a/extensions/amp-lightbox/1.0/base-element.js b/extensions/amp-lightbox/1.0/base-element.js index 5a3529f0bfc6..e23e1531742c 100644 --- a/extensions/amp-lightbox/1.0/base-element.js +++ b/extensions/amp-lightbox/1.0/base-element.js @@ -20,6 +20,7 @@ import {PreactBaseElement} from '../../../src/preact/base-element'; import {dict} from '../../../src/utils/object'; import {toggle} from '../../../src/style'; import {toggleAttribute} from '../../../src/dom'; +import {unmountAll} from '../../../src/utils/resource-container-helper'; export class BaseElement extends PreactBaseElement { /** @param {!AmpElement} element */ @@ -33,24 +34,48 @@ export class BaseElement extends PreactBaseElement { /** @override */ init() { return dict({ - 'onBeforeOpen': this.toggle_.bind(this, true), - 'onAfterClose': this.toggle_.bind(this, false), + 'onBeforeOpen': () => this.beforeOpen_(), + 'onAfterOpen': () => this.afterOpen_(), + 'onAfterClose': () => this.afterClose_(), }); } + /** @override */ + unmountCallback() { + this.removeAsContainer(); + } + /** @override */ updatePropsForRendering(props) { props['closeButtonAs'] = () => props['closeButton']; } - /** - * Toggle open/closed attributes. - * @param {boolean} opt_state - */ - toggle_(opt_state) { - this.open_ = toggleAttribute(this.element, 'open', opt_state); - toggle(this.element, this.open_); - this.triggerEvent(this.element, this.open_ ? 'open' : 'close'); + /** @private */ + beforeOpen_() { + this.open_ = true; + toggleAttribute(this.element, 'open', true); + toggle(this.element, true); + this.triggerEvent(this.element, 'open'); + } + + /** @private */ + afterOpen_() { + const scroller = this.element.shadowRoot.querySelector('[part=scroller]'); + this.setAsContainer(scroller); + } + + /** @private */ + afterClose_() { + this.open_ = false; + toggleAttribute(this.element, 'open', false); + toggle(this.element, false); + this.triggerEvent(this.element, 'close'); + + this.removeAsContainer(); + + // Unmount all children when the lightbox is closed. They will automatically + // remount when the lightbox is opened again. + unmountAll(this.element, /* includeSelf */ false); } /** @override */ @@ -72,7 +97,6 @@ BaseElement['props'] = { 'animation': {attr: 'animation', media: true, default: 'fade-in'}, 'closeButton': {selector: '[slot="close-button"]', single: true}, 'children': {passthrough: true}, - 'id': {attr: 'id'}, }; /** @override */ diff --git a/extensions/amp-lightbox/1.0/component.js b/extensions/amp-lightbox/1.0/component.js index ad3c6adfff3e..63ff4a36ee22 100644 --- a/extensions/amp-lightbox/1.0/component.js +++ b/extensions/amp-lightbox/1.0/component.js @@ -46,6 +46,8 @@ const ANIMATION_PRESETS = { const DEFAULT_CLOSE_LABEL = 'Close the modal'; +const CONTENT_PROPS = {'part': 'scroller'}; + /** * @param {!LightboxDef.Props} props * @param {{current: (!LightboxDef.LightboxApi|null)}} ref @@ -58,6 +60,7 @@ function LightboxWithRef( closeButtonAs, onBeforeOpen, onAfterClose, + onAfterOpen, ...rest }, ref @@ -77,14 +80,13 @@ function LightboxWithRef( const animationRef = useValueRef(animation); const onBeforeOpenRef = useValueRef(onBeforeOpen); const onAfterCloseRef = useValueRef(onAfterClose); + const onAfterOpenRef = useValueRef(onAfterOpen); useImperativeHandle( ref, () => ({ open: () => { - if (onBeforeOpenRef.current) { - onBeforeOpenRef.current(); - } + onBeforeOpenRef.current?.(); setMounted(true); setVisible(true); }, @@ -108,6 +110,7 @@ function LightboxWithRef( setStyle(element, 'opacity', 1); setStyle(element, 'visibility', 'visible'); tryFocus(element); + onAfterOpenRef.current?.(); }; if (!element.animate) { postVisibleAnim(); @@ -147,7 +150,7 @@ function LightboxWithRef( animation.cancel(); } }; - }, [visible, animationRef, onAfterCloseRef]); + }, [visible, animationRef, onAfterCloseRef, onAfterOpenRef]); return ( mounted && ( @@ -159,6 +162,7 @@ function LightboxWithRef( part="lightbox" contentClassName={classes.content} wrapperClassName={classes.wrapper} + contentProps={CONTENT_PROPS} role="dialog" tabIndex="0" onKeyDown={(event) => { diff --git a/extensions/amp-lightbox/1.0/component.type.js b/extensions/amp-lightbox/1.0/component.type.js index 3111355beee4..1d63c6ec5a9c 100644 --- a/extensions/amp-lightbox/1.0/component.type.js +++ b/extensions/amp-lightbox/1.0/component.type.js @@ -27,6 +27,7 @@ var LightboxDef = {}; * scrollable: (boolean), * initialOpen: (boolean), * onBeforeOpen: (function|undefined), + * onAfterOpen: (function|undefined), * onAfterClose: (function|undefined), * }} */ diff --git a/extensions/amp-lightbox/1.0/test/test-amp-lightbox.js b/extensions/amp-lightbox/1.0/test/test-amp-lightbox.js index 1c5736f2b084..f339c1936cc4 100644 --- a/extensions/amp-lightbox/1.0/test/test-amp-lightbox.js +++ b/extensions/amp-lightbox/1.0/test/test-amp-lightbox.js @@ -19,6 +19,7 @@ import {ActionTrust, DEFAULT_ACTION} from '../../../../src/action-constants'; import {htmlFor} from '../../../../src/static-template'; import {poll} from '../../../../testing/iframe'; import {toggleExperiment} from '../../../../src/experiments'; +import {whenCalled} from '../../../../testing/test-helper'; describes.realWin( 'amp-lightbox:1.0', @@ -87,6 +88,9 @@ describes.realWin( } it('should open with default action', async () => { + env.sandbox.stub(element, 'setAsContainerInternal'); + env.sandbox.stub(element, 'removeAsContainerInternal'); + expect(element.hasAttribute('open')).to.be.false; expect(element.hasAttribute('hidden')).to.be.true; @@ -105,9 +109,18 @@ describes.realWin( expect(contentEls[0].textContent).to.equal('Hello World'); expect(eventSpy).to.be.calledOnce; + + await whenCalled(element.setAsContainerInternal); + const scroller = element.shadowRoot.querySelector('[part=scroller]'); + expect(scroller).to.exist; + expect(element.setAsContainerInternal).to.be.calledWith(scroller); + expect(element.removeAsContainerInternal).to.not.be.called; }); it('should open and close', async () => { + env.sandbox.stub(element, 'setAsContainerInternal'); + env.sandbox.stub(element, 'removeAsContainerInternal'); + expect(element.hasAttribute('open')).to.be.false; expect(element.hasAttribute('hidden')).to.be.true; @@ -139,6 +152,8 @@ describes.realWin( expect(openSpy).to.be.calledOnce; expect(closeSpy).to.be.calledOnce; + expect(element.setAsContainerInternal).to.not.be.called; + expect(element.removeAsContainerInternal).to.be.calledOnce; }); }); } diff --git a/extensions/amp-lightbox/1.0/test/test-component.js b/extensions/amp-lightbox/1.0/test/test-component.js index 809d41be8333..45f8ef4bba8e 100644 --- a/extensions/amp-lightbox/1.0/test/test-component.js +++ b/extensions/amp-lightbox/1.0/test/test-component.js @@ -43,6 +43,10 @@ describes.sandboxed('Lightbox preact component v1.0', {}, () => { const closeButton = buttons.first().getDOMNode(); expect(closeButton.getAttribute('aria-label')).to.equal('Close the modal'); expect(closeButton.textContent).to.equal(''); + + // Scroller. + const scroller = wrapper.getDOMNode().querySelector('[part=scroller]'); + expect(scroller).to.exist; }); it('renders custom close button', () => { diff --git a/extensions/amp-video/1.0/video-wrapper.js b/extensions/amp-video/1.0/video-wrapper.js index 5bc6c8f25c92..bbc10e816640 100644 --- a/extensions/amp-video/1.0/video-wrapper.js +++ b/extensions/amp-video/1.0/video-wrapper.js @@ -157,7 +157,7 @@ function VideoWrapperWithRef( }, [readyDeferred]); const pause = useCallback(() => { - readyDeferred.promise.then(() => playerRef.current.pause()); + readyDeferred.promise.then(() => playerRef.current?.pause()); }, [readyDeferred]); const requestFullscreen = useCallback(() => { diff --git a/src/preact/base-element.js b/src/preact/base-element.js index 71d3418f8422..6d3c36f03593 100644 --- a/src/preact/base-element.js +++ b/src/preact/base-element.js @@ -1147,12 +1147,16 @@ function parsePropDefs(Ctor, props, propDefs, element, mediaQueryProps) { let value; if (def.passthrough) { devAssert(Ctor['usesShadowDom']); - value = []; + // Use lazy loading inside the passthrough by default due to too many + // elements. + value = []; } else if (def.passthroughNonEmpty) { devAssert(Ctor['usesShadowDom']); + // Use lazy loading inside the passthrough by default due to too many + // elements. value = element.getRealChildNodes().every(IS_EMPTY_TEXT_NODE) ? null - : []; + : []; } else if (def.attr) { value = element.getAttribute(def.attr); if (def.media && value != null) { diff --git a/src/preact/slot.js b/src/preact/slot.js index b91f5eb180f8..1fe6151e65c2 100644 --- a/src/preact/slot.js +++ b/src/preact/slot.js @@ -16,6 +16,7 @@ import * as Preact from './index'; import {CanPlay, CanRender, LoadingProp} from '../core/contextprops'; +import {Loading} from '../core/loading-instructions'; import {pureDevAssert as devAssert} from '../core/assert'; import { loadAll, @@ -26,6 +27,8 @@ import {rediscoverChildren, removeProp, setProp} from '../context'; import {useAmpContext} from './context'; import {useEffect, useLayoutEffect, useRef} from './index'; +const EMPTY = {}; + /** * @param {!Element} element * @param {string} name @@ -34,7 +37,7 @@ import {useEffect, useLayoutEffect, useRef} from './index'; */ export function createSlot(element, name, props) { element.setAttribute('slot', name); - return ; + return ; } /** @@ -46,7 +49,7 @@ export function createSlot(element, name, props) { export function Slot(props) { const ref = useRef(/** @type {?Element} */ (null)); - useSlotContext(ref); + useSlotContext(ref, props); useEffect(() => { // Post-rendering cleanup, if any. @@ -60,8 +63,10 @@ export function Slot(props) { /** * @param {{current:?}} ref + * @param {!JsonObject=} opt_props */ -export function useSlotContext(ref) { +export function useSlotContext(ref, opt_props) { + const {'loading': loading} = opt_props || EMPTY; const context = useAmpContext(); // Context changes. @@ -98,13 +103,17 @@ export function useSlotContext(ref) { const slot = ref.current; devAssert(slot?.nodeType == 1, 'Element expected'); - // TODO(#31915): switch to `mount`. - execute(slot, loadAll); + // Mount children, unless lazy loading requested. If so the element should + // use `BaseElement.setAsContainer`. + if (loading != Loading.LAZY) { + // TODO(#31915): switch to `mount`. + execute(slot, loadAll); + } return () => { execute(slot, unmountAll); }; - }, [ref]); + }, [ref, loading]); } /** diff --git a/src/service/scheduler.js b/src/service/scheduler.js index 23c8c8e0eb02..139ef0dca7d2 100644 --- a/src/service/scheduler.js +++ b/src/service/scheduler.js @@ -18,7 +18,6 @@ import {LayoutPriority} from '../layout'; import {READY_SCAN_SIGNAL} from './resources-interface'; import {VisibilityState} from '../visibility-state'; import {containsNotSelf, hasNextNodeInDocumentOrder, isIframed} from '../dom'; -import {devAssert} from '../log'; import {getServiceForDoc, registerServiceBuilderForDoc} from '../service'; import {removeItem} from '../utils/array'; @@ -135,7 +134,6 @@ export class Scheduler { * @param {!Element=} opt_scroller */ setContainer(container, opt_scroller) { - devAssert(!opt_scroller || container.contains(opt_scroller)); if (this.containerMap_.has(container)) { return; } diff --git a/test/unit/preact/test-base-element-mapping.js b/test/unit/preact/test-base-element-mapping.js index c77ec474fad0..81f0588dab2b 100644 --- a/test/unit/preact/test-base-element-mapping.js +++ b/test/unit/preact/test-base-element-mapping.js @@ -873,7 +873,7 @@ describes.realWin('PreactBaseElement', spec, (env) => { expect(children).to.have.lengthOf(1); const child = children[0]; expect(child.type).to.equal(Slot); - expect(child.props).to.deep.equal({}); + expect(child.props).to.deep.equal({loading: 'lazy'}); expect(element.querySelector('b').slot).to.equal(''); }); @@ -888,7 +888,7 @@ describes.realWin('PreactBaseElement', spec, (env) => { expect(children).to.have.lengthOf(1); const child = children[0]; expect(child.type).to.equal(Slot); - expect(child.props).to.deep.equal({}); + expect(child.props).to.deep.equal({loading: 'lazy'}); }); it('should re-render on empty content', async () => { @@ -902,7 +902,7 @@ describes.realWin('PreactBaseElement', spec, (env) => { expect(children).to.have.lengthOf(1); const child = children[0]; expect(child.type).to.equal(Slot); - expect(child.props).to.deep.equal({}); + expect(child.props).to.deep.equal({loading: 'lazy'}); }); it('should ignore service children mutations', async () => { @@ -956,7 +956,7 @@ describes.realWin('PreactBaseElement', spec, (env) => { expect(children).to.have.lengthOf(1); const child = children[0]; expect(child.type).to.equal(Slot); - expect(child.props).to.deep.equal({}); + expect(child.props).to.deep.equal({loading: 'lazy'}); }); it('should pass children as undefined when empty', async () => { @@ -978,7 +978,7 @@ describes.realWin('PreactBaseElement', spec, (env) => { expect(children).to.have.lengthOf(1); const child = children[0]; expect(child.type).to.equal(Slot); - expect(child.props).to.deep.equal({}); + expect(child.props).to.deep.equal({loading: 'lazy'}); }); it('should ignore service children mutations', async () => { @@ -1136,7 +1136,7 @@ describes.realWin('PreactBaseElement', spec, (env) => { expect(children).to.have.lengthOf(1); const child = children[0]; expect(child.type).to.equal(Slot); - expect(child.props).to.deep.equal({}); + expect(child.props).to.deep.equal({loading: 'lazy'}); expect(element.querySelector('#child1').slot).to.equal(''); expect(element.querySelector('#child2').slot).to.equal(''); expect(element.textContent).to.contain('text (should be passed through)'); @@ -1163,7 +1163,7 @@ describes.realWin('PreactBaseElement', spec, (env) => { // New child. expect(child.type).to.equal(Slot); - expect(child.props).to.deep.equal({}); + expect(child.props).to.deep.equal({loading: 'lazy'}); expect(element.querySelector('#child3').slot).to.equal(''); // No changes. @@ -1189,7 +1189,7 @@ describes.realWin('PreactBaseElement', spec, (env) => { // New child. expect(child.type).to.equal(Slot); - expect(child.props).to.deep.equal({}); + expect(child.props).to.deep.equal({loading: 'lazy'}); expect(element.textContent).to.contain('more text'); // No changes. @@ -1213,7 +1213,7 @@ describes.realWin('PreactBaseElement', spec, (env) => { expect(children).to.have.lengthOf(1); const child = children[0]; expect(child.type).to.equal(Slot); - expect(child.props).to.deep.equal({}); + expect(child.props).to.deep.equal({loading: 'lazy'}); // No changes. expect(element.querySelector('#child2').slot).to.equal(''); @@ -1240,7 +1240,7 @@ describes.realWin('PreactBaseElement', spec, (env) => { expect(children).to.have.lengthOf(1); const child = children[0]; expect(child.type).to.equal(Slot); - expect(child.props).to.deep.equal({}); + expect(child.props).to.deep.equal({loading: 'lazy'}); // No changes, except for ordering expect(child1.slot).to.equal(''); diff --git a/test/unit/preact/test-slot.js b/test/unit/preact/test-slot.js index dede1786e293..270bd0168ba7 100644 --- a/test/unit/preact/test-slot.js +++ b/test/unit/preact/test-slot.js @@ -203,6 +203,68 @@ describes.realWin('Slot mount/unmount', {}, (env) => { }); }); + describe('with Shadow DOM and loading=lazy slot', () => { + let shadowRoot; + let child1, child2; + + before(function () { + if (!Element.prototype.attachShadow) { + this.skipTest(); + } + }); + + beforeEach(() => { + child1 = createAmpElement({slot: 'slot1'}); + child2 = createAmpElement({slot: 'slot1'}); + host.append(child1, child2); + + shadowRoot = host.attachShadow({mode: 'open'}); + setIsRoot(shadowRoot, true); + + wrapper = mount( + +
+ +
+ , + {attachTo: shadowRoot} + ); + }); + + function createAmpElement(attrs) { + const element = createElementWithAttributes(doc, 'amp-element', attrs); + return stubAmpElement(element); + } + + it('should load AMP elements on mount', () => { + clock.runAll(); + expect(child1.ensureLoaded).to.not.be.called; + expect(child2.ensureLoaded).to.not.be.called; + expect(child1.unmount).to.not.be.called; + expect(child2.unmount).to.not.be.called; + expect(child1.pause).to.not.be.called; + expect(child2.pause).to.not.be.called; + }); + + it('should unmount AMP elements on unmount', () => { + wrapper.unmount(); + clock.runAll(); + expect(child1.unmount).to.be.calledOnce; + expect(child2.unmount).to.be.calledOnce; + expect(child1.pause).to.not.be.called; + expect(child2.pause).to.not.be.called; + }); + + it('should pause AMP elements when playable changes', () => { + wrapper.setProps({playable: false}); + clock.runAll(); + expect(child1.pause).to.be.calledOnce; + expect(child2.pause).to.be.calledOnce; + expect(child1.unmount).to.not.be.called; + expect(child2.unmount).to.not.be.called; + }); + }); + describe('with Light DOM', () => { let child1Ref, child2Ref;