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

🐛Fix lightbox transition when things on the page have z-index #20078

Merged
merged 3 commits into from Dec 26, 2018

Conversation

sparhami
Copy link

  • Remove z-index from lightbox element during transition since it creates a new stacking context
  • Move the transition layer into the lightbox element, give it the same z-index as the background
    • It will stack on top since it is after the background element
  • Add a sticky footer to the manual test

//cc @aghassemi

Fixes #20066

Copy link
Contributor

@cathyxz cathyxz left a comment

Choose a reason for hiding this comment

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

LGTM once we verify this works + doesn't break anything in the viewer.

@@ -277,7 +277,7 @@ amp-lightbox-gallery .amp-carousel-button {
.i-amphtml-lightbox-gallery-trans {
pointer-events: none !important;
position: fixed !important;
z-index: 1001 !important;
z-index: 2147483642 !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add documentation or at least a comment to the effect to say that elements with equal or higher z-index will still suffer from this problem until we fix it? Maybe just a comment here saying that this is necessary for fixing this bug here will suffice for now.

Copy link
Author

Choose a reason for hiding this comment

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

Added a comment.

@@ -277,7 +277,7 @@ amp-lightbox-gallery .amp-carousel-button {
.i-amphtml-lightbox-gallery-trans {
pointer-events: none !important;
position: fixed !important;
z-index: 1001 !important;
z-index: 2147483642 !important;
top: 0 !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since with the animation library, we no longer need this element to be sized to the viewport size anymore, let's see if we can move the top, bottom, left, right CSS for this and reduce some bloat?

Copy link
Author

Choose a reason for hiding this comment

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

Reworked this a bit and removed the transition layer element completely.

@sparhami sparhami merged commit d56fdd6 into ampproject:master Dec 26, 2018
noranazmy pushed a commit to noranazmy/amphtml that referenced this pull request Mar 22, 2019
…ject#20078)

* Fix amp-lightbox-gallery transition when things on the page have
z-index.
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

4 participants