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-reddit embeds don't resize correctly #11869

Open
Cauchon opened this issue Oct 30, 2017 · 26 comments

Comments

@Cauchon
Copy link

commented Oct 30, 2017

What's the issue?

amp-reddit embeds isn't resizing the height of the Reddit embed correctly, causing the bottom of the Reddit embed to get cut off

How do we reproduce the issue?

If this is a bug please provide a public URL and ideally a reduced test case (e.g. on jsbin.com) that exhibits only your issue and nothing else. Then provide step-by-step instructions for reproducing the issue:

  1. Use amp-reddit
  2. Load AMP document using Chrome Inspector
  3. Toggle Device Toolbar and view the AMP doc as an iPhone 5 and iPhone 6
  4. Notice that the bottom of the Reddit embed is cut off.

Example: http://cauchon.co/reddit.html

What browsers are affected?

All browsers

Which AMP version is affected?

AMP ⚡ HTML – Version 1508794187431
amp-reddit-0.1.js

I believe this has always been broken

@dvoytenko

This comment has been minimized.

Copy link
Collaborator

commented Nov 1, 2017

/to @jridgewell ptal

@jridgewell

This comment has been minimized.

Copy link
Member

commented Nov 2, 2017

There doesn't appear to be any communication between the embed's inner iframe and the embed. I'm not sure how we can support this.

/cc @samiamorwas, who wrote the amp-reddit extension.

@jridgewell jridgewell added the Type: Bug label Nov 2, 2017

@samiamorwas

This comment has been minimized.

Copy link
Contributor

commented Nov 2, 2017

I believe I ran into this issue myself, and couldn't find any way to support it at the time.

@ericlindley-g ericlindley-g added this to Needs Triage in UI Jan 4, 2018

@ericlindley-g ericlindley-g moved this from Needs Triage to Bugs in UI Jan 8, 2018

@ampprojectbot

This comment has been minimized.

Copy link
Collaborator

commented Feb 12, 2018

This issue hasn't been updated in awhile. @jridgewell Do you have any updates?

@johnnyshankman

This comment has been minimized.

Copy link

commented Apr 18, 2018

Ran into this today, would love to see if this is on the schedule for fixing.

@jridgewell

This comment has been minimized.

Copy link
Member

commented Apr 18, 2018

It's not prioritized. We'd need someone from reddit to help us figure out their iframe API.

@johnnyshankman

This comment has been minimized.

Copy link

commented Apr 18, 2018

Hm alright. Well I guess I'll subscribe to this issue and check back in the future.

My team will punt on implementing <amp-reddit> for now since there's no reliable way to find the proper aspect ratio from the embed code (for dynamically setting height and width properties) and we can't have broken-looking embeds on the site.

Thanks for the quick response @jridgewell !

@Cauchon

This comment has been minimized.

Copy link
Author

commented Apr 18, 2018

I just reached out to a contact at Reddit to see if they could send this to the right team for feedback.

@jridgewell

This comment has been minimized.

Copy link
Member

commented Apr 18, 2018

Thanks @Cauchon!

@Cauchon

This comment has been minimized.

Copy link
Author

commented May 4, 2018

Update from Reddit:

I checked in with Embedly, the company that powers our post embeds. They noted they're investigating some issues with Google AMP, so they're looking into this. If it's useful to the AMP team in the meantime, they provided their documentation on how Embedly iframes communicate resize requests with the pages that support their height resizing: http://docs.embed.ly/v1.0/docs/provider-height-resizing
I'll circle back when I have more info. Thanks!

@jridgewell

This comment has been minimized.

Copy link
Member

commented May 4, 2018

WOO! We can work with http://docs.embed.ly/v1.0/docs/provider-height-resizing!

Passing this to @aghassemi

@jridgewell jridgewell assigned aghassemi and unassigned jridgewell May 4, 2018

@aghassemi

This comment has been minimized.

Copy link
Contributor

commented May 4, 2018

