search

ampproject / amphtml

The AMP web component framework.
https://amp.dev
Apache License 2.0
14.89k stars 3.89k forks source link

Support attemptResize with unit other than px #8095

Closed lannka closed 6 years ago

lannka commented 7 years ago

As discussed here #7957, is it a good idea to support resizing to CSS values like:

@dvoytenko @zhouyx

dvoytenko commented 7 years ago

Don't see a reason, TBH. Measurements are always done in px in DOM APIs. 100% and 100vw probably indicate the desire to expand to the full viewport. This is good, but:

  1. Should be handled via special API (e.g. enterLightboxMode or enterOverlayMode)
  2. Should be guarded by the user action
  3. Should be reversible by the AMP Runtime: in other words, the full-screen-overlays must be always cancelable and in AMP Runtime's (read user's) control.
  4. Should integrate accessibility tools and history API. E.g. the back-button should work.
zhouyx commented 7 years ago

I agree expanding to full viewport should be handled via special API, not #attemptChangeSize. But I think the request is different from entering a full screen mode. It is only resizing through attemptChangeSize function. Even now, user can take the whole screen by calling attemptChangeSize to a very large width/height, and I don't think we are applying any restrictions on the new width/height. User can always scroll away from the resized element.

Are we planning to apply any restriction on the percentage value? For example 100vw sounds reasonable, but to me 100vh does not.

Is % and vw vh the only three we want to add support to ?

coreybyrnes commented 7 years ago

@lannka I'm not familiar with attemptResize. Did you intend to indicate requestResize or attemptChangeSize instead? I feel this update would be helpful for both of them.

@zhouyx Is there documentation somewhere on attemptChangeSize? Looks like parameters were added to support for margins. And just like requestResize I assume the width and height parameters are numeric and are automatically assigned px as the unit? And yes in my opinion %, vw, and vh are the only additional units needing support.

@dvoytenko I'm actually not looking to expand to the full viewport. For further information, this request came about because I'm trying to set the width and height of the tag within a flying-carpet component to be 100vw and 100vh so that the image contained in the is horizontally and vertically centered. Then the flying-carpet container element restricts the height to some smaller value to create the 'window' effect of the flying-carpet component. Without being able to center the image, it will always be top-left aligned. I have another ticket open for that specific work here so perhaps that should now be marked as related?

zhouyx commented 7 years ago

https://github.com/ampproject/amphtml/blob/master/src/service/resources-impl.js#L761 documented here. And yes the parameters are numeric and has px as the unit.

coreybyrnes commented 7 years ago

@zhouyx Thanks for the link. I'd like to try it out, but I'll need to pass the as the element (first param of attemptChangeSize). Is there a way to reference that from within a DFP ad? I can't get it because of js security issues. Perhaps it can be referenced as a part of window.parent.context?

jridgewell commented 7 years ago

@coreybyrnes: Ads only have access to window.context.requestResize(width, height). #attemptChangeSize is only available to the AMP runtime (requestResize uses it internally).

coreybyrnes commented 7 years ago

@jridgewell Good to know - thanks.

So can the original request to allow units be a part of the requestResize continue?

jridgewell commented 7 years ago

Ok, discussed this in design-review today. The use case right now seems solvable by specifying width="100%" height="100%" or width="100vw" height="100vh" on whatever amp-element creates the iframe. So for ads:

<amp-ad type="..." width="100vw" height="100vh">
</amp-ad>

This, coupled with the current pixel resize API, seems to cover everything necessary.

coreybyrnes commented 7 years ago

I think being able to specify a unit other than the assumed px is still a valid request so IMHO this should be reopened for further conversation.

As for the proposed solution, if the ad size needs to match the width and height attributes in the tag there is no way to get a creative to serve because there's no "100vw x 100vh" ad size option in DFP.

lannka commented 7 years ago

@coreybyrnes we think it should be publisher's control to decide the width of the ad. If pub puts ad in a small fixed width, ad should not enlarge itself to full viewport width which may lead to an overflow.

Can you give example why this is necessary?

coreybyrnes commented 7 years ago

The issue I'm trying to solve for is #8097. There is no way to horizontally & vertically center the image asset served in a DFP ad into the flying-carpet component if the image is larger than the ad size and therefore the ad container (since the ad size determines the width & height of the container).

jridgewell commented 7 years ago

Reopening due to https://github.com/ampproject/amphtml/issues/8097#issuecomment-286897296. Ad servers only understand absolute pixels, which means you have to use width="320" height="100".

ampprojectbot commented 6 years ago

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

ampprojectbot commented 6 years ago

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

ampprojectbot commented 6 years ago

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

zhouyx commented 6 years ago

I think it's reasonable to expose the requestResizes API with unit other than px feature to 3P iframes. internal attemptChangeSize() API only accepting px.

But is the feature still required since DPF ad has migrated to using Fast Fetch? to @lannka

lannka commented 6 years ago

Closing this because doubleclick supports specifying ad dimension explicitly in AMP now: https://github.com/ampproject/amphtml/issues/8097#issuecomment-406732234