amphtml icon indicating copy to clipboard operation
amphtml copied to clipboard

amp-social-share causes layout shift on upgrade

Open kevinkimball opened this issue 5 years ago • 4 comments

What's the issue?

When .i-amphtml-unresolved class is removed on component upgrade, overflow is changed from hidden to visible, which causes subsequent elements to shift (both sibling inline element and following image).

This issue was surfaced by this issue from the Page Experience tool (https://github.com/ampproject/wg-performance/issues/48).

This may be related to https://github.com/ampproject/amphtml/issues/27228 but data-mode=replace is not used in the example.

How do we reproduce the issue?

Visit the URL below with "Layout Shift Regions" enabled in Rendering tab of Chrome Dev Tools to visualize the layout shift.

https://tasty.co/amp/recipe/garlic-bread-bruschetta

What browsers are affected?

Chrome 87

Which AMP version is affected?

2009252320001

kevinkimball avatar Oct 14 '20 00:10 kevinkimball

This is a high priority if this is causing CLS issues - it is a highly used AMP component.

@zhouyx to assign and @caroqliu for context.

nainar avatar Oct 19 '20 19:10 nainar

On potential solution for this would be to permanently add the overflow: hidden; style to this component. However this change would be backwards incompatible and would require a new revision of the social-share component.

@alanorozco, @kristoferbaxter, @krdwan Do you have any thoughts the correct path forward?

rbeckthomas avatar Jan 21 '21 04:01 rbeckthomas

Discussed offline with @rebeccanthomas:

The "print" button appears to have display: inline-block which aligns itself to the baseline of the amp-social-share icons. It looks like applying overflow: hidden; vertical-align: top; to the amp-social-share selector will fix it for this particular document, not sure about others. Alternatively, the developers for could applyvertical-align: top; to their Print button and fix that part of the page's CLS without us making any changes to amp-social-share.

Do we have other sample documents @kevinkimball?

Short term recommendations:

  • Apply overflow: hidden; vertical-align: top; on the repro case.
  • Investigate if the overflow: hidden from .i-amphtml-unresolved can be overridden to be overflow: visible for amp-social-share.i-amphtml-unresolved in the ampshared.css as @rebeccanthomas mentioned above.

Long term recommendation:

  • This further strengthens the case for a no-CLS v2 redesign. :)

caroqliu avatar Feb 10 '21 21:02 caroqliu

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Aug 12 '22 00:08 stale[bot]