facebookarchive / facebook-instant-articles-sdk-extensions-in-php

Facebook Instant Articles SDK Extensions in PHP.
https://www.facebook.com/facebookmedia/blog/instant-articles-sdk-extension-now-supports-google-amp-soon-apple-news
Other
45 stars 48 forks source link

Recognition of Interactive elements too restrictive #27

Open rbraun75 opened 7 years ago

rbraun75 commented 7 years ago

The recognition of interactive elements in the instant article is too restrictive.

else if ((Type::is($child, Interactive::getClassName()) || Type::is($child, SocialEmbed::getClassName())) && !Type::isTextEmpty($child->getSource())) {
                    if (!$containsIframe) {
                        $containsIframe = true;
                        $context->getHead()->appendChild($this->buildCustomElementScriptEntry('amp-iframe', 'https://cdn.ampproject.org/v0/amp-iframe-0.1.js', $context));
                    }
                    $childElement = $this->observer->applyFilters('IA_INTERACTIVE', $this->buildIframe($child, $context, 'interactive', true), $child, $context);
                }

Filters with tag "IA_INTERACTIVE" are only applied when the iframe has a "src" attribute, because of && !Type::isTextEmpty($child->getSource()).

Standard Twitter, Facebook, Instagram elements for example are not getting recognized (@see https://developers.facebook.com/docs/instant-articles/reference/embeds)

<figure class="op-interactive">
  <iframe>
    <!-- Include Twitter embed code here. Your embed code should look like <blockquote class="twitter-tweet" ... -->    
  </iframe>
</figure>

Please change the recognition so that iframes without src within "op_interactive" figures are getting recognized too.

timjacobi commented 6 years ago

I agree with this. Care to submit a PR?