amp-consent iframe modal UI by micajuine-ho · Pull Request #29204 · ampproject/amphtml

micajuine-ho · 2020-07-08T20:48:23Z

Overview:

Created new option for consent prompt iframe to appear as a modal rather than a bottom sheet, via promptUiSrc
modal mode will always include the overlay and force borders/box shadow
In the ready postMessage sent by the iframe, any vh value between (60-80] will trigger the new modal UI
Fullscreen requests from the iframe will be ignored if modal is enabled
Will send the Viewer a request to enter fullOverlay mode
Using CSS to create the modal effect (not the viewport service)
Initial Design Doc

Changes:

Change vh restriction received from the ready message from [30-60] to [30-80]
- Values of (60-80] will trigger modalEnabled_ and overlayEnabled_
Create new CSS class: .modal and modal.enableBorder
- .modal has !important styles to override some iframeActive rules
- Use CSS var substitution (--i-amp-modal-height) set on the element style to be '${initialHeight}vh' to customize the height of the modal
- Created borderEnabled to round all corners of the modal
Request fullOverlay from Viewer to hide Viewer header (similar to fullscreen mode)
If modalEnabled_, don't allow iframe to enter fullscreen
Onhide(), remove .modal & .borderEnabledModal
Tests:
- show and hide for modal
- no fullscreen
- force overlay + borders
- viewer messages being sent
Added documentation for initialHeight & enableBorder

Notes:

Default width: 90vw is used for the modal. Essentially, a margin of 5 on left and right to clearly show that the consent prompt is appearing above user content.
Max 80vh to show that consent prompt is appearing above user content (giving more room for underlying content to be shown). This was found to be a safe value when tested in all different browsers (no conflicts with navigation bar etc...). Additionally, there is no real difference between 80vh and 90vh unless all of the content will be shown in 90vh vs 80vh and thus no scrolling. Otherwise, user will still have to scroll down to the bottom to reach the consent decision buttons.

Normal Ampdoc:

Simulated Viewer:

Widescreen Desktop:

extensions/amp-consent/0.1/amp-consent.css

zhouyx · 2020-07-10T23:38:18Z

extensions/amp-consent/0.1/amp-consent.css

+  background: white;
+  flex-direction: column;
+  height: var(--lightbox-height);
+  width: 90vw;


Out of curiously, how is the 90vw picked?

Confirmed with Zewen: We wanted a margin on the left and right to clearly show that the consent prompt is appearing above user content, which would be better UX.

Have we tested desktop (wide viewports)? This might need a max-width in pixels.

Yes, Zewen said it is best practice to include a max-width so that the text isn't too stretched and hard to read. He suggested a value of 760px. Updated the description.

extensions/amp-consent/0.1/amp-consent.css

extensions/amp-consent/0.1/consent-ui.js

hrkhal · 2020-07-13T09:52:15Z

@micajuine-ho Why is the maximum height 80vh? Can this be increased to 90/95vh while additionally hiding the AMP viewer when the lightbox is launched?

Currently when we go into fullscreen we'd get 100vh and the AMP viewer would be hidden for maximum space.
#23491

With this PR as it stands we'd be gaining an extra 20vh for the initial prompt but then losing 20vh from the settings view of our CMP.

This is not an ideal compromise.

extensions/amp-consent/integrating-consent.md

micajuine-ho · 2020-07-15T21:09:14Z

Hi @hrkhal Thanks for your feedback.

Why is the maximum height 80vh?

This is safe height for the consent iframe; it creates a large enough margin on the top and bottom (on all major browsers), to clearly show users that the consent prompt is above their content and that they must interact with it.

Can this be increased to 90/95vh

While we could increase the VH limit, the more we do so, the less apparent it will become to the user that their content is being blocked by this consent prompt, which would lead to a bad user experience. This problem is similar to issue #26197 (since lightbox mode would trigger without user interaction), and we would like to avoid that type of situation. Additionally, a change from 80vh to say 90vh should not have much impact, unless all the the disclaimers, information, and buttons will be shown in 90vh rather than 80vh.

while additionally hiding the AMP viewer when the lightbox is launched?

Thanks for the suggestion! I have edited the PR to include this.

With this PR as it stands we'd be gaining an extra 20vh for the initial prompt but then losing 20vh from the settings view of our CMP.

The intent of this feature is to allow a way for more information to be shown immediately on iframe loading, while maintaining a good UX. Can you elaborate on what you mean by "settings view".

zhouyx

Looks good to me!

A few things that I'd like the UI team to review

Since this is a lightbox experience. Is it possible to reuse methods/css from amp lightbox?
When the consent UI takes the entire screen, does it affect other UI component. For example I found amp-side-bar has a higher z index compared to amp-consent.

extensions/amp-consent/0.1/amp-consent.css

hrkhal · 2020-07-24T16:33:05Z

