Added fallback support to `amp-twitter` #10281

wassgha · 2017-07-06T17:10:03Z

This adds the support for a fallback element inside amp-twitter that only appears when a tweet can not be loaded or was deleted.

Changes

Twitter iframe now emits a no-content postMessage when a tweet can't be loaded
amp-twitter listens to no-content and toggles the fallback accordingly
amp-twitter no longer shows the placeholder when a tweet can't be loaded (now as soon as the tweet finished retrieving, the placeholder is hidden regardless of whether the tweet was found or not)

Closes #9355 , Related-to https://stackoverflow.com/questions/43975240/amp-twitter-fallback-when-tweet-deleted

cramforce · 2017-07-06T17:41:35Z

examples/twitter.amp.html

@@ -20,30 +20,45 @@
      layout="responsive"
      data-tweetid="638793490521001985">
      <blockquote placeholder class="twitter-tweet" data-lang="en"><p lang="en" dir="ltr">I only needed to change some CSS. <a href="http://t.co/LvjLbjgY9F">pic.twitter.com/LvjLbjgY9F</a></p>&mdash; Malte Ubl (@cramforce) <a href="https://twitter.com/cramforce/status/638793490521001985">September 1, 2015</a></blockquote>
+      <div fallback>
+        Something happened..


Please provide a better fallback text that makes clear what this is for.

I think there is some confusion: Typically the placeholder is the right fallback. A very different fallback will be a super special case.

What I understood from the discussion in #9355 is that the requested feature is a fallback that only appears when there's a problem (while the current behavior using a placeholder makes the error message appear while the element is loading and persist if the error is detected or disappear if the error wasn't detected). Let's discuss this offline whenever you have time.

Yeah, I understand the request, but I assume that the vast majority of publishers will want the current behavior. We should not change the behavior when no fallback is provided: Placeholder is shown when no tweet can be loaded.

We could do it in stages?

Show placeholder till load

If load success, hide placeholder and display twitter

Else if load failure,

If fallback, display fallback

Else if placeholder, display placeholder

That works :) Although not what placeholder is intended to be.. Maybe we should make a warning in the validator for publishers to switch to the correct behavior if they're not using placeholder correctly.

Archiving deleted tweets this way is actually important in the journalism space. Just because PoliticianX deletes their tweet should not mean that the article about it should no longer show the tweet content.

They would be able to preserve that behavior if the text of the tweet was put inside both the fallback and the placeholder, no?

We just don't want to change the behavior. As long as without a fallback the placeholder is still shown, that is fine. Definitely don't want to duplicate the markup.

Agree with this. I tested with amp-img and it tries to do the same flow as described here but is somewhat broken: created #10321

cramforce · 2017-07-06T17:42:06Z

examples/twitter.amp.html

+      layout="responsive"
+      data-tweetid="882818033403789316"
+      data-cards="hidden">
+      <blockquote placeholder class="twitter-tweet" data-lang="en"><p lang="en" dir="ltr">AMP test</p>&mdash; Gharbi Wassim (@wassgha) <a href="https://twitter.com/wassgha/status/882818033403789316">July 6, 2017</a></blockquote>


Can placeholder go on the blockquote.

Lets make two examples here:

deleted tweet (unchanged)

deleted tweet with fallback

cramforce · 2017-07-06T17:42:20Z

extensions/amp-twitter/0.1/amp-twitter.js

      this./*OK*/changeHeight(data['height']);
    }, /* opt_is3P */true);
