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.

Sign up for GitHub

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

Jump to bottom

Basic support for lightboxes in inabox #9646

Merged
merged 8 commits into from Jun 12, 2017
Merged

Basic support for lightboxes in inabox #9646

merged 8 commits into from Jun 12, 2017

Conversation

alanorozco
Copy link
Member

@alanorozco alanorozco commented May 31, 2017 •
edited

Summary

  • Changed signature of viewport services' enterLightboxMode method so that it's async. Updated tests to match.
  • Added postMessage API for full frame overlay requests.
  • Added host logic to update frames.
  • Updated inabox viewport binding to use postMessage API for overlay frames.

Upcoming work

  • Backport FIE logic into viewport service to match this implementation.
  • Update viewport measurements inside xorigin frame service to match resize.
  • Block zoom and scroll on host document when in full overlay frame mode.

@alanorozco alanorozco requested a review from lannka May 31, 2017 20:24
lannka
lannka reviewed Jun 8, 2017
View reviewed changes
src/3p-frame-messaging.js Outdated
// For the frame to be placed in full overlay mode for lightboxes
FULL_OVERLAY_FRAME: 'full-overlay-frame',
FULL_OVERLAY_FRAME_RESPONSE: 'full-overlay-frame-response',
RESET_FULL_OVERLAY_FRAME: 'reset-full-overlay-frame',
Copy link
Contributor

Choose a reason for hiding this comment

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

Naming suggestion: RESET -> CANCEL, FULL-> FULLSCREEN, and remove FRAME (given the context)

FULLSCREEN_OVERLAY:
CANCEL_FULLSCREEN_OVERLAY

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't like "fullscreen" specifically because it's not really fullscreen. It fills the entire viewport, but the browser chrome may still be visible. Changed reset to cancel.

src/inabox/inabox-viewport.js
// Move this where it makes sense
export function getFixedContainer(bodyElement) {
return dev().assertElement(childElementByTag(
dev().assertElement(bodyElement), 'amp-ad-banner'));
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm... i really wanted to get rid of this :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really possible when coordinating repositioning :)

src/inabox/inabox-viewport.js
requestFullOverlayFrame_() {
return new Promise((resolve, reject) => {
this.iframeClient_.makeRequest(
MessageType.FULL_OVERLAY_FRAME,
Copy link
Contributor

Choose a reason for hiding this comment

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

shall we introduce an ID? so that multiple calls won't affect each other.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would that be a problem? A sequence of the same call should have the same result anyways, so there's no need to map responses on the receiving end.

Copy link
Contributor

Choose a reason for hiding this comment

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

Really depends on if there will be multiple clients making the same request here

Copy link
Member Author

Choose a reason for hiding this comment

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

True, but I think it's also not an issue since the request-result-response mapping is 1:1:1 so if two overlay requests are done at the same time it doesn't matter if a client handles a response that is not meant for it.

lannka
lannka approved these changes Jun 9, 2017
View reviewed changes
ads/inabox/inabox-messaging-host.js Outdated
serializeMessage(
MessageType.FULL_OVERLAY_FRAME_RESPONSE,
request.sentinel,
{content: {accept: true}}),
Copy link
Contributor

Choose a reason for hiding this comment

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

naming consistency with resize API ->
success: true.

also, why nest the data in content?

Copy link
Member Author

Choose a reason for hiding this comment

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

I nested the content since serializeMessage adds top-level members and that's a little weird :/

Renaming to top-level success is still alright I guess. It's a simple API. Changed.

ads/inabox/inabox-messaging-host.js Outdated
* @param {string} origin
* @return {boolean}
*/
handleResetFullOverlay_(iframe, request, source, origin) {
Copy link
Contributor

Choose a reason for hiding this comment

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

handleExitFullOverlay

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to handleCancelFullOverlay_ for consistency with message type.

test/functional/test-viewport.js Outdated
expect(win.document.body.style.borderLeftStyle).to.not.equal('solid');
expect(win.document.body.style.borderRightStyle).to.not.equal('solid');
binding.updateLightboxMode(false).then(() => {
expect(win.document.body.style.borderTopStyle).to.equal('solid');
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think those assertion are run.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right! Fixed.

test/functional/inabox/test-inabox-viewport.js Outdated
@@ -118,4 +131,97 @@ describes.fakeWin('inabox-viewport', {amp: {}}, env => {
expect(binding.getLayoutRect(element))
.to.deep.equal(layoutRectLtwh(20, 20, 100, 100));
});

it('should center content and request resize on enter overlay mode', done => {
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of using done, return the promise.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done (hehe).

ads/inabox/util.js Outdated
* @visibleForTesting
*/
// TODO(alanorozco): Figure out a longer-term solution
export function fakeVsync(win, task, opt_state) {
Copy link
Contributor

Choose a reason for hiding this comment

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

call it something else. fake is not a good word in production code
something like runOnAnimationFrame ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to restrictedVsync.

@alanorozco alanorozco merged commit 76374a8 into ampproject:master Jun 12, 2017
@alanorozco alanorozco deleted the inabox-lightbox branch June 12, 2017 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lannka lannka lannka approved these changes

Assignees
No one assigned
Labels
cla: yes PR use: In Beta / Experimental
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@alanorozco @lannka @googlebot @ampprojectbot