@micajuine-ho The settings view would be the second layer of a GDPR TCFv2 prompt where you're able to review your consent status for all of the vendors and purposes. See the below image.

Loosing height here makes an already cramped UX worse. We can obviously rejig the UI to make better use of the space but in general more height is preferable.

What's the eta for getting this into production? Cheers.

micajuine-ho · 2020-07-24T17:27:25Z

The settings view would be the second layer of a GDPR TCFv2 prompt where you're able to review your consent status for all of the vendors and purposes. See the below image.

How would you get to this view? By hitting the manage button after making an initial consent decision?

What's the eta for getting this into production?

We are still waiting for review from the UI team (ping @alanorozco).

hrkhal · 2020-07-27T07:33:11Z

@micajuine-ho:

How would you get to this view? By hitting the manage button after making an initial consent decision?
Users can reach this via the the initial prompt as well as the manage button after an initial consent decision.

In our case the user would hit a cookie settings button (instead of a consent to all button) on the initial prompt that would allow them to configure their individual consent statuses before providing us with their overall consent.

We are still waiting for review from the UI team

Understood. I'd like to state that the deadline for TCFv2 is fast approaching (15th August 00:00 BST) and as a publisher we'll need a few days to implement this light box change and have it reviewed internally before we could get it to production. Ideally we'll be using this UI on day one.

alanorozco

Let's reconsider how this is done conditionally.

Maybe we can remove a couple of clauses for checking whether it's a lightbox, and use compound CSS selector for the styling props that change instead.

Consider also sharing class names that aren't necessary for either mode, but have no effect to reduce complexity.

alanorozco · 2020-07-27T22:36:53Z

extensions/amp-consent/0.1/amp-consent.css

 }

