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

Only allowing amp-audio layout=nodisplay within amp-story. #20969

Merged
merged 1 commit into from Feb 21, 2019

Conversation

gmajoulet
Copy link
Contributor

@gmajoulet gmajoulet commented Feb 20, 2019

Forcing amp-audio within amp-story to use layout="nodisplay".

Using it with any other display breaks in different ways:

  • The underlying <audio> element is created at a different time, potentially creating a race condition that results in the audio element not being handled by amp-story. It'd play when it's not supposed to, and not react to the story mute/unmute buttons.
  • The audio element would be visible, but trying to interact with its controls would just navigate to the previous/next page instead of playing the audio.

Copy link
Contributor

@newmuis newmuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding in @aghassemi, who owns amp-audio

@Gregable
Copy link
Member

Approved for validator changes, not for the decision itself.

Copy link
Member

@Gregable Gregable left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, scratch that. This change potentially breaks existing pages. Do you have a reason to believe that this will not be the case?

@gmajoulet
Copy link
Contributor Author

We've been talking offline about this, I should have added more context:

If any publisher is currently using that, the pages would be very broken already (cf PR description). The sound would be playing right away on page load on desktop, and as you navigate on mobile. And all that even if the story is muted :((. Also, the audio controls would not work, trying to interact with the audio element would trigger a page navigation instead of a audio play or other action.
Providing a fix today would require quite some work, both engineering to fix the race conditions and UX to make the audio controls interactive. We don't really have a use case for these today.

Copy link
Contributor

@aghassemi aghassemi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gmajoulet I assume amp-story has its own UI control for audio and amp-audio is purely a sound source within amp-story?

@gmajoulet
Copy link
Contributor Author

Correct :)
The audio plays for the visible page only, and users can mute/unmute it using this top right button

@Gregable
Copy link
Member

We've been talking offline about this, I should have added more context:

If any publisher is currently using that, the pages would be very broken already (cf PR description). The sound would be playing right away on page load on desktop, and as you navigate on mobile. And all that even if the story is muted :((. Also, the audio controls would not work, trying to interact with the audio element would trigger a page navigation instead of a audio play or other action.
Providing a fix today would require quite some work, both engineering to fix the race conditions and UX to make the audio controls interactive. We don't really have a use case for these today.

Ack. I also confirmed that this wouldn't break anything in at least a small sample.

@gmajoulet
Copy link
Contributor Author

Thanks everybody. :)

@gmajoulet gmajoulet merged commit 5d3a281 into ampproject:master Feb 21, 2019
Gregable added a commit that referenced this pull request Feb 27, 2019
* cl/234896906 Revision bump for #20969

* cl/235090073 Allow i-amphtml-layout on AMP elements only for Transformed AMP

* cl/235216325 Disallow document properties in <form> attribute name.

* cl/235271919 Revision bump for #20989

* cl/235833553 Revision bump for #20928

* cl/235843207 Revision bump for #20905
noranazmy pushed a commit to noranazmy/amphtml that referenced this pull request Mar 22, 2019
noranazmy pushed a commit to noranazmy/amphtml that referenced this pull request Mar 22, 2019
* cl/234896906 Revision bump for ampproject#20969

* cl/235090073 Allow i-amphtml-layout on AMP elements only for Transformed AMP

* cl/235216325 Disallow document properties in <form> attribute name.

* cl/235271919 Revision bump for ampproject#20989

* cl/235833553 Revision bump for ampproject#20928

* cl/235843207 Revision bump for ampproject#20905
bramanudom pushed a commit to bramanudom/amphtml that referenced this pull request Mar 22, 2019
bramanudom pushed a commit to bramanudom/amphtml that referenced this pull request Mar 22, 2019
* cl/234896906 Revision bump for ampproject#20969

* cl/235090073 Allow i-amphtml-layout on AMP elements only for Transformed AMP

* cl/235216325 Disallow document properties in <form> attribute name.

* cl/235271919 Revision bump for ampproject#20989

* cl/235833553 Revision bump for ampproject#20928

* cl/235843207 Revision bump for ampproject#20905
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants