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-audio default layout only works when script is cached #100

Closed
dvoytenko opened this issue Sep 12, 2015 · 10 comments · Fixed by #121
Closed

amp-audio default layout only works when script is cached #100

dvoytenko opened this issue Sep 12, 2015 · 10 comments · Fixed by #121
Assignees

Comments

@dvoytenko
Copy link
Contributor

/cc @rgthree @cramforce

For non-upgraded amp-audio it will have no chance to download the script and execute getBrowserAudioDefaultDimensions so it will fail in cases where amp-audio.js is not available quickly enough.

I only see two options:

  1. Create a fallback similar to getBrowserAudioDefaultDimensions where we'd take an amp element, remove "amp-" prefix, try to insert it into DOM and get the dimensions back - obviously cache those.
  2. Hardcode "amp-audio" default layout calculation in the runtime as an exception. Possibly we will have under 5 exceptions like this overall.
  3. Give it default layout, e.g. 1px/1px and eat the cost of the component re-laying itself.
@cramforce
Copy link
Member

  1. No special handling.

@dvoytenko
Copy link
Contributor Author

No special handling as in a layout type that would just let an element use
its default size? E.g. layout=default? Or layout=natural?
On Sep 11, 2015 7:37 PM, "Malte Ubl" [email protected] wrote:

  1. No special handling.


Reply to this email directly or view it on GitHub
#100 (comment).

@cramforce
Copy link
Member

That is not what I meant and while it would be cool I think it wouldn't
work due to async loading.

I meant: require specifying size.
On Sep 11, 2015 8:22 PM, "Dima Voytenko" [email protected] wrote:

No special handling as in a layout type that would just let an element use
its default size? E.g. layout=default? Or layout=natural?
On Sep 11, 2015 7:37 PM, "Malte Ubl" [email protected] wrote:

  1. No special handling.


Reply to this email directly or view it on GitHub
<#100 (comment)
.


Reply to this email directly or view it on GitHub
#100 (comment).

@cramforce
Copy link
Member

However, having a default size map (constant) in core would be fine with me.
On Sep 11, 2015 8:26 PM, "Malte Ubl" [email protected] wrote:

That is not what I meant and while it would be cool I think it wouldn't
work due to async loading.

I meant: require specifying size.
On Sep 11, 2015 8:22 PM, "Dima Voytenko" [email protected] wrote:

No special handling as in a layout type that would just let an element use
its default size? E.g. layout=default? Or layout=natural?
On Sep 11, 2015 7:37 PM, "Malte Ubl" [email protected] wrote:

  1. No special handling.


Reply to this email directly or view it on GitHub
<
https://github.com/ampproject/amphtml/issues/100#issuecomment-139703688>.


Reply to this email directly or view it on GitHub
#100 (comment)
.

@rgthree
Copy link
Contributor

rgthree commented Sep 13, 2015

After thinking about it, I don't think a default size map would work, there just too much variance. Instead, I'll have a PR that uses "natural" as a keyword for layout, height, or width that triggers a natural-size calculation based on the amp-xxx element name; essentially @dvoytenko #1 generic sizing idea.

@dvoytenko
Copy link
Contributor Author

If we go for "natural" - we don't really need to calculate width/height, right? E.g. we can do something like this:

NATURAL_FIXED_SIZE_ELEMENTS = {'amp-audio': true};

// I'm about to throw an exception because layout/width/height not defined, instead:
if (element.tagName in NATURAL_FIXED_SIZE_ELEMENTS) {
layout = Layout.FIXED;
// Don't throw an exception and let browser layout this element according to its defaults
}

@dvoytenko
Copy link
Contributor Author

In other words our main requirement that element's size doesn't change after loading. There's no risk of this with amp-audio, amp-pixel and such elements so we fulfill our requirements and we just need to express it in the code so that we don't trip up any exceptions.

@rgthree
Copy link
Contributor

rgthree commented Sep 13, 2015

It was my assumption that an element needs to reserve its height upfront such that any elements afterwards do not change their position. If we just squash the exception its height would still not be known until the extension js has loaded and calculated the default. Is that not an issue?

The PR also allows an element to use a different layout value, but still have natural calculation for a specific width/height attribute, in the case an author wanted an audio element to fill the width of a container, but wants it's height to the the browser's natural.

@dvoytenko
Copy link
Contributor Author

Yep, sorry. Running ahead of myself.

@dvoytenko
Copy link
Contributor Author

So, I agree. We could have a map that would allow a function for an element name that would return default layout, width and height.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants