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-pan-zoom] Fix bug where zoom breaks after toggling display #17451

Merged
merged 2 commits into from Aug 14, 2018

Conversation

cathyxz
Copy link
Contributor

@cathyxz cathyxz commented Aug 11, 2018

Fixes #17406. The resetContentDimensions in pauseCallback was originally made to reset after swiping between images in lightbox since carousel would call pauseCallback. Not applicable for current use case, though we may want to tweak it once we refactor image-viewer to use this.

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. Is pauseCallback called when style="display: none" is toggled?

@cathyxz
Copy link
Contributor Author

cathyxz commented Aug 14, 2018

@cvializ yes it is. But strangely only the first time it's toggled.

@cathyxz cathyxz merged commit c845439 into ampproject:master Aug 14, 2018
@cathyxz cathyxz deleted the bugfix/pan-zoom-toggle branch August 14, 2018 17:57
@cvializ
Copy link
Contributor

cvializ commented Aug 14, 2018

Weird, I wonder if that behavior is supposed to happen according to the CustomElement lifecycle docs. It looks like it should only be called when the ampdoc becomes inactive, not the individual element becoming invisible. @jridgewell can you confirm if this is expected behavior?

amphtml/src/custom-element.js

Lines 1172 to 1179 in 43be5f5

/**
* Requests the resource to stop its activity when the document goes into
* inactive state. The scope is up to the actual component. Among other
* things the active playback of video or audio content must be stopped.
*
* @package @final @this {!Element}
*/
pauseCallback() {

@jridgewell
Copy link
Contributor

Yes, it's expected.

@cathyxz
Copy link
Contributor Author

cathyxz commented Aug 14, 2018

I can look into this a bit more.

@jridgewell so it seems like if toggling display: none via the hidden property (via amp-bind) causes the component's pauseCallback to be called, that's a bug? Empirically, I know that amp-carousel manually calls pauseCallback when we switch slides, and I believe amp-lightbox possibly also does something similar to its children... So both of those components do not exactly follow this spec.

@jridgewell
Copy link
Contributor

It's not a bug. When a component becomes hidden, pauseCallback then unlayoutCallback is called.

@cathyxz
Copy link
Contributor Author

cathyxz commented Aug 14, 2018

Oh okay. Cool. Yes, that matches the behavior that I'm observing, so I think all's good.

@cvializ
Copy link
Contributor

cvializ commented Aug 14, 2018

Is this behavior documented anywhere currently? That jsdoc comment should be updated to fully describe the lifecycle

Enriqe pushed a commit to Enriqe/amphtml that referenced this pull request Nov 28, 2018
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.

None yet

5 participants