This should be subtree observer I believe to deal with <ul> and other markup nuances. And we should also limit it to the attributes we look for in children. E.g. only with "option" and "selected", right?

Oops. I shouldn't be reviewing both amp-fit-text and amp-selector at the same time. attributesFilter is not needed here. However, this should still be subtree, right?

Ya, good catch. But I think we need to resize on any childlist mutation? That could include child attr mutations, too.

To summarize:

• Add childList for changes in the target node
• Add subtree for changes on descendants of the target node
• Add attributes for attr changes all along this tree ^

Is there any need to throttle the callback in case there are many DOM mutations occurring in quick succession? Does the browser handle this?

I'd not throttle, browser manages the load for us: we will just have several mutation records in the array - though we don't really care how many of them are - one or many we do the same thing.

Having both subtree and attributes present is causing the observer callback to fire infinitely due to the i-amphtml-context attribute being re-applied on every render. Going to remove one of these for now in favor of improving this separately later on.

Can we simply ignore mutation records with attribute = i-amphtml-context? Or even with any startsWith(attribute, 'i-amphtml-')?

Yes, would it be costly to filter every set of mutations for this?

@dvoytenko @jridgewell FYI since I know we prefer to use AmpFoo['props'] whenever possible -- I discovered from the e2e tests that the font size should be sized to the offset dimensions of the element if we are to stay consistent with 0.1

I think sizing to offset dimensions was always a little bit wrong. The right dimension are clientWidth/Height. But we can make it so it doesn't matter.

But ultimately, can we just drop this code entirely and not pass width and height explicitly?

I think there are now couple of threads on this review on this subject, so please feel free to dedup. But I'll make more targetted comments over the pull request on the subject in this pass.

On one hand, we are saying "the dev should specify width/height via styles" and on the other we are saying we should not explicitly pass dimensions from AMP mode. In this case we are the dev who should specify the dimensions. I'm OK with doing this via prop.style instead of via prop but I don't think we can get away with doing neither.

Certainly. AMP would just pass style={width:100%, height:100%}, right?

The only thing this changes is the e2e test case wherein the expected font size accounts for the border dimensions. Are we OK with breaking that?

But ultimately, can we just drop this code entirely and not pass width and height explicitly?

I think there are now couple of threads on this review on this subject, so please feel free to dedup. But I'll make more targetted comments over the pull request on the subject in this pass.

This one we get for free on the first resize.

Yes that's right, are you suggesting fine tuning more?

I meant that we don't need useLayoutEffect here, do we? If we don't have it, then we also don't need to pass width/height from the outside.

Changed this to useEffect and moved the width and height deps to the useCallback deps for the resize method.

Took another look at this, and with useEffect instead of useLayoutEffect you can see some intermediary styling while the font size is being calculated. I'd prefer to keep useLayoutEffect since its a bit more polished.

useEffect:

useLayoutEffect:

I agree with useLayoutEffect vs useEffect. However, here are two important nuances:

1. children always almost change on re-render, even if they are in fact didn't change. That's how Preact works - they don't want to do item-by-item comparisons on the children array. So this callback will trigger on each re-render and could be wasteful.
2. The opposite is also true: children do not need to change for the size to change. E.g. a parent font change could lead to children resize. We could potentially ignore this for awhile, but if we'd get it for free from resize observer, that'd be potentially a good win.

It's fine to defer this as a TODO in the tracker. But it's something that we should deal with very soon.

We get (2) with the resize callback that is dependent on the min and max font size props, no?

I'll add (1) to the open tasks.

(2): Not always. Min/max or any other props do not need to change. It's enough for some far away parent to change a font-family which may never even be propagated this far.

That being said, I'm ok if this is an open task as well.

This covers the parent size changes, but not the content changes. E.g. if the updated children are passed with more text, then font-size should get smaller to accommodate. We may either have to do to resize observers, one for contentRef and one for measurerRef. Or maybe just one for measurerRef will suffice. I also commented on the maxHeight issue below for measurerRef. All of these cases, btw, ideally would be storybook cases as well to track regressions.

I'm not sure how we can track this in storybook since the component is forced to render (as if on first load) every time you change a knob. Is there a way to change state in Storybook and look for reactionary render rather than render caused by Storybook?

The secondary useLayoutEffect discussed here is for managing content changes.