amp-selector implementation in React

[dvoytenko]

amp-selector is the most unusual component so far.

The top level API (value property, onChange, etc) are modeled on React's support for <select>.

Next, in AMP we allow a mostly free-form structure of amp-selector content as long as there are some elements that have option attribute. This is done to easily support ul > li, table > tr > td, and similar structures. Thus, the SD slots do not work here. Instead I adopted a different structure: AmpSelector > whatever > AmpSelector.Option connected via AmpSelectorContext. AmpSelectorContext is private and otherwise not visible to the outside API.

Finally, AMP side is unusual too: we have to observe subtree mutations to find [option] deep-children. And w/o slots we use the Preact's render function to keep DOM and VDOM in-sync.

[dvoytenko]

Reimplemented VDOM -> DOM mapping via slots, using ~the same~ code as for slot retargetting in carousel arrows.

[jridgewell]

What's this for?

[dvoytenko]

I think we will eventually switch to an explicit mapping for slot -> assignedNode attribute propagation. We currently do disabled because it doesn't inherit. We do hidden more as an signal for AMP for our hidden-observer. The selected, expanded and such are more similar to disabled. But I think we'll eventually want to make this mapping explicit like we do right now in AMP-binding's opts: {attrs: {...}} structure. Probably that mapping code could be even reused.

[jridgewell]

Should this be a part of the AmpBaseElement options? It doesn't seem scalable to have sync every attribute between slot and children.

[dvoytenko]

Yes. I think this is where we are headed.

[jridgewell]

I think this is more of an AMP concern, instead of a React component one. How about if init returns an object that could have an optional postRender method, that way we can completely remove this from the React side?

[dvoytenko]

Slot component is managed by Preact, but otherwise it only exists for a single purpose of AMP/Preact integration. So it's not entirely too foreign here. But do you have any suggestion how we can do it better?

There are many things I don't like about this, but here's what I was trying to achieve. The main goal is to reduce render/re-render cycles. A mutation on a DOM element can lead to the component re-rendering (with new properties), which in turn can lead to DOM mutation. Typically this are benign and only cause extra re-rendering. But I did run into an infinite loop once or twice. Thus, we want to have a code such as:

pauseMutationObserver()
render()
resumeMutationObserver()

This poses a chain of its own problems:

1. Pause/resume observer is a bit expensive - it's essentially equivalent to disconnect/reconnect observer. It's cheaper to just call observer.takeRecords(). Not sure how much cheaper this is - we need to profile.
2. observer.takeRecords() could be dangerous - we could accidentally purge other mutation records that do not belong to Slot implementation, if they happen to have happened very close to each other. I used it here as a first draft, but this is likely not the last on this.
3. If we do adopt pause/resume - resume is harder to do. We have to repeat observe() with all the same configuration.
4. How do we actually pass the wrapper (or pause/resume pair) to the Slot component? The only options appear to be props or context. Since Slot is very AMP-specific, I decided to just use props instead of context.

As we develop it, we could make it look cleaner using some sort of useAmpRenderingEffect that would do pause/resume auto-magically. But we're not quite there yet.

[jridgewell]

A mutation on a DOM element can lead to the component re-rendering (with new properties), which in turn can lead to DOM mutation. Typically this are benign and only cause extra re-rendering. But I did run into an infinite loop once or twice.

What if we use a passthrough default slot for the light tree? Or is it a mutation on the light tree that's causing the infinite loop?

[dvoytenko]

For a selector, all mutations are in the main DOM tree and all observable by the main script/css/etc. E.g. a CSS like .option[selected] {border: 1px solid red}. That's currently virtually the only option for us to signify that a selection option (as a DOM element) is currently selected. So a subtree-based MutationObserver would see these mutations too. E.g. both a valid code paths:

/1/ User clicks on an option and the React component sets it as selected. Our Slot delegation updates DOM:

// A. React's onClick handler -> state
function AmpSelection.Option() {
  ...
  (<x onClick={() => setSelectedOptionState(props.option)} ...>)
  ...
}

// B. Slot's props.selected is set in React:
function AmpSelection.Option() {
  ...
  (<Slot selected={isSelectedOptionState(props.option)}>)
  ...
}

// C. Slot's side effect sets DOM attribute:

function Slot() {
  const domRef = useRef();
  useEffect(() => {
    const slot = domRef.current;
    ...
    // Update in the main DOM:
    const assignedOption = slot.assignedElements()[0];
    if (props.selected) {
      assignedOption.setAttribute('selected', '');
    } else {
      assignedOption.removeAttribute('selected');
    }
  });
  ...
}

/2/ A script in the main document manually writes DOM:

button1.onclick = () => {
  option2.setAttribute('selected', '');
};

We need the mutation observer to synchronize DOM -> React component in the case /2/. But we don't really need mutation observer for /1/ since we ourselves ensure that DOM/React are in full sync. In general case, incorrectly working /1/ can cause cycles. So far the cycles in such mutations have been easy to work around or ignore. But in general case this is still a dangerous situation. IMHO it'd be nice to have a more general solution for this.

And, finally, writing selected attribute is definitely not great. It'd be much better from API point of view if we could set a custom state, e.g. .option:selected {...}. But custom states spec is still ways and ways away.

[dvoytenko]

Filed #40 for this nuance.

[jridgewell]

A mutation on a DOM element can lead to the component re-rendering (with new properties), which in turn can lead to DOM mutation. Typically this are benign and only cause extra re-rendering. But I did run into an infinite loop once or twice.

What if we use a passthrough default slot for the light tree? Or is it a mutation on the light tree that's causing the infinite loop?

[jridgewell]

Should this be a part of the AmpBaseElement options? It doesn't seem scalable to have sync every attribute between slot and children.