amp-consent fullscreen warning#26467
Conversation
|
I'm not sure we're collecting dev warnings. Is that something we want to know about in our reporting? |
I don't think so, but I'll wait til @zhouyx is back for confirmation |
|
Discussed offline. AMP doesn't want to collect this error message. |
|
SGTM |
3f5d224 to
8ba2cc8
Compare
8ba2cc8 to
87dc001
Compare
87dc001 to
2dae6c1
Compare
| const error = 'Could not enter fullscreen'; | ||
| dev().warn(TAG, error); | ||
| this.sendIframeMessage_('Error', error); |
There was a problem hiding this comment.
Should there be two different errors sent to the iframe: 1 for if the frame isn't visible & another for if the iframe has not received user interaction first? @zhouyx
There was a problem hiding this comment.
That would be great. Or you could use one message that covers both, for example "Could not enter fullscreen, fullscreen is only supported when the iframe is visible and after user interaction" : )
| const error = 'Could not enter fullscreen'; | ||
| dev().warn(TAG, error); | ||
| this.sendIframeMessage_('Error', error); |
There was a problem hiding this comment.
That would be great. Or you could use one message that covers both, for example "Could not enter fullscreen, fullscreen is only supported when the iframe is visible and after user interaction" : )
67b1afa to
a46641a
Compare
zhouyx
left a comment
There was a problem hiding this comment.
Let's write down the API format before we make change to the PR. Thanks
| 'Could not enter fullscreen. Fullscreen is only supported ' + | ||
| 'when the iframe is visible and after user interaction.'; | ||
|
|
||
| export const messageType = { |
| const iframeWindow = this.ui_.contentWindow; | ||
| if (iframeWindow) { | ||
| // No sensitive information sent, so safe to use '*' | ||
| iframeWindow./*OK*/ postMessage( |
There was a problem hiding this comment.
Sorry I know we agree to send back the status and the error message, but I realize the format doesn't scale well.
Imagine we want to expand the feature to work with following request.
{
'type': 'consent-response'
'action': 'accept'
'info': '.............'
}
It would be difficult for CMPs to categorize response based on the message info. Instead I think the following items are also required.
- echo back the request type
- echo back the request action
- request status
- errorMessage if there's any
|
Should I use this API to send back responses from the |
|
Let's save it : ) |
* master: Launch `amp-next-page` v2 & clean up experiment (ampproject#27035) ✨Implement Display Locking on amp-accordion (ampproject#25867) 📖<amp-next-page> documentation and validation (ampproject#26841) Improve visibility event tests (ampproject#26847) amp-geo: Fall back to API for country (ampproject#26407) ✨ Add customization options to <amp-story-quiz> (ampproject#26714) navigation: Minor test improvements (ampproject#26926) ♻️ Alias video.fullscreen action for symmetry with interface names (ampproject#27017) ✨ Implements `amp-analytics` support for `amp-next-page` (ampproject#26451) ✨ amp-video-iframe integration: global jwplayer() singleton by default (ampproject#26969) Fix unit tests broken by ampproject#26687 (ampproject#27000) Filter redirect info from emails (ampproject#27012) 📖 Make amp-bind doc valid, fix amp-form stories filter (ampproject#27003) url-replacements: Minor test improvement (ampproject#26930) viewport: Minor test improvement (ampproject#26931) amp-consent fullscreen warning (ampproject#26467) dep-check: USE CAPS FOR IMPORTANCE (ampproject#26993) fix img url (ampproject#26987) # Conflicts: # extensions/amp-next-page/amp-next-page.md
Task 4 of #26432
enter-fullscreenmessage is sent from iframe to parent window