Thanks @jridgewell and @Cauchon. (a related but separate FYI: We are also working on a generic amp-embedly-card component #14819 )

/to @nainar, Looks like we can now receive resize events and call updateDimensions similar to other embeds.

@aghassemi

This comment has been minimized.

Copy link
Contributor

commented May 30, 2018

Fixed by #15530

@aghassemi aghassemi closed this May 30, 2018

@johnnyshankman

This comment has been minimized.

Copy link

commented Jun 5, 2018

@aghassemi I'm still having issues with this embed not resizing its own height correctly and being cut-off or too tall.

Here's a JSFiddle showing the issue where it is cut off too soon.

I also noticed an edge case where when I resize the window, suddenly it goes from being cut off to being too tall.

Is this expected behavior w/ my embed setup or this possibly a bug with the new implementation? Did I miss something in my JSFiddle?

@aghassemi

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2018

/to @nainar see comment above from @johnnyshankman . #15530 is in production.

@aghassemi aghassemi reopened this Jun 8, 2018

@aghassemi aghassemi removed their assignment Jun 8, 2018

@nainar

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2018

Hi @johnnyshankman, first of all sorry that your initial complain fell through the cracks on my end.

Looking at your demo link. The aspect ratio you have specified is too small which is why the component is being cut off since layout is marked as responsive which respects the aspect ratio you provided.

If you increase the component's height so that it is say 475px to match the width of 300px then the component has an aspect ratio retains the content without cutting it off.
AMP layout=responsive isn't really "responsive" per say as can be seen here: #13872. It is instead sized to 100% width of parent and height auto per aspect ratio. There is an open issue to fix this but as a work around setting a larger height to get a correct aspect ration ought to work.

@nainar

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2018

Closing this as per previous comment, please file any new feedback as issues. Thank you!

@nainar nainar closed this Jun 19, 2018

@johnnyshankman

This comment has been minimized.

Copy link

commented Jul 10, 2018

@nainar thank you. We'll have to only use reddit embeds where we can assert the aspect ratio ahead of time.

@dermatzi

This comment has been minimized.

Copy link

commented Jul 17, 2018

i am confused that reddit itself offers an "amp-iframe" tag when getting the embed code for a post. why would they not use their own amp-reddit tag?
also i notice, that the responsive resize of reddit's amp-iframe works fine ("resizable" attribute).
so why not simply implementing the "resizable" attribute for the amp-reddit tag?

@mehigh

This comment has been minimized.

Copy link

commented May 17, 2019

If you use amp-embedly-card component instead of the amp-reddit one you won't have to fill in the aspect ratio ahead of time.
The generic component mentioned earlier (here's the link again: #14819) works perfectly for this purpose.

<amp-embedly-card
    data-url="https://www.reddit.com/r/..."
    layout="responsive"
    width="100"
    height="100"
    data-card-controls="0">
</amp-embedly-card>
@mehigh

This comment has been minimized.

Copy link

commented May 17, 2019

I've created a pull request against the WordPress plug-in to use the amp-embedly-card. Maybe the amp-reddit component needs to not be recommended any longer in the main documentation and recommend embedly instead, as I imagine it would be very difficult to propagate iframe resizing in the current approach where an iframe contains the embedly iframe.

@westonruter

This comment has been minimized.

Copy link
Member

commented May 20, 2019

@nainar This issue should be re-opened. The issue is that the amp-reddit component's iframe is not resizing itself as it should be, while the amp-embedly-card is. Compare the following, with 100x10 dimensions to exaggerate the point:

  <p>
    <code>amp-reddit</code>:
  </p>
  <amp-reddit
      layout="responsive"
      data-embedtype="post"
      width="100"
      height="10" 
      data-src="https://www.reddit.com/r/me_irl/comments/52rmir/me_irl/?ref=share&amp;ref_source=embed">
  </amp-reddit>
  <p>
    <code>amp-embedly-card</code>:
  </p>
  <amp-embedly-card
      data-url="https://www.reddit.com/r/me_irl/comments/52rmir/me_irl/"
      layout="responsive"
      width="100"
      height="10">
  </amp-embedly-card>

This is getting rendered as:

image

Notice that the amp-reddit is not resizing like the amp-embedly-card is.

Perhaps there is a discrepancy between the way these two components are sending the resize messages:

amphtml/3p/reddit.js

Lines 86 to 91 in 873708f

global.addEventListener('resize', event => {
global.context.updateDimensions(
event.target.outerWidth,
event.target.outerHeight
);
});

amphtml/3p/embedly.js

Lines 101 to 115 in 873708f

getEmbedly(global, function() {
// Given by the parent frame.
delete data.width;
delete data.height;
global.window['embedly']('card', card);
// Use embedly SDK to listen to resize event from loaded card
global.window['embedly']('on', RESIZE_EVENT_NAME, function(iframe) {
context.requestResize(
iframe./*OK*/ width,
parseInt(iframe./*OK*/ height, 10) + /* margin */ 5
);
});
});

The code in reddit.js is not working while the code in embedly.js apparently is.

The initial dimensions for this component should be intended for any placeholder content or a best guess as to what the dimensions will be. The actual dimensions should be determined by the component upon load, just like amp-gist:

image

@mehigh

This comment has been minimized.

Copy link

commented May 21, 2019

Actually reddit mentioned they are using embedly for embeds. When inspecting the reddit amp embed you can see it's an iframe with another iframe inside of it (maybe it's an embedly inside of a reddit frame).

@nainar nainar reopened this May 22, 2019

@jridgewell

This comment has been minimized.

Copy link
Member

commented May 22, 2019

Is it possible to rewrite the reddit component to use embedly now that it's the recommended way?

@mehigh

This comment has been minimized.

Copy link

commented May 23, 2019

@jridgewell

This comment has been minimized.

Copy link
Member

commented May 23, 2019

That would be fine, too. But if it's possible to just upgrade the current <amp-reddit layout="responsive" data-embedtype="post" width="100" height="10" data-src="..."> </amp-reddit> to use embedly under the hood, all current users of <amp-reddit> would get a better experience without having to do anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.