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.

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

[amp-fit-text:1.0] Use ResizeObserver for content changes #28496

Merged
merged 6 commits into from Nov 2, 2020

Conversation

caroqliu
Copy link
Contributor

@caroqliu caroqliu commented May 19, 2020

Instead of useEffect for children prop. Partial for #28281

@dvoytenko
Copy link
Contributor

Ok. This looks super simple. Even simpler than before. But does this work?

@caroqliu
Copy link
Contributor Author

Ok. This looks super simple. Even simpler than before. But does this work?

I tested with resizing the dimensions on the container by modifying the computed dimensions from the Chrome Developer Tools. I also tested with resizing the contents by appending and removing text nodes in the DOM. I did this in both the AMP example document and the Preact Storybook sample. :) Is there anything else you think would be useful here?

@dvoytenko
Copy link
Contributor

Ok. This looks super simple. Even simpler than before. But does this work?

I tested with resizing the dimensions on the container by modifying the computed dimensions from the Chrome Developer Tools. I also tested with resizing the contents by appending and removing text nodes in the DOM. I did this in both the AMP example document and the Preact Storybook sample. :) Is there anything else you think would be useful here?

This sounds awesome. The only confirmation I'm really looking for is a test like this:

  1. Start with some parameters close to the max. The content is fully visible, but we're definitely close.
  2. Do something at the parent level (e.g. set parent's font-size to a higher value) so that the content is expected to overflow. Confirm that the content has overflow and fit-text set line-clamp/max-height, and we're seeing "...".
  3. Change the parent back. Observe that line-clamp/max-height are gone.

@caroqliu
Copy link
Contributor Author

caroqliu commented May 27, 2020

Do something at the parent level (e.g. set parent's font-size to a higher value) so that the content is expected to overflow. Confirm that the content has overflow and fit-text set line-clamp/max-height, and we're seeing "...".
Change the parent back. Observe that line-clamp/max-height are gone.

Here is what I did:

  1. Run gulp storybook-preact and open the "Default" FitText example. Change the knob so that the minFontSize is set to 35.
  2. Select a parent node. In this case I chose to modify the div that directly wraps the div containing the FitText calculated font-size style. I set the style font-weight: bold.
  3. You will see there is overflow but there is not added line-clamp/max-height:
    image

It appears that the style modification in (2) does not cause a record on the ResizeObserver to fire. I did some digging and discovered that this is due to the overflow: hidden property applied to the content (measurerRef) element. -- When the bold is applied and causes the content to grow, its measured dimensions do not change due to overflow: hidden.

I then moved the overflow: hidden property one level above to make the content change impact the dimensions. This does trigger the resize observer and apply the component's overflow styles. However, as you can see below because the overflow: hidden was moved to the parent node, the overflow content is still rendered past the ellipsis:
image

Finally I set overflow: hidden as a property that can be added with lineClamp and etc. in setOverflowStyle, causing a successful resize to content change:
image

However both before and after the inclusion of overflow: hidden as one of the applied overflow styles in setOverflowStyle, removing the font-weight: bold style did not cause a record on the ResizeObserver to fire, thereby leaving the overflow styles there:
image

Perhaps even worse, it appears to be inconsistently firing when toggling the font-weight: bold property from the Chrome Developer Tools: Following steps 1-3 above, then unchecking the new style appears to lead to a new ResizeObserver entry and removal of the overflow CSS styles, but on subsequent times rechecking and unchecking the new style does not appear to fire any ResizeObserver entries:
5261d9c1-49d4-4925-940c-5663e6f6c4ca

tldr; We can only modify to the extent that we can consistently reproduce (1) and (2) in your manual test confirmation but not (3).

@caroqliu
Copy link
Contributor Author

Note: This feature also addresses some a11y implications: #29556

@caroqliu
Copy link
Contributor Author

caroqliu commented Nov 2, 2020

Updated some merge conflicts with the ContainWrapper usage, PTAL @dvoytenko

@caroqliu caroqliu merged commit 9605486 into ampproject:master Nov 2, 2020
ed-bird pushed a commit to ed-bird/amphtml that referenced this pull request Dec 10, 2020
…#28496)

* Observe measurerRef

* Set and observe a nested `contentRef` div with `height: min-content`

* Move `height: min-content` to css

* Use `ContainWrapper` preset refs

* Place in css file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants