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

video-manager: honour VideoEvents.VISIBILITY event #11787

Merged
merged 4 commits into from Oct 26, 2017

Conversation

aghassemi
Copy link
Contributor

Closes #8681

Value of getIntersectionChangeEntry is not always accurate in cases like Carousel. If VideoEvents.VISIBILITY says video is visible, honour it regardless of value of getIntersectionChangeEntry.

Something that might be fixed by layers, but this is a low-risk small fix for now.

@@ -833,6 +838,8 @@ class VideoEntry {
// Only muted videos are allowed to autoplay
this.video.mute();

//
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extraneous?

@aghassemi
Copy link
Contributor Author

@rsimha-amp I believe the Percy failure is a flake (also wondering how I can restart the Percy builds, could not find an option on their UI)

Copy link
Contributor

@cvializ cvializ left a comment

Choose a reason for hiding this comment

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

LGTM

@aghassemi aghassemi merged commit b8e6327 into ampproject:master Oct 26, 2017
@rsimha
Copy link
Contributor

rsimha commented Oct 27, 2017

@aghassemi Yes, the percy failure in https://percy.io/ampproject/amphtml/builds/385871 looks like flake. Adding @dvoytenko, who wrote that visual test.

There's no way to rerun a percy test similar to Travis. However, you can restart the "integration_test" shard of the travis run for that commit, which will go on to run a new percy build against the same commit.

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.

Middle video does not restart playback on carousel navigation
4 participants