+    listenFor(iframe,'no-content', () => {


Nit: space after ,

cramforce · 2017-07-06T17:43:28Z

extensions/amp-twitter/0.1/amp-twitter.js

@@ -54,7 +54,8 @@ class AmpTwitter extends AMP.BaseElement {

  /** @override */
  firstLayoutCompleted() {
-    // Do not hide placeholder
+    // Hide the placeholder
+    this.togglePlaceholder(false);


This is definitely wrong. Most will want to show the placeholder on deletion.

jridgewell · 2017-07-06T19:48:50Z

examples/twitter.amp.html

@@ -20,30 +20,45 @@
      layout="responsive"
      data-tweetid="638793490521001985">
      <blockquote placeholder class="twitter-tweet" data-lang="en"><p lang="en" dir="ltr">I only needed to change some CSS. <a href="http://t.co/LvjLbjgY9F">pic.twitter.com/LvjLbjgY9F</a></p>&mdash; Malte Ubl (@cramforce) <a href="https://twitter.com/cramforce/status/638793490521001985">September 1, 2015</a></blockquote>
+      <div fallback>
+        Something happened..


We could do it in stages?

Show placeholder till load

If load success, hide placeholder and display twitter

Else if load failure,

If fallback, display fallback

Else if placeholder, display placeholder

jridgewell · 2017-07-06T19:49:17Z

extensions/amp-twitter/0.1/amp-twitter.js

@@ -54,7 +54,8 @@ class AmpTwitter extends AMP.BaseElement {

  /** @override */
  firstLayoutCompleted() {
-    // Do not hide placeholder
+    // Hide the placeholder
+    this.togglePlaceholder(false);


We'd still want to wait here.

aghassemi · 2017-07-07T23:38:57Z

examples/twitter.amp.html

@@ -20,30 +20,45 @@
      layout="responsive"
      data-tweetid="638793490521001985">
      <blockquote placeholder class="twitter-tweet" data-lang="en"><p lang="en" dir="ltr">I only needed to change some CSS. <a href="http://t.co/LvjLbjgY9F">pic.twitter.com/LvjLbjgY9F</a></p>&mdash; Malte Ubl (@cramforce) <a href="https://twitter.com/cramforce/status/638793490521001985">September 1, 2015</a></blockquote>
+      <div fallback>


let's just add the fallback for a single example ( the new one you added )

aghassemi · 2017-07-07T23:40:44Z

examples/twitter.amp.html

+      data-cards="hidden">
+      <blockquote placeholder class="twitter-tweet" data-lang="en"><p lang="en" dir="ltr">AMP test</p>&mdash; Gharbi Wassim (@wassgha) <a href="https://twitter.com/wassgha/status/882818033403789316">July 6, 2017</a></blockquote>
+      <div fallback>
+        Something happened..


Tweet has been deleted and is no longer available.

wassgha · 2017-07-10T00:37:33Z

Implemented the following agreed on behavior:

Show placeholder till load
If load success, hide placeholder and display twitter
Else if load failure,
1. If fallback, display fallback
2. Else if placeholder, keep placeholder
  In simple terms:
  If only placeholder is provided then use it as both a placeholder and a fallback. If both placeholder and fallback are provided, display the placeholder until load (regardless of whether there's an error or not) and use the fallback to display errors.

I have two questions about this:
1 . Is it ok that if both placeholder and fallback are provided and an error occurs, the placeholder will now be hidden?
2. Since when an error happens we don't get the resize message, the iframe's height is not set which leaves a lot of empty space when there's only a placeholder / fallback. Not sure what to set the height to since the placeholder could contain a tweet and it might actually need the space.
@cramforce

cramforce

LGTM with two small comments.

cramforce · 2017-07-10T13:47:15Z

examples/twitter.amp.html

+      layout="responsive"
+      data-tweetid="882818033403789316"
+      data-cards="hidden">
+      <blockquote placeholder class="twitter-tweet" data-lang="en"><p lang="en" dir="ltr">AMP test</p>&mdash; Gharbi Wassim (@wassgha) <a href="https://twitter.com/wassgha/status/882818033403789316">July 6, 2017</a></blockquote>


Lets make two examples here:

deleted tweet (unchanged)

deleted tweet with fallback

cramforce · 2017-07-10T13:47:59Z

extensions/amp-twitter/0.1/amp-twitter.js

@@ -54,7 +54,7 @@ class AmpTwitter extends AMP.BaseElement {

  /** @override */
  firstLayoutCompleted() {
-    // Do not hide placeholder
+    // Do not hide the placeholder


nit: Please also add a period at the end of the sentence :)

jridgewell · 2017-07-12T18:10:11Z

extensions/amp-twitter/0.1/amp-twitter.js

      this.togglePlaceholder(false);
      this./*OK*/changeHeight(data['height']);
    }, /* opt_is3P */true);
+    listenFor(iframe, 'no-content', () => {
+      if (this.getFallback()) {


Nit: We could check this outside the listener, to avoid creating the listener when there's no fallback:

if (this.getFallback()) { listenFor(iframe, 'no-content', () => {}); }

googlebot added the cla: yes label Jul 6, 2017

wassgha requested review from cramforce and aghassemi July 6, 2017 17:10

cramforce requested changes Jul 6, 2017

View reviewed changes

jridgewell reviewed Jul 6, 2017

View reviewed changes

aghassemi suggested changes Jul 7, 2017

View reviewed changes

wassgha force-pushed the twitter-fallback branch 2 times, most recently from bed3b26 to dda268b Compare July 10, 2017 00:40

wassgha added this to the Fixit Week EOQ2 milestone Jul 10, 2017

cramforce approved these changes Jul 10, 2017

View reviewed changes

wassgha force-pushed the twitter-fallback branch from dda268b to 2fef90d Compare July 10, 2017 18:09

Added fallback support to amp-twitter

5c4f34d

wassgha force-pushed the twitter-fallback branch from 2fef90d to 5c4f34d Compare July 10, 2017 21:35

aghassemi approved these changes Jul 12, 2017

View reviewed changes

aghassemi merged commit b87b71e into ampproject:master Jul 12, 2017

jridgewell reviewed Jul 12, 2017

View reviewed changes

wassgha deleted the twitter-fallback branch July 13, 2017 01:24

aghassemi mentioned this pull request Jul 14, 2017

amp-list: Support placeholder and fallback #10426

Merged

dreamofabear mentioned this pull request Jul 14, 2017

Specify placeholder/fallback interaction ampproject/amp.dev#618

Closed

ampprojectbot added the PR use: In Beta / Experimental label Jul 17, 2017

ampprojectbot added the PR use: In Stable label Jul 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added fallback support to `amp-twitter` #10281

Added fallback support to `amp-twitter` #10281

wassgha commented Jul 6, 2017

cramforce Jul 6, 2017

cramforce Jul 6, 2017

wassgha Jul 6, 2017

cramforce Jul 6, 2017

jridgewell Jul 6, 2017

wassgha Jul 6, 2017

cramforce Jul 6, 2017

wassgha Jul 6, 2017

cramforce Jul 6, 2017 •

edited

aghassemi Jul 7, 2017

cramforce Jul 6, 2017

cramforce Jul 10, 2017

cramforce Jul 6, 2017

cramforce Jul 6, 2017

jridgewell Jul 6, 2017

jridgewell Jul 6, 2017

aghassemi Jul 7, 2017

aghassemi Jul 7, 2017

wassgha commented Jul 10, 2017 •

edited

cramforce left a comment

cramforce Jul 10, 2017

cramforce Jul 10, 2017

jridgewell Jul 12, 2017

Added fallback support to amp-twitter #10281

Added fallback support to amp-twitter #10281

Conversation

wassgha commented Jul 6, 2017

Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cramforce Jul 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wassgha commented Jul 10, 2017 • edited

cramforce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Added fallback support to `amp-twitter` #10281

Added fallback support to `amp-twitter` #10281

cramforce Jul 6, 2017 •

edited

wassgha commented Jul 10, 2017 •

edited