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 story auto-advance-after a background-audio skips the page on iOS #33831

Open
mszylkowski opened this issue Apr 14, 2021 · 4 comments
Open

Comments

@mszylkowski
Copy link
Contributor

When setting the auto-advance-after to the pageID it should use the background-audio length to advance, but on iOS (tested on Safari) it auto-advances the page really fast. This makes it impossible to navigate backwards.

<amp-story-page id="p8" auto-advance-after="p8" background-audio=".audio.mp3">
</amp-story-page>

The background audio does load and play if the auto-advance is disabled on the page, so the problem should not be related to the audio file.

cc @ampproject/wg-stories

@mszylkowski mszylkowski added this to To do - P1 in wg-stories Fixit via automation Apr 14, 2021
@mszylkowski mszylkowski moved this from To do - P1 to To do - P2 in wg-stories Fixit Apr 14, 2021
@newmuis
Copy link
Contributor

newmuis commented Apr 14, 2021

Technically I don't think this is supported (the ID is supposed to be that of a media element), but I agree we should either (a) support this, or (b) fail more explicitly (e.g. console error that says this is not supported).

@gmajoulet
Copy link
Contributor

It's actually supported, I'd be curious to know when it broke and how it's broken.

Code:

if (
matches(
pageEl,
`amp-story-page[background-audio]#${escapeCssSelectorIdent(
autoAdvanceStr
)}`
)
) {
element = pageEl;
}

Test:

it('should return a media advancement for amp-story-page with background audio', () => {
const pageEl = html`
<amp-story-page
id="story-id"
auto-advance-after="story-id"
background-audio="foo.mp3"
>
</amp-story-page>
`;
const advancement = AdvancementConfig.forElement(win, pageEl);
expect(advancement).to.be.instanceOf(MediaBasedAdvancement);
});

Given we have a unit test to make sure we create the correct advancement mode, I'd think the code fails here when we try to retrieve the audio element:

} else if (
this.element_.hasAttribute('background-audio') &&
tagName === 'amp-story-page'
) {
return this.element_.querySelector('.i-amphtml-story-background-audio');

@mszylkowski
Copy link
Contributor Author

mszylkowski commented Apr 14, 2021

The functionality works on Desktop and Android but not iOS, which is weird. The Q/A story we use for audio has an example of this, but the Q/A story has an mp4 video as background-audio, which might contribute to the bug not triggering. @hongcatlover sent me a video of this bug and was able to repro as well with an mp3 audio file

@newmuis
Copy link
Contributor

newmuis commented Apr 14, 2021

Was the story muted when you tried this? I don't know whether the sequence of events for when you try to play a muted audio file is the same cross-browser (but, for playing muted video, the behavior is well-defined).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
wg-stories Fixit
  
To do - P2
Development

No branches or pull requests

4 participants