Exposing play, pause, mute, unmute actions and making integration tests run on iOS 10 by aghassemi · Pull Request #7171 · ampproject/amphtml

aghassemi · 2017-01-24T01:49:42Z

This CL:

Exposes play, pause, mute, unmute actions on amp-video, amp-youtube
Adds muted and unmuted events to the video-interface
Adds new examples pages for amp-video, amp-youtube
Changes the layout of of amp-actions-and-events.md
Unskips video player integration tests in Safari. Now the tests run and pass on iOS 10.
Closes Intend to Implement: play, pause, mute, unmute AMP actions for any player that implements the video-interface #6198
Fixes iframed integration tests should get the IosEmbed not the Natural viewport binding #7143

…layactions

aghassemi · 2017-01-24T16:43:01Z

spec/amp-actions-and-events.md

 ```

 ## Element Specific Events
+### form


@bpaduch for review. I changed the layout of this page (now each component gets its own table rather than all in one table) also added actions and their description for amp-video, amp-youtube.

It seems good. I would recommend putting the elements in alphabetical order (just move sidebar before user-notification).

Also, please add ending punctuation to each description (ie., just needs a period if the sentence doesn't already have one).

aghassemi · 2017-01-24T16:46:30Z

src/service/viewport-impl.js

    return ViewportType.NATURAL_IOS_EMBED;
  }
+
+  // Enable iOS Embedded mode for iframed tests (e.g. integration tests).


@dvoytenko When running integration tests in iOS 10, I was getting the Natural instead of Embed iOS viewport. That was causing certain parts of the test (around scrolling and viewability) to get the wrong values. I think this change makes sense, please confirm.

@aghassemi This is a bit tricky and confusing. We emulate standalone in tests as an iframe. This is because the only way we can run a test is in an iframe. Ideally, we need to be able to indicate whether (while always an iframe) we want to test an iframing viewer case vs standalone case (vs even in-a-box) case. I'm not sure how to communicate all that here... If all tests are ok in this way - we can use this mode and then we can delay accommodating other use cases when we run into them.

Discussed offline. Tests pass with this.

ghost · 2017-01-24T16:47:08Z

spec/amp-actions-and-events.md

+</table>
+
+### amp-sidebar
+<table>


Needs a header row like the others

aghassemi · 2017-01-24T16:49:01Z

/to @mkhatib and @chenshay for review
/cc @dvoytenko

ghost · 2017-01-24T16:51:51Z

spec/amp-actions-and-events.md

  </tr>
  <tr>
-    <td width="30%">amp-image-lightbox</td>
    <td>(default)</td>


Shouldn't this row be specific to the amp-image-lightbox element? Also, should the action be named as open (default) instead of (default)?

good catch. done.

ghost

Minor changes needed.

ghost

Approve changes for MD file.

mkhatib

LGTM - please make sure to test in /pwa/ and /a4a/ manually as mentioned.

mkhatib · 2017-01-24T19:25:54Z

extensions/amp-youtube/0.1/amp-youtube.js

-    installVideoManagerForDoc(this.getAmpDoc());
+    const ampdoc = this.getAmpDoc();
+    installVideoManagerForDoc(ampdoc);
+    videoManagerForDoc(this.win.document).register(this);


Any reason why videoManagerForDoc is using this.win.document instead of ampdoc? Using win.document might break YouTube inside shadow-doc/embedded use cases. You can test these manually by using the /pwa/ and /a4a/ prefixes before any example to see how it works. For example to test the amp-youtube.amp.max.html example this would be the URLs:

http://localhost:8000/pwa/examples/amp-youtube.amp.max.html
http://localhost:8000/a4a/examples/amp-youtube.amp.max.html

The only gotcha for the pwa example you'd probably need to make sure the service worker installed so it doesn't redirect you to the actual example instead of embedding it into shadow-doc

You could also possibly use this.element for any _ForDoc service call.

cc/ @dvoytenko for any mis-info I am spreading 😄

I'd just recommend using installVideoManagerForDoc(this.element). And, yes, this should definitely not be win.document. Is something not working? Maybe this is just a incorrect rebase?

done (including other cases in video players)

Still seeing it. Did it get pushed?

ugh, made the change on laptop, forgot to push. should be in now

mkhatib · 2017-01-24T19:31:54Z

extensions/amp-youtube/0.1/amp-youtube.js

-    ampdoc.win.addEventListener(
+
+
+    this.win.addEventListener(


I think this is accurate but please test the /pwa/ and /a4a/ cases and make sure it is behaving as expected.

ack. looks fine in both

aghassemi · 2017-01-26T01:05:40Z

extensions/amp-youtube/0.1/amp-youtube.js

-    const ampdoc = this.getAmpDoc();
-    ampdoc.win.addEventListener(
+
+


to self: use iframe messaging helpers

will have a separate PR for this.

aghassemi · 2017-01-27T23:14:42Z

@mkhatib @dvoytenko PTAL. Thanks.

aghassemi · 2017-01-27T23:16:02Z

spec/amp-actions-and-events.md

 ```

 ## Element Specific Events
