-
Notifications
You must be signed in to change notification settings - Fork 384
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Unwrap all embeds with paragraph tags #4650
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay in reviewing!
private function sanitize_raw_embed( DOMElement $iframe_node ) { | ||
$next_sibling = $iframe_node->nextSibling; | ||
|
||
if ( $next_sibling && 'script' === $next_sibling->nodeName ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this verify the script is the expected one? Namely, one with the src
of https://videopress.com/videopress-iframe.js
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Come to mention it, what does this script do? It appears to be handling resizing. Is resizing something that we're missing? Or is resizing not needed in an AMP context? If it is, we should contact WordPress.com about emitting the AMP resizing messages.
'width' => $this->args['width'], | ||
'height' => $this->args['height'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note the width/height are overridden below if defined on the iframe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before you going, I suggest refactoring the code to put more recurring logic into the base embed handler.
Basically, what this does is provide more of "Template Method Pattern" approach to dealing with embeds, where the base embed handler lays down a framework of how to deal with a typical embed. The individual embeds can still override any step that doesn't directly fit within the general framework, but overall, there'll be less code, needing less testing, as a lot of them will be perfectly happy with this default framework if it is well chosen.
So, if you look at my comments, I'd suggest using sanitize_raw_embeds()
as one of the steps in this template, with its default implementation (using is_raw_embed()
and sanitize_raw_embed()
, but minus the hard-coded strings) just being implemented once within the base embed handler.
@@ -190,6 +117,22 @@ private function create_amp_instagram_and_replace_node( $dom, $node ) { | |||
$this->did_convert_elements = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As uncovered in https://github.com/rtCamp/themes/pull/3/files#r423249922, it may make sense in this create_amp_instagram_and_replace_node()
method to preserve some inline styles from the $node
to copy onto $new_node
. In particular, border-radius: 3px; border: 1px solid rgb(219, 219, 219);
.
Nevertheless, this is is probably not something we should be concerned with. In fact, it's probably actually an AMP issue where amp-instagram
should be applying those styles. So nevermind!
# Conflicts: # tests/php/test-amp-dailymotion-embed-handler.php # tests/php/test-amp-instagram-embed-handler.php # tests/php/test-amp-pinterest-embed-handler.php # tests/php/test-amp-scribd-embed-handler.php # tests/php/test-amp-soundcloud-embed-handler.php # tests/php/test-amp-vimeo-embed-handler.php # tests/php/test-amp-vine-embed-handler.php # tests/php/test-class-amp-gfycat-embed-handler.php # tests/php/test-class-amp-hulu-embed-handler.php # tests/php/test-class-amp-imgur-embed-handler.php # tests/php/test-class-amp-youtube-embed-handler.php
# Conflicts: # tests/php/test-amp-dailymotion-embed-handler.php # tests/php/test-amp-instagram-embed-handler.php # tests/php/test-class-amp-imgur-embed-handler.php
Marking this as ready for review although one of the tests is failing. |
Plugin builds for 0860f48 are ready 🛎️!
|
…wrapped-embeds * 'develop' of github.com:ampproject/amp-wp: Fix PHPCS issues Use SVG variant of `yes-alt` Dashicon Disable filtering of services by default Add additional tests for ServiceBasedPlugin Add test to verify that filtering is disabled by default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to see much of the duplicated code being factored away.
There are a couple areas that need to be addressed before this is ready:
It is relatively common for developers to create their own embed handlers, slightly less common I'd say to creating custom sanitizers. Sanitizers and embed handlers are two key ways the plugin is extended. Therefore, it is critical that backwards-compatibility be maintained for existing embed handlers (and sanitizers).
Additionally, it is important that any APIs introduced on the base method be ergonomic for developers to use when they extend the embed base handler. For example, some of the methods take a lot of positional parameters which make it hard to read.
@@ -1179,8 +1180,9 @@ public static function register_content_embed_handlers() { | |||
); | |||
continue; | |||
} | |||
|
|||
$embed_handler->register_embed(); | |||
if ( $embed_handler instanceof Registerable ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this have a backwards-incompatibility problem? If someone has existing subclasses of AMP_Base_Embed_Handler
with register_embed
defined, should it not do something like this:
if ( $embed_handler instanceof Registerable ) { | |
if ( $embed_handler instanceof Registerable || method_exists( $embed_handler, 'register_embed' ) ) { |
@@ -40,14 +42,18 @@ abstract class AMP_Base_Embed_Handler { | |||
protected $did_convert_elements = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This property can be marked as @deprecated
and any usages in the subclasses can be eliminated (all of which are write-only currently), as it is not serving any purpose anymore.
In the past it was used to determine whether or not the AMP component script needed to be added to the page. But this is obsolete.
*/ | ||
abstract public function unregister_embed(); | ||
protected $base_embed_url = ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see this property as being used anywhere. Can it be removed?
* @return DOMNodeList|null A list of DOMElement nodes, or null if not implemented. | ||
*/ | ||
abstract protected function get_raw_embed_nodes( Document $dom ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If not implemented? Doesn't an abstract
method always have to be implemented by a subclass? Shouldn't this method just be changed to be non-abstract
and then return null
here? Then only the subclassed methods would return DOMNodeList
. This would avoid needing to define the method to return null
as it stands today for Twitter, Playlist, Gallery, and Blocks.
/** | ||
* Get all raw embeds from the DOM. | ||
* | ||
* @param Document $dom Document. | ||
* @return DOMNodeList|null A list of DOMElement nodes, or null if not implemented. | ||
*/ | ||
protected function get_raw_embed_nodes( Document $dom ) { // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable | ||
return null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just making this the method in the base class.
protected function get_raw_embed_nodes( Document $dom ) { // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable | ||
return null; | ||
} | ||
|
||
/** | ||
* Make embed AMP compatible. | ||
* | ||
* @param DOMElement $node DOM element. | ||
*/ | ||
protected function sanitize_raw_embed( DOMElement $node ) { // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments in base class about moving these methods there.
|
||
if ( empty( $args['url'] ) ) { | ||
return ''; | ||
private function get_first_child_element( DOMElement $node ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems useful to be in the base class as a protected
method. It could be called get_first_element_child()
to reflect the DOM: https://developer.mozilla.org/en-US/docs/Web/API/ParentNode/firstElementChild
] | ||
); | ||
|
||
$overflow_node->textContent = esc_html__( 'See more', 'amp' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to escape when manipulating the DOM:
$overflow_node->textContent = esc_html__( 'See more', 'amp' ); | |
$overflow_node->textContent = __( 'See more', 'amp' ); |
'overflow' => '', | ||
'tabindex' => 0, | ||
'role' => 'button', | ||
'aria-label' => esc_attr__( 'See more', 'amp' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to escape when manipulating the DOM:
'aria-label' => esc_attr__( 'See more', 'amp' ), | |
'aria-label' => __( 'See more', 'amp' ), |
$amp_node->appendChild( $overflow_node ); | ||
|
||
// Append the original link as a placeholder node. | ||
if ( $node->firstChild instanceof DOMElement && 'a' === $node->firstChild->nodeName ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this could make use of get_first_element_child
mentioned above.
As discussed over chat, let's pause work on this until after beta1 or else 1.7 worst case. This will allow us to focus on the RME issues. |
Closing in favor of #7564. |
Summary
There are inconsistencies with some embeds being wrapped with a
<p>
tag, and some not. We should strive towards having a uniform markup across all embeds.Fixes #4450.
Fixes #4697.
Fixes #4754.
Fixes #4757.
Embeds to be unwrapped:
Checklist