Remove registration of Facebook embed handler #4384
Conversation
| $parent_node->removeChild( $script ); | ||
|
|
||
| // Remove parent node if it is an empty <p> tag. | ||
| if ( 'p' === $parent_node->tagName && null === $parent_node->firstChild ) { |
There was a problem hiding this comment.
The script tag is wrapped with a p tag (from wpautop()), so I've removed the lingering empty tag if it's there.
| $parent_node->removeChild( $script ); | ||
|
|
||
| // Remove parent node if it is an empty <p> tag. | ||
| if ( 'p' === $parent_node->tagName && null === $parent_node->firstChild ) { |
There was a problem hiding this comment.
| if ( 'p' === $parent_node->tagName && null === $parent_node->firstChild ) { | |
| if ( 'p' === $parent_node->nodeName && null === $parent_node->firstChild ) { |
There was a problem hiding this comment.
After looking at some of the other embeds, they are for the most part also wrapped with a <p> tag. That isn't an issue per se, but if this PR is merged we should probably standardize the other embeds to not be wrapped with a paragraph element.
Co-Authored-By: Weston Ruter <[email protected]>
|
Humm, there are phpunit test errors. |
| // Remove parent node if it is an empty <p> tag. | ||
| if ( 'p' === $parent_node->nodeName && null === $parent_node->firstChild ) { | ||
| $parent_node->parentNode->removeChild( $parent_node ); | ||
| } |
There was a problem hiding this comment.
I don't see a test for this.
| // Remove parent node if it is an empty <p> tag. | ||
| if ( 'p' === $parent_node->nodeName && null === $parent_node->firstChild ) { | ||
| $parent_node->parentNode->removeChild( $parent_node ); | ||
| } | ||
| } | ||
| $fb_root->parentNode->removeChild( $fb_root ); |
There was a problem hiding this comment.
It turns out that when you have multiple Facebook embeds on a page, each instance will include a div#fb-root. So I suppose this should be changed to:
| $fb_root->parentNode->removeChild( $fb_root ); | |
| while ( $fb_root ) { | |
| $fb_root->parentNode->removeChild( $fb_root ); | |
| $fb_root = $dom->getElementById( 'fb-root' ); | |
| } |
There was a problem hiding this comment.
In theory this should work, but when it retrieves the #fb_root div again it's returning the same div as before. I'll adapt this to use an XPath query instead.
There was a problem hiding this comment.
Why would it return the same div as before if it was removed from the document?
|
Note: It appears Facebook tests don't have mocks for the non- |
Summary
This PR removes the custom WordPress embed handler for Facebook and instead relies on
\AMP_Facebook_Embed_Handler::sanitize_raw_embedsto convert such embeds to their appropriate AMP components.Fixes #4358.
Checklist