+### amp-carousel


@choumx I moved things around here a bit. Please take a quick look to make sure I did not mess up your recent changes to this file.

dvoytenko

LGTM on my side as soon as the changes to viewerManager lookup are done.

mkhatib

LGTM

…ts run on iOS 10 (ampproject#7171)

aghassemi added 9 commits January 19, 2017 13:16

a

d80012c

Merge branch 'master' of github.com:ampproject/amphtml into playactions

15ae275

a

9300e71

Merge branch 'master' of https://github.com/ampproject/amphtml into p…

c370f19

…layactions

a

41ea2a7

Fix viewer in test case

93cea51

Merge branch 'master' of github.com:ampproject/amphtml into playactions

524868f

tests

81f020b

lint

9daca06

aghassemi added the WIP label Jan 24, 2017

aghassemi added 3 commits January 23, 2017 18:02

merge

0872e54

Lint

f2f92f4

Lint

87d8e3f

aghassemi commented Jan 24, 2017

View reviewed changes

ghost reviewed Jan 24, 2017

View reviewed changes

Lint

b233f63

aghassemi removed the WIP label Jan 24, 2017

aghassemi requested review from chenshay and mkhatib January 24, 2017 16:48

ghost reviewed Jan 24, 2017

View reviewed changes

ghost suggested changes Jan 24, 2017

View reviewed changes

docs review comments

a10d4a0

ghost approved these changes Jan 24, 2017

View reviewed changes

mkhatib suggested changes Jan 24, 2017

View reviewed changes

aghassemi commented Jan 26, 2017

View reviewed changes

aghassemi added 2 commits January 27, 2017 15:01

Merge branch 'master' of github.com:ampproject/amphtml into playactions

37caef2

Fix bad merge

60e4abc

aghassemi commented Jan 27, 2017

View reviewed changes

dvoytenko approved these changes Jan 28, 2017

View reviewed changes

aghassemi added 2 commits January 27, 2017 16:39

Review comments

c5ee8b8

type check

2df18dc

mkhatib approved these changes Jan 31, 2017

View reviewed changes

aghassemi merged commit cd884cf into ampproject:master Jan 31, 2017

torch2424 pushed a commit to torch2424/amphtml that referenced this pull request Feb 14, 2017

Exposing play, pause, mute, unmute actions and making integration tes…

34fa9d1

…ts run on iOS 10 (ampproject#7171)

mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017

Exposing play, pause, mute, unmute actions and making integration tes…

362dd6c

…ts run on iOS 10 (ampproject#7171)

		const ampdoc = this.getAmpDoc();
		ampdoc.win.addEventListener(

Conversation

aghassemi commented Jan 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aghassemi commented Jan 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

mkhatib left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aghassemi Jan 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aghassemi commented Jan 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dvoytenko left a comment

Choose a reason for hiding this comment

Uh oh!

mkhatib left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aghassemi commented Jan 24, 2017 •

edited

Loading

aghassemi Jan 26, 2017 •

edited

Loading