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 ✨ 📖 Add SourcePoint to CMPs list #23917

Merged
merged 9 commits into from Sep 9, 2019
Merged

amp-consent ✨ 📖 Add SourcePoint to CMPs list #23917

merged 9 commits into from Sep 9, 2019

Conversation

andresilveirah
Copy link
Contributor

  • Add SourcePoint to the CMPs list
  • Add the required documentation
    • Example
    • Instructions
    • Front page

@zhouyx not sure if I'd have to tag you here since this is a PR related to <amp-consent> sorry if that's unnecessary.

@andresilveirah
Copy link
Contributor Author

Not sure what to do about the failure reported by Travis. It seems there're some dead links in the extensions/amp-consent/amp-consent.md file but none of them are related to my changes.

@andresilveirah
Copy link
Contributor Author

Really sorry to spam you @zhouyx but there's a small pressure on me to deliver our AMP solution 😬 Is there any way you could have a look at this PR?

@zhouyx
Copy link
Contributor

zhouyx commented Aug 15, 2019

Sorry about the delay here. Was back from a trip. Taking a look now

@zhouyx zhouyx self-requested a review August 16, 2019 00:04
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 integrating with AMP. I tried to test the page but get an 404.
image

Could you please provide a mock example that I can test manually? Thank you very much.

examples/cmp-vendors.amp.html Outdated Show resolved Hide resolved
extensions/amp-consent/amp-consent.md Outdated Show resolved Hide resolved
examples/cmp-vendors.amp.html Show resolved Hide resolved
examples/cmp-vendors.amp.html Show resolved Hide resolved
@andresilveirah
Copy link
Contributor Author

@zhouyx the check consent url was wrong, my bad. It should be working now.

@zhouyx
Copy link
Contributor

zhouyx commented Aug 17, 2019

@andresilveirah Thank you! I am able to see the example you provided now.

Two things I noticed

  1. The consent UI is resolved immediately. I assume this is because the server recognize me as a user which consent is not required. Do you know what would be the best way to see your UI
  2. I noticed the iframe calls ready then immediately full-screen then dismiss. Which lead to a flickering UI. Like this

ezgif com-video-to-gif (5)

@andresilveirah
Copy link
Contributor Author

Hi @zhouyx sorry for the delay. There was a mistake on my side. I just fixed that and, at least locally, it seems it's working.
Do you mind giving it another try?

@zhouyx
Copy link
Contributor

zhouyx commented Aug 21, 2019

Thank you @andresilveirah. I don't see the fullscreen now. However, the flickering UI still exist because the dismiss is received right after ready that we set the UI height to 30vh and then hide it right afterwards.

To fix this, I think we can follow either approach:

  1. Do not send the ready message, and ignore it if we are going to send dismiss later. This may delay the initial consent UI but it eliminate the case where we dismiss the UI right after displaying it.
    or
  2. More optimal, have the checkConsentHref endpoint tells AMP whenever consent is not required. So that no consent UI will be displayed.

Please note that dismiss is different from consent_not_required. the dismiss response may still block components depending on their own implementation logic. Want to confirm that sending the dismiss response is the expected behavior here. Thanks!

@andresilveirah
Copy link
Contributor Author

@zhouyx I see what's happening here. Our platform relies on cookies to determine if the user should see the consent message or not. There's a chance the user ends up in a state in which amp-consent thinks the message should be displayed but, according to the cookies stored, our platform decides otherwise (therefore calling dismiss right away). I think this happens when the localStorage is cleaned up but the cookies aren't.

I've just tested in an incognito window and it seems to be working as expected. The user first sees a fullscreen message, clicks on accept and, upon reloading the page, the flickering is gone.

Do not send the ready message, and ignore it if we are going to send dismiss later. This may delay the initial consent UI but it eliminate the case where we dismiss the UI right after displaying it.

I've tried that as well, but if I don't send the ready message, the "loading" spinner keeps on spinning indefinitely.
consent-spinner

