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

Twitter bento component does not resize properly #37117

Open
deepaklalwani97 opened this issue Dec 6, 2021 · 17 comments · May be fixed by #37740
Open

Twitter bento component does not resize properly #37117

deepaklalwani97 opened this issue Dec 6, 2021 · 17 comments · May be fixed by #37740

Comments

@deepaklalwani97
Copy link
Contributor

deepaklalwani97 commented Dec 6, 2021

Description

The Twitter bento component does not resize properly when the page is scrolled fast. The embed shows the unexpanded state with an expand button. In both cases, the Tweet embed will appear unbuilt when it is first scrolled into view. This indicates the intersection observer needs to be tweaked for lazy construction of the embed. This issue occurs only for the amp-twitter-1.0 version the amp-twitter-0.1 works as expected check this example for reference.

ezgif com-gif-maker

Reproduction Steps

Open https://bento-twitter-intersection-observer-issue.glitch.me URL and scroll quickly to the Twitter embed the embed should appear in non expanded state.

Relevant Logs

Uncaught (in promise) Error: changeSize attempt denied
    at Object.callback (mutator-impl.js:79)
    at Pj (resources-impl.js:927)
    at h (resources-impl.js:1642)
    at Gj.setState (finite-state-machine.js:53)
    at Ij.g.doPass (resources-impl.js:643)
    at resources-impl.js:538
    at Ll (vsync-impl.js:456)
    at Hl.g.Bh (vsync-impl.js:413)

Browser(s) Affected

No response

OS(s) Affected

No response

Device(s) Affected

No response

AMP Version Affected

No response

@deepaklalwani97
Copy link
Contributor Author

@westonruter
Copy link
Member

Compare with the sizing of the non-Bento component: https://bento-twitter-intersection-observer-issue.glitch.me/non-bento.html

Lazy construction should happen before the component actually entered the viewport. It should happen when it hears the viewport. I believe amp-img uses 3 viewport heights and amp-iframe uses 1 viewport height for distances, or something to that effect.

@deepaklalwani97
Copy link
Contributor Author

I investigated this issue further and found this issue is not with the lazy construction as the amp-twitter gets built earlier in the viewport before it comes into the viewport. The issue is with the embed-size message that is being sent only after the iframe comes into the viewport. Here promise for the createTweet function resolves after the iframe comes into viewport. As the createTweet function is part of the Twitter platform.js we cannot make any changes to it. One possible solution for this issue could be to use forceChangeHeight here instead of attemptChangeHeight.

CC: @westonruter @adamsilverstein

@westonruter
Copy link
Member

One possible solution for this issue could be to use forceChangeHeight here instead of attemptChangeHeight.

The use of attemptChangeHeight instead of forceChangeHeight was intentionally done in #35027.

@deepaklalwani97
Copy link
Contributor Author

@westonruter I could not figure out any other solution to fix this issue without using the forceChangeHeight as the promise for the createTweet function resolves only after the embed comes into the viewport and the attemptChangeHeight rejects the resizing attempt once the element is completely visible. Could you please advise any alternate possible way we can fix this?

@westonruter
Copy link
Member

the promise for the createTweet function resolves only after the embed comes into the viewport

I think this is the crux of the problem. It should rather resolve when the embed nears the viewport, not when it is actually inside the viewport. I recall that amp-img starts loading when it gets within 3 viewports of the current viewport, whereas a more expensive element like amp-youtube starts loading when it is within 0.75 viewport of the current viewport. That same approach should be taken with createTweet.

See the renderOutsideViewport function in amp-youtube:

/** @override */
renderOutsideViewport() {
// We are conservative about loading YT videos outside the viewport,
// because the player is pretty heavy.
// This will still start loading before they become visible, but it
// won't typically load a large number of embeds.
return 0.75;
}

Compare with the default value in the base class:

amphtml/src/base-element.js

Lines 489 to 501 in cd80895

/**
* Subclasses can override this method to opt-out of rendering the element
* when it is not currently visible.
* Returning a boolean allows or prevents rendering outside the viewport at
* any distance, while returning a positive number allows rendering only when
* the element is within X viewports of the current viewport. Returning a
* zero causes the element to only render inside the viewport.
* @return {boolean|number}
*/
renderOutsideViewport() {
// Inabox allow layout independent of viewport location.
return getMode(this.win).runtime == 'inabox' || 3;
}

@deepaklalwani97
Copy link
Contributor Author

That same approach should be taken with createTweet

@westonruter The createTweet and other functions like createTimeline and createMoment are part of the twitter's Widget.js API so we cannot make any changes to that.
Ref: createTweet

@adamsilverstein
Copy link
Contributor

the promise for the createTweet function resolves only after the embed comes into the viewport

@deepaklalwani97 I don't quite understand, are you calling createTweet directly and getting the promise? and the promise only resolves when the tweet is in the viewport? i didn't see anything that would do that in the code you linked. can you manually call resolve on the promise?

@deepaklalwani97
Copy link
Contributor Author

@adamsilverstein Yes, calling the createTweet returns a promise and it is only getting resolved only when it comes in the viewport, and manually calling the resolve on the promise doesn't help.

@westonruter
Copy link
Member

Ah, so the problem is that Twitter's own embed API is asynchronous (naturally). But if you set the equivalent of renderOutsideViewport to 3 wouldn't createTweet be getting called early enough that by the time createTweet resolves the Tweet embed (hopefully) is not yet in the viewport and attemptChangeHeight won't be rejected?

@deepaklalwani97
Copy link
Contributor Author

@westonruter No, even if we set the renderOutsideViewport to 3 this wouldn't help because no matter how early the component is built the embed resizes only when it comes into the viewport. You can verify this in this example itself the amp-twitter is getting built on the page load but it requests resize when it comes into the viewport.

Screenshot 2022-02-14 at 11 40 22 AM

@westonruter
Copy link
Member

OK, right. So the issue is that Twitter is deferring the initialization until it comes inside the viewport. I guess the only two options I see are:

  1. See if Twitter's createTweet function can be forked to initialize when further away from the current viewport, or else contact Twitter to see if they can essentially add a renderOutsideViewport parameter to createTweet.
  2. Revert Bento: iframe components attemptChangeHeight  #35027.

@alanorozco
Copy link
Member

OK, right. So the issue is that Twitter is deferring the initialization until it comes inside the viewport. I guess the only two options I see are:

  1. See if Twitter's createTweet function can be forked to initialize when further away from the current viewport, or else contact Twitter to see if they can essentially add a renderOutsideViewport parameter to createTweet.

I wonder if we can somehow hack this. When the embed is out of view we could initially style the actual twitter embed as position: fixed; opacity: 0; pointer-events: none. From the embed's perspective, this might trick it into believing it's in viewport and trigger the resize events. Once we get a resize event, or the container comes into view, we can revert to position: static.

@westonruter
Copy link
Member

Oh yes, that's brilliant.

@deepaklalwani97
Copy link
Contributor Author

@alanorozco Thanks for your suggestion I have created a PR #37740 with your mentioned workaround with this the embed now gets resized as soon as the component gets built when shown below the fold. Can you review and provide any necessary feedback.

@stale
Copy link

stale bot commented Mar 19, 2023

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 stale bot added the Stale Inactive for one year or more label Mar 19, 2023
@brad82
Copy link

brad82 commented May 2, 2023

This issue is also present in both the 0.1 and 1.0 versions of the standard amp-tweet component when using a timeline source. The embedded timeline does not resize correctly within the amp iframe, causing it to be displayed at 1px wide and invisible

@stale stale bot removed the Stale Inactive for one year or more label May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants