search

Lullabot / amp-library

Convert HTML to AMP HTML and report HTML compliance with the AMP HTML specification
Other
381 stars 178 forks source link

Fix for Twitter embeds #271

Closed echosa closed 1 year ago

echosa commented 4 years ago

Fixes #264

As stated in that issue, there is an issue with embedded tweets that quote other tweets. The issue is that there are nested <blockquote> tags, and when the DOMQuery loops through the DOM to parse out the tweets to convert to AMP, it runs into an issue. The inner blockquote is one of the elements that are looped through, but before it is processed the outer blockquote containing it is processed first. During processing, the inner blockquote is removed from DOM when the outer blockquote is removed and replaced with its AMP tag. This results in a null error when the loop gets to the previously removed inner blockquote.

In this PR, I have changed the CSS lookup to only grab and loop through the outer blockquotes. This has been running successfully in production for us for over a week, and I've added tests to the test suite, so I feel confident that this fix is working.

umpirsky commented 4 years ago

@sidkshatriya Any updates on this? We are blocked by this issue.

sidkshatriya commented 4 years ago

Can you please have a look @karens if possible ^^ ?

adc91 commented 3 years ago

This correction is extremely important. Please include the main branch.

m4olivei commented 1 year ago

Closing in favor of https://github.com/Lullabot/amp-library/pull/307. Wanted to add something, couldn't do it on this forked branch.