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-consent: Fixed the flash of the leftover CMP Iframe Container (Flash of teal) #20086

Merged
merged 2 commits into from Dec 28, 2018

Conversation

torch2424
Copy link
Contributor

@torch2424 torch2424 commented Dec 27, 2018

relates to #19969

This essentially reverts #19830 . I kept this PR in a note in case I noticed something weird down the line and I am glad I did 😂I think this should be undone for the following reasons:

  1. This was meant to fix amp-iframe can create multiple iframe when scheduled layout more than once #19599 , but we eventually found the actual fix was 🐛Amp-iframe: Only create one iframe #19603

  2. updateFixedLayer is only used in amp-live-list. And I think it's more for dynamically incoming multiple fixed elements.

  3. updateFixedLayer throws a console error about the element already having a top applied before being removed.

  4. removeFromFixedLayer appears to be the better option still, as we are hiding / removing the element, thus it should also be removed from the fixed layer, and not updated since it no longer exists.

The root cause however, is due to how the fixed layer will handle the top styling for elements. When we add it, the top is set to 0px for us. But when we use update to remove (which also removes all the classes), this top is re-applied which causes the flash, because the 100vh element gets pulled to the top, before being removed. removedFromFixedLayer, removes this top styling entirely (which is what we want).

@zhouyx Let me know what you think! 😄

Gifs

Bug

The low gif framerate may not show the bug, but still shows the console error.

flashbugampconsent

Fix

flashfixampconsent

Copy link
Contributor

@zhouyx zhouyx left a comment

Choose a reason for hiding this comment

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

Thank you for investigating! Really interesting found. Could you please add a comment to explain the reason and so we don't change it again. Basically say don't touch this : )

@torch2424 torch2424 merged commit 8243db9 into ampproject:master Dec 28, 2018
@torch2424 torch2424 deleted the amp-consent-ui-flash-fix branch December 28, 2018 00:24
noranazmy pushed a commit to noranazmy/amphtml that referenced this pull request Mar 22, 2019
…lash of teal) (ampproject#20086)

* Fixed flash of placeholder on consent UI

* Added comment about sensitive flashing
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.

amp-iframe can create multiple iframe when scheduled layout more than once
4 participants