-amp-consent.i-amphtml-consent-ui-iframe-active.i-amphtml-consent-ui-enable-border {
+amp-consent.i-amphtml-consent-ui-enable-border {


I think for customization we should leave these classnames open (without i-amphtml) and use !important properties for whatever we find necessary. IMO, border-radius and box-shadow should be able to be modified.

Customization could be a nice feature for developers. I am down to address these changes in a future PR, since removing !important could be breaking.

alanorozco · 2020-07-27T22:37:33Z

extensions/amp-consent/0.1/amp-consent.css

+  background: white;
+  flex-direction: column;
+  height: var(--lightbox-height);
+  width: 90vw;


Have we tested desktop (wide viewports)? This might need a max-width in pixels.

extensions/amp-consent/0.1/amp-consent.css

alanorozco · 2020-07-27T22:39:14Z

extensions/amp-consent/0.1/consent-ui.js

        const dataHeight = parseInt(data['initialHeight'], 10);
+        // Set initialHeight to max height fallback if applicable
+        this.initialHeight_ =
+          dataHeight >= 80 ? MAX_INITIAL_HEIGHT : this.initialHeight_;


Why 80? Name this number as a constant if arbitrary, for future context.

80 is the MAX_INITIAL_HEIGHT agreed upon

Meaning, maxInitialHeightForModal or how would you express it? It's just about giving it a name in the code.

extensions/amp-consent/0.1/consent-ui.js

alanorozco · 2020-07-27T22:46:45Z

extensions/amp-consent/0.1/consent-ui.js

+    if (this.enableBorder_ && !this.lightboxEnabled_) {
      classList.add(consentUiClasses.enableBorder);
+    } else if (this.lightboxEnabled_) {
+      classList.add(consentUiClasses.enableBorderLightbox);
+      this.sendViewerMessage_(REQUEST_OVERLAY);


Flip to simplify conditional clause.

Suggested change

if (this.enableBorder_ && !this.lightboxEnabled_) {

classList.add(consentUiClasses.enableBorder);

} else if (this.lightboxEnabled_) {

classList.add(consentUiClasses.enableBorderLightbox);

this.sendViewerMessage_(REQUEST_OVERLAY);

if (this.lightboxEnabled_) {

classList.add(consentUiClasses.enableBorderLightbox);

this.sendViewerMessage_(REQUEST_OVERLAY);

} else if (this.enableBorder_) {

classList.add(consentUiClasses.enableBorder);

Also, consider flipping enableBorder_ to match lightboxEnabled_, that way you can use a compound selector and only send the message for lightbox.

In any case, you should use the Viewport service for this, don't talk to the viewer directly.

Suggested change

if (this.enableBorder_ && !this.lightboxEnabled_) {

classList.add(consentUiClasses.enableBorder);

} else if (this.lightboxEnabled_) {

classList.add(consentUiClasses.enableBorderLightbox);

this.sendViewerMessage_(REQUEST_OVERLAY);

if (this.enableBorder_) {

// This classname is applied differently by CSS selector

classList.add(consentUiClasses.enableBorder);

}

if (this.lightboxEnabled_) {

this.viewport_.enterLightboxMode();

}

In any case, you should use the Viewport service for this, don't talk to the viewer directly.

Following the precedence set here #23491; basically this.viewport_.enterLightboxMode() does more work than we need.

Which work is unnecessary? If it doesn't affect the use case negatively, I'd say it'd be better to consolidate implementations rather than duplicate them.

Yeah, I agree.

zhouyx

LG on my side. Please get approval on the UI changes from the UI team.

alanorozco

Just nits, I think there are opportunities to simplify but overall LGTM.

extensions/amp-consent/0.1/amp-consent.css

extensions/amp-consent/0.1/consent-ui.js

* init, no tests * tests * documentation + cleanup * Suggested Changes * Suggested Changes * Lightbox => Modal, housekeeping, clarity * Compound classes

init, no tests

6cafcce

googlebot added the cla: yes label Jul 8, 2020

tests

8e7a2e2

micajuine-ho mentioned this pull request Jul 9, 2020

[Discussion] <amp-consent> TCFv2 Support #26229

Open

documentation + cleanup

1d6a515

micajuine-ho force-pushed the lightbox branch from bca1303 to 1d6a515 Compare July 9, 2020 23:43

micajuine-ho changed the title ~~[WIP] amp-consent iframe lightbox UI~~ amp-consent iframe lightbox UI Jul 10, 2020

micajuine-ho requested a review from zhouyx July 10, 2020 00:43

zhouyx reviewed Jul 11, 2020

View reviewed changes

hrkhal reviewed Jul 13, 2020

View reviewed changes

extensions/amp-consent/integrating-consent.md Outdated Show resolved Hide resolved

Micajuine Ho added 2 commits July 14, 2020 08:40

Suggested Changes

7a12a63

Suggested Changes

b62a3c8

Merge branch 'lightbox' of github.com:micajuine-ho/amphtml into lightbox

38d8397

micajuine-ho force-pushed the lightbox branch from 5653553 to 38d8397 Compare July 15, 2020 22:03

micajuine-ho requested review from alanorozco and zhouyx July 15, 2020 22:04

zhouyx reviewed Jul 17, 2020

View reviewed changes

extensions/amp-consent/0.1/amp-consent.css Outdated Show resolved Hide resolved

extensions/amp-consent/0.1/amp-consent.css Outdated Show resolved Hide resolved

micajuine-ho force-pushed the lightbox branch from f8ae58c to 39a70f9 Compare July 17, 2020 18:54

Merge branch 'master' into lightbox

3bdca8f

micajuine-ho force-pushed the lightbox branch from 39a70f9 to 3bdca8f Compare July 17, 2020 18:56

zhouyx mentioned this pull request Jul 20, 2020

wg-analytics Status Update 2020-07-20 ampproject/wg-analytics#28

Closed

alanorozco requested changes Jul 27, 2020

View reviewed changes

Lightbox => Modal, housekeeping, clarity

9edc41a

micajuine-ho force-pushed the lightbox branch from cce4f81 to 9edc41a Compare July 29, 2020 19:00

micajuine-ho requested a review from zhouyx July 29, 2020 19:02

micajuine-ho requested a review from alanorozco July 29, 2020 19:02

zhouyx approved these changes Jul 31, 2020

View reviewed changes

micajuine-ho changed the title ~~amp-consent iframe lightbox UI~~ amp-consent iframe modal UI Jul 31, 2020

alanorozco approved these changes Aug 12, 2020

View reviewed changes

extensions/amp-consent/0.1/amp-consent.css Outdated Show resolved Hide resolved

extensions/amp-consent/0.1/consent-ui.js Outdated Show resolved Hide resolved

extensions/amp-consent/0.1/consent-ui.js Outdated Show resolved Hide resolved

Compound classes

e6b00fd

micajuine-ho force-pushed the lightbox branch from 7a15180 to e6b00fd Compare August 12, 2020 21:35

micajuine-ho merged commit 3970e5e into ampproject:master Aug 12, 2020

ampprojectbot added the PR use: In Beta / Experimental label Aug 18, 2020

Conversation

micajuine-ho commented Jul 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

micajuine-ho Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hrkhal commented Jul 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

micajuine-ho commented Jul 15, 2020

Uh oh!

zhouyx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hrkhal commented Jul 24, 2020

Uh oh!

micajuine-ho commented Jul 24, 2020

Uh oh!

hrkhal commented Jul 27, 2020

Uh oh!

alanorozco left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

micajuine-ho Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhouyx left a comment

Choose a reason for hiding this comment

Uh oh!

alanorozco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

micajuine-ho commented Jul 8, 2020 •

edited

Loading

micajuine-ho Jul 29, 2020 •

edited

Loading

hrkhal commented Jul 13, 2020 •

edited

Loading

micajuine-ho Jul 28, 2020 •

edited

Loading