More optimal, have the checkConsentHref endpoint tells AMP whenever consent is not required. So that no consent UI will be displayed.

Not sure this would be possible for us given our dependency on cookies to identify the user's consent state.

Having said all that. I've made some changes to our ui that addressed the flickering (at least from the user perspective) as well as the inconsistent state between amp-consent's localStorage and our cookies. Namely:

  • Storing the consent state into a cookie. If we land in that inconsistent state mentioned above, our ui will call accept or reject based on the consent state value.
  • If, however, the consent state cookie is not present and we're still in an inconsistent state, our UI will call dismiss faster, eliminating the flickering.

Could you give it yet another try and share your thoughts?

@zhouyx
Copy link
Contributor

zhouyx commented Aug 23, 2019

Thanks for the explanation. I think it is valid use case to load the iframe and then decide whether consent UI should be displayed to user or not.

I've tried that as well, but if I don't send the ready message, the "loading" spinner keeps on spinning indefinitely.

Thanks for reporting. This is a bug on our end. Fixed by #24105 which is merged.

Not sure this would be possible for us given our dependency on cookies to identify the user's consent state.

Got it. You're right, cookies won't be available in this case.

I'm not seeing the flickering anymore, thanks for the fix.

If, however, the consent state cookie is not present and we're still in an inconsistent state, our UI will call dismiss faster, eliminating the flickering.

By calling dismiss faster is it guaranteed to eliminate the flickering, or that happens at a lower percentage? With #24105 in, do you think it's worth it to have ready wait for the dismiss message in this case. I'll let you make the final call here.

One more thing I found.
image

I got this error when clicking cancel button at the fullscreen. And the iframe won't get hidden because of the error. Could you please fix it. Everything looks great otherwise.

@andresilveirah
Copy link
Contributor Author

andresilveirah commented Aug 26, 2019

By calling dismiss faster is it guaranteed to eliminate the flickering, or that happens at a lower percentage?

I believe it's not guaranteed to eliminate the flickering. There might exist a timing issue in which the consent ui code takes longer to be evaluated than the setTimeout responsible for calling dismiss. However, I'm unable to reproduce it. I have tried Network as well as CPU throttling and it still couldn't see any flickering.

With #24105 in, do you think it's worth it to have ready wait for the dismiss message in this case.

Not sure I understand the order of execution here. Given our ui decided the message won't show, but we do have consent info stored in the cookies, should we call dismiss then ready?

@zhouyx
Copy link
Contributor

zhouyx commented Aug 29, 2019

Hello @andresilveirah, sorry about the delay in response. I am traveling this week.

Not sure I understand the order of execution here. Given our ui decided the message won't show, but we do have consent info stored in the cookies, should we call dismiss then ready?

Great question. You're right, the most common execution would be ready -> accept/reject/dismiss based on user's interaction. But the two messages are designed for completely different purposes, and there's no required order on executing them. Readyis purely to tell AMP runtime that the consent UI is ready and to display it, whiledismissis used to inform AMP the user decision. In your case, I'd understand the situation as the UI is not ready until you look at the existing cookie, and senddismiss` message.

Let me know what do you think. Also if the PR is ready for review again. Thank you!

@andresilveirah
Copy link
Contributor Author

Hey @zhouyx sorry for the delay.
I believe we got all the issues fixed now including that nasty error message on the console.

Can you have another look?

I'm also bringing @mguerra10128 to the conversation since I'll be on vacation from Friday on.

@zhouyx
Copy link
Contributor

zhouyx commented Sep 9, 2019

Thank you @andresilveirah This looks great to me 🎉

@zhouyx zhouyx merged commit 851bf1d into ampproject:master Sep 9, 2019
@zhouyx
Copy link
Contributor

zhouyx commented Sep 9, 2019

Thank you for adding your support to AMP 🎉 🎉
PR merged. The change should be ready for dev opt-in this week. And ship in prod next week.

@andresilveirah
Copy link
Contributor Author

Thank you @zhouyx ! 🎉

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

3 participants