wearethoughtfox / amnesty-facebook

0 stars 0 forks source link

iPad: Amnesty website iframe extends past the bottom of the background image #52

Closed pauldwaite closed 7 years ago

pauldwaite commented 7 years ago

Steps to reproduce

  1. On an iPad, nominate a friend.

Expected behaviour

I’m not entirely sure (see #36), but I presume the iframe that shows the Amnesty website shouldn’t extend further than the page background.

Actual behaviour

The Amnesty website iframe extends quite a long way past the page background: it appears to show the entire Amnesty homepage. (Maybe that’s intended.)

ipad

robertocarroll commented 7 years ago

I put the iframe in a responsive container (same as responsive video trick):

.iframeContainer {
    position: relative;
    width: 100%;
    height: 0;
    padding-bottom: 56.25%;
}

.iframeInner {
    position: absolute;
    top: 0;
    left: 0;
    width: 100%;
    height: 100%;
}

I didn't realise that approach didn't work everywhere. Any ideas? See update to #36 for details on what this is meant to do.

pauldwaite commented 7 years ago

Yeah weird — I think that’s not working on iOS at least. I’ll take another look.

pauldwaite commented 7 years ago

Yup — seems like, just on iOS, height:100% on .iframeInner is not calculated relative to the height of .iframeContainer. I’m not sure what it is calculated relative too: possibly the height of the document contained within the iframe.

Safari on macOS, Chrome on macOS and Android, and Android Browser on this Samsung Galaxy S4 all calculate height:100% relative to .iframeContainer.

This Stack Overflow answer kind of explains a fix:

http://stackoverflow.com/questions/23083462/how-to-get-an-iframe-to-be-responsive-in-ios-safari

Essentially, you have to add scrolling="no" to the iframe, and instead of height:100%, you go with height:1px;min-height:100%. I’ve implemented that here: https://github.com/wearethoughtfox/amnesty-facebook/commit/7dc171b02c139fd7074084be4d1497975c22bc52

That seems to fix the issue, and not cause any problems in other browsers (although I haven’t confirmed in IE yet). And if scrolling="no" is a no-no, obviously it’s a non-starter.

Is there any chance the call-to-action can or will have a fixed pixel width and height?

robertocarroll commented 7 years ago

Let's hope it doesn't need scroll. I'll know more tomorrow. Closing and see #53 for follow on.

robertocarroll commented 7 years ago

So I added a call to action iFrame from Amnesty and it looks like it does need to scroll. @pauldwaite any ideas?

robertocarroll commented 7 years ago

Here's the full link: https://join.amnesty.org/ea-action/action?ea.client.id=1924&ea.campaign.id=69355

robertocarroll commented 7 years ago

could we use the same responsive iframe technique that we used on the DP map and would use here to embed in the Amnesty site?

pauldwaite commented 7 years ago

Alrighty — by default, on iOS, iframes don’t honour a width or height you set for them if the page they’re showing is wider or taller.

For the Amnesty URL above, that doesn’t seem to be a problem: the content only has a max-width, set to 500px. So we can set whatever width we want for the iframe, and the height will expand so that all the contents are available.

If we want a fixed height for the iframe that’s honoured in iOS, and we want to allow the iframe to scroll, we have to fake it. We can do this by wrapping the <iframe> in a <div> with a fixed height, overflow-y:scroll, and -webkit-overflow-scrolling: touch;. I’ve implemented that in the branch (https://github.com/wearethoughtfox/amnesty-facebook/commit/a62d9ef5f3be500f35b0e4fa5d48bbc031a60f9a), and it seems to be working.

As far as I can tell, we won’t need the iframe resizer script — the point of that script is to change an iframe’s height/width to match the dimensions of the page being displayed in it. As I understand it, we don’t want to do that here.

robertocarroll commented 7 years ago

Thanks! I had merged two things in my head:

  1. this problem
  2. embedding our app in Amnesty's site see #60

Obviously two different problems, which my addled brain connected. I must admit I'm a bit lost in this situation, but what if we didn't want this to scroll at all and just show the entire iframe?

pauldwaite commented 7 years ago

what if we didn't want this to scroll at all and just show the entire iframe?

You mean have the iframe be as tall as the Amnesty call-to-action page that’s displayed in it. iOS does that by default, so there, we can remove our fake fixed-height iframe. For other browsers, we’d need Amnesty to make the iframe resizer scripts run on the call-to-action page (maybe they do already?).

The background image is set to background-cover, so it will zoom in if we make the page taller.

robertocarroll commented 7 years ago

Thanks! I haven't heard back about this and it works OK in the scroll, so I'm going to close it for now.

pauldwaite commented 7 years ago

I’ve changed how this iframe works now — our solution to make it fixed height in iOS was resulting in double scrollbars everywhere else, and I couldn’t work out how to prevent that from happening.

The iframe is now fixed at 600 pixels tall. iOS ignores this, and makes it as tall as required to fit the page shown in the iframe, but that page (Amnesty’s CTA form) is about 600 pixels anyway, so it doesn’t make much difference.

https://github.com/wearethoughtfox/amnesty-facebook/commit/773d85a3bff6d6a623ceb30c01e57c29742334ca

What we couldn’t do was have the existing layout where the iframe’s height was a percentage of its width. We couldn’t have this because it requires putting the iframe in a position:absolute container, which means it no longer takes up vertical space on the page, which means when iOS makes the iframe as tall as its content, that doesn’t affect the page layout, so the iframe extends past the bottom of the page, and thus the page’s background image doesn’t stretch to the full height of the page (we get white space at the bottom, like in the original bug screenshot).

We can discuss this further if this change is no good.