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

Simplify ActionTrust to two tiers #10633

Merged
merged 1 commit into from Jul 29, 2017

Conversation

dreamofabear
Copy link

@dreamofabear dreamofabear commented Jul 25, 2017

Fixes #10166 and #9953.

Discussed with @ericlindley-g.

  • Reduce granularity of ActionTrust from three to two tiers.
  • All current events and actions have/require "high" trust, which means that ActionTrust as a whole will effectively be a no-op for now (until we have low-trust events).

@dreamofabear
Copy link
Author

/to @aghassemi /cc @cramforce

ActionTrust.MEDIUM);
// Only require ActionTrust.LOW for video actions to defer to platform
// specific handling (e.g. user gesture requirement for unmuted playback).
video.registerAction('play',
Copy link
Contributor

Choose a reason for hiding this comment

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

let's keep all of these at HIGH as well. Aside from unmuted play, rest need to be HIGH regardless.

Copy link
Author

Choose a reason for hiding this comment

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

@aghassemi Malte suggested that we should just defer to the browser for video actions to allow different handling on desktop vs. mobile. WDYT?

Copy link
Contributor

@aghassemi aghassemi Jul 27, 2017

Choose a reason for hiding this comment

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

I see, works as long as AMP and platform's idea of good UX don't differ in regards to these actions. One concern is unmute on desktop (without high trust actions) where AMP can so "no no" but platform can say "yes". But both Safari and Chrome are aligning desktop and mobile policies now so I am okay with allowing platform to handle this (can't think of any other issue beside unmute either)

Copy link
Contributor

Choose a reason for hiding this comment

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

@aghassemi — can you elaborate on this? Are there conditions where unmuted playback could happen without user action?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not at the moment since we don't have any LOW trust events. But since we are keeping unmute at LOW, if we ever expose a LOW trust event (e.g. onload for amp-list?) then on certain platforms unmuted playback can happen.

@dreamofabear dreamofabear merged commit 0ad36da into ampproject:master Jul 29, 2017
@dreamofabear dreamofabear deleted the action-trust-v2 branch July 29, 2017 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants