Embed fallbacks #493
Embed fallbacks #493
Conversation
Both the audio and video sanitizers use the same routine to iterate through the source elements within the main node and filter/sanitize their attributes. However, the filtered attributes are not added back to the source element before it is appended to the new amp-audio or amp-video element. This is problematic when the attribute filtering method converts URLs from http to https, but the converted URL does not make it back into the source element. This commit addresses the issue by updating each source element's attributes before it is appended.
If an audio or video element, or its source child element, has a src URL, but it is invalid (eg it uses the http protocol instead of https), this creates a fallback blockquote element to replace the original that explains that the media could not be loaded.
|
So far I've added fallbacks for audio and video. |
Creates a fallback blockquote element to replace an iframe that doesn't have a valid src attribute.
For a URL with a relative protocol, such as //www.youtube.com/watch?v=dQw4w9WgXcQ ...the assumption is that either http or https will work with it. Since AMP requires https, we should go ahead and add the https scheme to these during filtering, if the sanitizer is configured to require or force the protocol.
|
|
||
| if ( $protocol === $src && ( $https_required || $force_https ) ) { | ||
| // src has relative protocol, ie //example.com/asdf, so add https. | ||
| $src = set_url_scheme( $src, 'https' ); |
There was a problem hiding this comment.
What would happen with relative URLs, like /path/to/file.mp3?
There was a problem hiding this comment.
Good point. Perhaps we could detect a double forward slash // at the beginning of the string to differentiate relative path from relative protocol?
Should we also try to handle relative path?
There was a problem hiding this comment.
Alright, I took a stab at handling relative paths along with the relative protocol scenario: f7dafe5
| $node->parentNode->replaceChild( $fallback_node, $node ); | ||
| } else { | ||
| $node->parentNode->removeChild( $node ); | ||
| } |
There was a problem hiding this comment.
This block of code (and the similar ones across the other sanitizers) could probably be abstracted to a new method?
There was a problem hiding this comment.
Good idea. 2365364
|
|
||
| $fallback_content = $this->dom->createDocumentFragment(); | ||
| $fallback_content->appendXML( sprintf( | ||
| wp_kses( __( 'Could not load <a href="%s">video</a>.', 'amp' ), array( 'a' => array( 'href' => true ) ) ), |
There was a problem hiding this comment.
Should we switch the statement from an error ("Could not load") to more of a action ("Watch video"). It's not entirely accurate to say that we couldn't load the video. (Note: this applies to all the sanitizers)
There was a problem hiding this comment.
We could use "Watch video" and "Listen to audio", but I'm not sure what we'd do for the iframe one...
There was a problem hiding this comment.
Also, I'm not sure "Could not load" is really misleading, since the AMP code is preventing non-https sources from loading.
In the previous commit I removed the embed-specific class name from the abstracted `create_fallback_node` method, but forgot to update the tests
This is a response to #354. Other related issues are #130, #288, #316, and #350.
The goal of this is to provide a consistent user experience when embedded media can't load because it's not from an https source. A few different options were discussed to make this happen, but this is what we settled on:
Here are the types of embeds that should be covered: