From 6bef0d2c5799cf8863155b22db792b859f7216d3 Mon Sep 17 00:00:00 2001 From: Dima Voytenko Date: Fri, 6 Sep 2019 13:47:10 -0700 Subject: [PATCH 1/6] Retargetting of slots --- src/amp-react-carousel.js | 10 +++++-- src/react-compat-base-element.js | 50 +++++++++++++++++++++++++++++--- 2 files changed, 54 insertions(+), 6 deletions(-) diff --git a/src/amp-react-carousel.js b/src/amp-react-carousel.js index edf7bd1..9400a0a 100644 --- a/src/amp-react-carousel.js +++ b/src/amp-react-carousel.js @@ -217,8 +217,14 @@ const AmpReactCarousel = ReactCompatibleBaseElement(AmpCarousel, { }, }, children: { - 'arrowNext': '[arrow-next]', - 'arrowPrev': '[arrow-prev]', + 'arrowNext': { + selector: '[arrow-next]', + props: {retarget: true}, + }, + 'arrowPrev': { + selector: '[arrow-prev]', + props: {retarget: true}, + }, 'children': '*', }, }); diff --git a/src/react-compat-base-element.js b/src/react-compat-base-element.js index 11280d8..9cee616 100644 --- a/src/react-compat-base-element.js +++ b/src/react-compat-base-element.js @@ -17,6 +17,11 @@ import AmpElementFactory from './amp-element.js'; import devAssert from './dev-assert.js'; +const { + useEffect, + useRef, +} = React; + /** * ReactCompatibleBaseElement is a compatibility wrapper around AMP's * BaseElement. It takes a Component to compose, and calls renders the @@ -486,8 +491,12 @@ function collectProps(element, opts) { (props[match] || (props[match] = [])); const slot = `i-amphtml-${match}-${list.length}`; childElement.setAttribute('slot', slot); - const child = React.createElement('slot', {name: slot}); - list.push(child); + const def = opts.children[match]; + const slotProps = Object.assign( + {name: slot}, + typeof def == 'object' && def.props || {} + ); + list.push(React.createElement(Slot, slotProps)); } props.children = children; } @@ -509,8 +518,9 @@ function matchChild(element, defs) { } // TBD: a little slow to do this repeatedly. for (const match in defs) { - const expr = defs[match]; - if (element.matches(expr)) { + const def = defs[match]; + const selector = typeof def == 'string' ? def : def.selector; + if (element.matches(selector)) { return match; } } @@ -540,3 +550,35 @@ function toUpperCase(_match, character) { function dashToCamelCase(name) { return name.replace(/-([a-z])/g, toUpperCase); } + +function Slot(props) { + const ref = useRef(); + const slotProps = Object.assign({}, props, {ref}); + useEffect(() => { + const slot = ref.current; + + // Retarget slots and content. + if (props.retarget) { + // TBD: retargetting here is for: + // 1. `disabled` doesn't apply inside subtrees. This makes it more like + // `hidden`. + // 2. re-propagate events to slots since React stops propagation. + const disabled = slot.hasAttribute('disabled'); + slot.assignedNodes().forEach(node => { + node.disabled = disabled; + if (!node['i-amphtml-event-distr']) { + node['i-amphtml-event-distr'] = true; + node.addEventListener('click', () => { + const event = new Event('click', { + bubbles: true, + cancelable: true, + composed: false, + }); + slot.dispatchEvent(event); + }); + } + }); + } + }); + return React.createElement('slot', slotProps); +} From 1ded0029dc9d5b554ea05154e8dbec30f42e4ce8 Mon Sep 17 00:00:00 2001 From: Dima Voytenko Date: Mon, 9 Sep 2019 13:26:42 -0700 Subject: [PATCH 2/6] Copy all attributes from slot --- src/react-compat-base-element.js | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/src/react-compat-base-element.js b/src/react-compat-base-element.js index 9cee616..5c65152 100644 --- a/src/react-compat-base-element.js +++ b/src/react-compat-base-element.js @@ -561,11 +561,24 @@ function Slot(props) { if (props.retarget) { // TBD: retargetting here is for: // 1. `disabled` doesn't apply inside subtrees. This makes it more like - // `hidden`. + // `hidden`. Similarly do other attributes. // 2. re-propagate events to slots since React stops propagation. - const disabled = slot.hasAttribute('disabled'); slot.assignedNodes().forEach(node => { - node.disabled = disabled; + // Basic attributes: + const { attributes } = slot; + for (let i = 0, l = attributes.length; i < l; i++) { + const { name, value } = attributes[i]; + if (name == 'name') { + // This is slot's name. + } else if (!node.hasAttribute(name)) { + // TBD: this means that attributes can be rendered only once? + // TBD: what do we do with style and class? + node.setAttribute(name, value); + } + } + // Boolean attributes: + node.disabled = slot.hasAttribute('disabled'); + node.hidden = slot.hasAttribute('hidden'); if (!node['i-amphtml-event-distr']) { node['i-amphtml-event-distr'] = true; node.addEventListener('click', () => { From bdaa61bf08a85bf37fbf17bdbd16bec6eb161cb0 Mon Sep 17 00:00:00 2001 From: Dima Voytenko Date: Mon, 9 Sep 2019 13:29:35 -0700 Subject: [PATCH 3/6] doc react bug on composed events --- src/react-compat-base-element.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/react-compat-base-element.js b/src/react-compat-base-element.js index 5c65152..14c98e6 100644 --- a/src/react-compat-base-element.js +++ b/src/react-compat-base-element.js @@ -562,7 +562,8 @@ function Slot(props) { // TBD: retargetting here is for: // 1. `disabled` doesn't apply inside subtrees. This makes it more like // `hidden`. Similarly do other attributes. - // 2. re-propagate events to slots since React stops propagation. + // 2. Re-propagate click events to slots since React stops propagation. + // See https://github.com/facebook/react/issues/9242. slot.assignedNodes().forEach(node => { // Basic attributes: const { attributes } = slot; From b83fed65382420858344c934cf304530d18277d2 Mon Sep 17 00:00:00 2001 From: Dima Voytenko Date: Mon, 9 Sep 2019 13:34:47 -0700 Subject: [PATCH 4/6] minor --- src/react-compat-base-element.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/react-compat-base-element.js b/src/react-compat-base-element.js index 14c98e6..6d5b01c 100644 --- a/src/react-compat-base-element.js +++ b/src/react-compat-base-element.js @@ -570,7 +570,7 @@ function Slot(props) { for (let i = 0, l = attributes.length; i < l; i++) { const { name, value } = attributes[i]; if (name == 'name') { - // This is slot's name. + // This is the slot's name. } else if (!node.hasAttribute(name)) { // TBD: this means that attributes can be rendered only once? // TBD: what do we do with style and class? From 4314c36e6b474e3640cebb71fffebc4ba17c2dab Mon Sep 17 00:00:00 2001 From: Dima Voytenko Date: Mon, 9 Sep 2019 13:44:42 -0700 Subject: [PATCH 5/6] Stop original event propagation --- src/react-compat-base-element.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/react-compat-base-element.js b/src/react-compat-base-element.js index 6d5b01c..45939b0 100644 --- a/src/react-compat-base-element.js +++ b/src/react-compat-base-element.js @@ -582,7 +582,12 @@ function Slot(props) { node.hidden = slot.hasAttribute('hidden'); if (!node['i-amphtml-event-distr']) { node['i-amphtml-event-distr'] = true; - node.addEventListener('click', () => { + node.addEventListener('click', e => { + // Stop propagation on the original event to avoid deliving this + // event twice with frameworks that correctly work with composed + // boundaries. + e.stopPropagation(); + e.preventDefault(); const event = new Event('click', { bubbles: true, cancelable: true, From 7ca6270055784dc5108cb8abb649cb06a699dd80 Mon Sep 17 00:00:00 2001 From: Dima Voytenko Date: Mon, 9 Sep 2019 14:44:08 -0700 Subject: [PATCH 6/6] rebase --- src/amp-react-carousel-hooks.js | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/amp-react-carousel-hooks.js b/src/amp-react-carousel-hooks.js index 3415edb..8a7e6a0 100644 --- a/src/amp-react-carousel-hooks.js +++ b/src/amp-react-carousel-hooks.js @@ -169,8 +169,14 @@ const AmpReactCarouselHooks = ReactCompatibleBaseElement(AmpCarouselHooks, { }, }, children: { - 'arrowNext': '[arrow-next]', - 'arrowPrev': '[arrow-prev]', + 'arrowNext': { + selector: '[arrow-next]', + props: {retarget: true}, + }, + 'arrowPrev': { + selector: '[arrow-prev]', + props: {retarget: true}, + }, 'children': '*', }, });