List AMP support for all native WordPress widgets #864

kienstra · 2018-01-16T00:03:43Z

Acceptance criteria:
AC1: List the AMP support for all native WordPress widgets, as produced by the script in #839.

This from AC5 in #839:
AC5: One story will be created as an outcome of the Discovery to address enhanced/added support for sidebar widgets.

When AMP doesn't support a widget, either describe it in detail here, or open a sub-issue.

There doesn't seem to be an issue for this yet.

The text was updated successfully, but these errors were encountered:

kienstra · 2018-01-16T02:15:27Z

Widget AMP Support

Update: this list is now updated, and located in the project wiki

Widget	Status	Note
Archives without 'dropdown'	✅
Archives with 'dropdown'	❌	AMP error from the script on the `onchange` attribute
Audio	❌	Has AMP errors
Calendar	✅
Categories without 'dropdown'	✅
Categories with 'dropdown'	❌	2 AMP errors
Custom HTML	✅
Gallery	❌	Displays, but has AMP errors
Image	❌	Displays, but has AMP errors
Meta	✅
Nav Menu	✅
Pages	✅
Recent Comments	❌	`WP_Widget_Recent_Comments::recent_comments_style()` outputs illegal `style` element in `head`.
RSS	❌	Echoes illegal `<img>`
Video	❌	2 AMP errors

AMP Errors
Archives Widget with "dropdown"

The attribute 'onchange' may not appear in tag 'select':
<select id="archives-dropdown-106" name="archive-dropdown" onchange='document.location.href=this.options[this.selectedIndex].value;'>
There's no filter for the markup in widget(), so we might subclass this, as @westonruter suggested.

Audio Widget

Only AMP runtime 'script' tags are allowed, and only in the document head.
This conditionally outputs inline styling.
Once the plugin's sanitizization is decoupled from the post content, we might use it to filter wp_audio_shortcode.
It also enqueues styling and a script.

Categories Widget with "dropdown"

The tag 'FORM [method=GET]' requires including the 'amp-form' extension JavaScript.
Only AMP runtime 'script' tags are allowed, and only in the document head. There's an inline script below the <form>:
<script type='text/javascript'>
This widget() method also doesn't have a filter. So we might subclass and sanitize it, to prevent outputting the inline <script>. We'll also have to use the amp-form extension.

Gallery Widget

The tag 'img' may only appear as a descendant of tag 'noscript'. Did you mean 'amp-img'?
If the theme doesn't support HTML5 galleries, this outputs an inline <style>.
This widget outputs the <img> elements by calling wp_get_attachment_link() or wp_get_attachment_image. Only wp_get_attachment_image_link() has a filter that we could use.

Image Widget

The inline 'style' attribute is not allowed in AMP documents. Use 'style amp-custom' tag instead.
The tag 'img' may only appear as a descendant of tag 'noscript'. Did you mean 'amp-img'?
There doesn't look to be a way to filter the widget output.

Recent Comments Widget

As Weston mentioned, WP_Widget_Recent_Comments::recent_comments_style() renders a disallowed style element in head.

RSS Widget

The tag 'img' may only appear as a descendant of tag 'noscript'. Did you mean 'amp-img'?

Video Widget

The inline 'style' attribute is not allowed in AMP documents. Use 'style amp-custom' tag instead
This usually enqueues 2 scripts and a style, which aren't allowed by AMP.
The tag 'video' may only appear as a descendant of tag 'noscript'. Did you mean 'amp-video'?
There's a way to filter the output, but only if it's an attachment, a YouTube video, or a Vimeo video. Subclassing this might be a good strategy.

@todo

The build will fail, as the filtering in widget() isn't present. There isn't yet a way to instantiate or init() class AMP_Widgets(). But it unregisters the 'Archives' and 'Categories' widgets. And registers new widgets that subclass them. @todo: filter the output of widget() in these subclasses. And add support for the remaining widgets.

As Weston mentioned, the plugin already prevents scripts in: 'wp_print_footer_scripts'. Also, move the comment filter to a subclass of WP_Widget_Recent_Comments. Includes PHPUnit tests for all of these changes.

@todo

Only implement the render_media() function. @todo: filter the markup in it. This includes a PHPUnit test class for it, which will now fail.

@todo

Like the 'Gallery' widget, only implement the render_media(). @todo: filter markup. The test currently fails because of this.

A PHPUnit test fails, as it's not yet sanitized. It needs to have an <amp-video>, and remove the 'style' attribute.

Use AMP_Theme_Support::filter_the_content(). Still, I need to ensure that this is the best way to filter. Add RSS and Audio widget subclasses. And PHPUnit classes for these.

@westonruter

Props @westonruter for the details of how to do this. Mainly copy WP_Widget_Categories::widget() into the subclass widget(). Add an 'id' attribute to the <form>. And use the 'id' in the dropdown <select>. This dropdown now redirects to the category pages, as expected. Also, bootstrap the widget support class.

There was an error from the textdomain not being included. But it's copied from the WordPress widget, So it should use the default textdomain. Use @codingStandardsIgnoreLine in those cases. Also, call wp_maybe_load_widgets() in the test setUp(). This ensures that the core widgets are loaded.

…mains. In phpcs.xml, add 'default' And remove the @codingStandardsIgnoreLine tags.

On Weston's suggestion, simply point to the full documentation. Also, remove widgets from the function if they don't exist. As Weston mentioned, this applies especially to the media-* widgets.

The diff looks much bigger than it is. No change to the classes, they're just wrapped in a conditional. For WP version less than 4.9, The media widgets won't exist. Prevents an error in declaring the AMP widgets that extend them. This is the least inelegant solution I saw. Another option is conditionally loading the files in: widgets/class-amp-widgets.php.

Media widgets won't be declared on WP 4.8 and earlier. So don't test them. Simply mark them as skipped.

I had tried to wrap the action hook in this conditional: add_action( 'init', 'amp_add_widget_support' ) But this produced this notice: is_amp_endpoint was called <strong>incorrectly</strong>. is_amp_endpoint() was called before the 'parse_query' hook was called.

The markup is filtered with AMP_Theme_Support::filter_the_content(). Still, I need to apply the dropdown to the 'Archives' widget.

@westonruter

Props @westonruter for describing how to do this. Like before, it mainly copies WP_Widget_Archives::widget(). It adds an id to the <form>. And an 'on' attribute to the <select> element.

@todo

Before, I had registered this too late. Also, move is_amp_endpoint() conditional to widget(). This isn't ideal. But that function isn't available before 'parse_query.' And that runs after 'widgets_init.' Also, is seems that all but 2 of these subclass widgets will be removed. @todo: Look at a regression, where the 'amp-form' extension isn't included.

The widgets now exit from their methods is is_amp_endpoint(). So set this to true, with amp_theme_support( 'amp' ).

Before, this wasn't included via the sanitizer. The ideal solution would probably involve editing the sanitizer. But this adds a filter to add the 'amp-form.' And it removes the AMP sanitization of 'Categories' and 'Archives' widgets.

There were 2 files with conflicts, Retain both of the edits. They were merely from adding 2 different functions in the same place.

@todo

That function has been removed in PR #888. It looks like that uses full-page buffering. So remove the widget subclasses that use that. @todo: look at regressions in the 'Archives' and 'Categories' dropdowns.

westonruter · 2018-01-27T01:26:27Z

@kienstra these are all supported now via #870, right?

kienstra · 2018-01-29T23:46:14Z

All Widgets Supported, But Gallery Widget Might Benefit From amp-carousel

Hi @westonruter,
Sorry for the delay here. All of the widgets above are now supported, without any AMP error.

But the 'Gallery' widget might be able to use an <amp-carousel>. It looks plain now:

AMP_Gallery_Embed looks to convert 'gallery' shortcodes into <amp-carousel> elements. I'll look at whether it could help with this.

There's an existing handler to create 'amp-carousel' elements: class AMP_Gallery_Embed_Handler. So override the 'Gallery' widget class. And use that in render_media(). Otherwise, that function is copied from the parent. The parent calls gallery_shortcode() at the end of render_media(). And that function doesn't have a filter for the markup.

Before, there was a workaround to ensure these didn't overflow. I added style="max-width:1005" This removes that workaround, And instead sets the layout to 'responsive.' Also, this updates the PHPUnit tests.

I had copied this line into the block above. But it's not needed there.

On Paul Baukus and Weston Ruter's suggestion. The sizes attribute isn't relevant to this WP plugin, as Paul Baukus mentioned.

This could lead to unexpected results. If the intended width or height is less than the container, This will increase it to fill the container. This fixed the issue of overflowing <ampiframe> in widgets. But it could create an issue in other places.

This was present earlier. Weston added this, and it describes where it will appear.

An earlier commit only prevented adding it. This removes the attribute it it's present.

Iframes from WordPress embeds usually have width and height. In AMP, the inferred value layout for this is fixed. This means that even if you apply max-width:100%, The height won't adjust to the new ratio. Setting the 'layout' to 'responsive' seems to be the only way to ensure the same aspect ratio. Of course, this has a risk that iframes will expand to fill their container, where they didn't before.

kienstra · 2018-02-21T06:09:32Z

Request For QA

Hi @csossi,
Could you please test this ticket? The widgets look to appear and work as expected in my local environment.

The test site might not be the best place for this, as it'll distort the pages when we place 20-30 test widgets in the sidebar.

We should run the widget test script on a site, as it will insert all of the needed widgets. As long as the site already has 3 videos and 3 audio files.

kienstra · 2018-02-26T19:13:49Z

Test Page

Hi @csossi,
Could you please test the widgets on this test page?

Please verify these widgets:

Audio
Archives (without dropdown)
Archives (with dropdown)
Calendar
Categories (without dropdown)
Categories (with dropdown)
Custom HTML
Gallery
Image
Menu
Pages
Recent Posts
RSS
Tag Cloud (without count)
Tag Cloud (with count)
Text
Video

csossi · 2018-02-27T17:16:00Z

verified in QA

ThierryA · 2018-03-01T13:17:28Z

@kienstra could kindly update the list of widget supported now that the work is done. It would be great to have it in the WIKI and then change all instances of the tables to link to the wiki to avoid confusion.

PS: this should apply to embeds too

kienstra · 2018-03-01T18:41:20Z

Added Page To Wiki

Hi @ThierryA,
This new page in the project wiki has the updated matrices for widget and embed support. Also, I made a comment in the existing matrices, referencing that wiki page.

In the image preprocessor, Convert a 'data-amp-layout' attribute to 'layout.' As Weston mentioned, this won't change the styling on non-AMP pages, As the preprocessor won't modify them.

Add this attribute to the allowed list for <img>. The sanitizer now converts this to 'layout.'

@todo

Per feedback, this should apply to all contexts, As long as they allow <img> with a width and height. Uses Weston's snippet in the filter callback. @todo: move it to AMP_Theme_Support.

Also, remove $context_type parameter. It's no longer used. Otherwise, there's no change to the function.

ThierryA · 2018-03-05T10:43:06Z

Awesome, thanks @kienstra

kienstra added this to the v0.7 milestone Jan 16, 2018

kienstra self-assigned this Jan 16, 2018

westonruter mentioned this issue Jan 16, 2018

Add styles for Recent Comments widget xwp/ampnews#28

Closed

kienstra mentioned this issue Jan 17, 2018

Issue #864: Native WordPress widget support, including subclasses #870

Merged

3 tasks

kienstra pushed a commit that referenced this issue Jan 18, 2018

Issue #864: Subclass 'Gallery' widget, to output valid AMP markup.

436722d

Only implement the render_media() function. @todo: filter the markup in it. This includes a PHPUnit test class for it, which will now fail.

kienstra pushed a commit that referenced this issue Jan 18, 2018

Issue #864: Subclass 'Image' widget to output valid AMP markup.

44287d8

Like the 'Gallery' widget, only implement the render_media(). @todo: filter markup. The test currently fails because of this.

kienstra pushed a commit that referenced this issue Jan 18, 2018

Issue #864: Subclass the 'Video' widget, in order to sanitize markup.

c7268bf

A PHPUnit test fails, as it's not yet sanitized. It needs to have an <amp-video>, and remove the 'style' attribute.

kienstra pushed a commit that referenced this issue Jan 20, 2018

Issue #864: Address failed Travis build by adding 'default' to textdo…

6ee75ee

…mains. In phpcs.xml, add 'default' And remove the @codingStandardsIgnoreLine tags.

kienstra pushed a commit that referenced this issue Jan 20, 2018

Issue #864: Skip tests if the classes aren't declared.

99dd862

Media widgets won't be declared on WP 4.8 and earlier. So don't test them. Simply mark them as skipped.

kienstra pushed a commit that referenced this issue Jan 23, 2018

Issue #864: Remove @todo tags from widgets, as they're implemented.

28b5cdd

The markup is filtered with AMP_Theme_Support::filter_the_content(). Still, I need to apply the dropdown to the 'Archives' widget.

kienstra pushed a commit that referenced this issue Jan 23, 2018

Issue #864: Fix PHPUnit tests, accomodating is_amp_endpoint().

e0456e1

The widgets now exit from their methods is is_amp_endpoint(). So set this to true, with amp_theme_support( 'amp' ).

kienstra pushed a commit that referenced this issue Jan 23, 2018

Issue #864: Merge in develop, resolve conflicts.

62a924f

There were 2 files with conflicts, Retain both of the edits. They were merely from adding 2 different functions in the same place.

kienstra pushed a commit that referenced this issue Feb 6, 2018

Issue #864: Remove extra line in set_layout().

936f302

I had copied this line into the block above. But it's not needed there.

kienstra pushed a commit that referenced this issue Feb 13, 2018

Issue #864: Remove the 'sizes' attribute from <amp-img>.

6848541

On Paul Baukus and Weston Ruter's suggestion. The sizes attribute isn't relevant to this WP plugin, as Paul Baukus mentioned.

kienstra pushed a commit that referenced this issue Feb 14, 2018

Issue #864: Add comment after addition of 'style.'

f95c0be

This was present earlier. Weston added this, and it describes where it will appear.

kienstra pushed a commit that referenced this issue Feb 15, 2018

Issue #864: Remove <amp-img> sizes attribute if present.

6bd4326

An earlier commit only prevented adding it. This removes the attribute it it's present.

kienstra assigned csossi Feb 21, 2018

kienstra reopened this Feb 21, 2018

kienstra removed their assignment Feb 21, 2018

kienstra assigned kienstra and csossi and unassigned csossi and kienstra Feb 26, 2018

csossi removed their assignment Feb 27, 2018

kienstra pushed a commit that referenced this issue Mar 1, 2018

Issue #864: Allow 'data-amp-layout' in wp_kses() for 'post.'

037f6ef

Add this attribute to the allowed list for <img>. The sanitizer now converts this to 'layout.'

kienstra pushed a commit that referenced this issue Mar 1, 2018

Issue #864: Move add_layout() to AMP_Theme_Support.

011e478

Also, remove $context_type parameter. It's no longer used. Otherwise, there's no change to the function.

ThierryA closed this as completed Mar 8, 2018

ThierryA added the Sprint 5 label Mar 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

List AMP support for all native WordPress widgets #864

List AMP support for all native WordPress widgets #864

kienstra commented Jan 16, 2018 •

edited

Loading

kienstra commented Jan 16, 2018 •

edited

Loading

westonruter commented Jan 27, 2018

kienstra commented Jan 29, 2018

kienstra commented Feb 21, 2018 •

edited

Loading

kienstra commented Feb 26, 2018

csossi commented Feb 27, 2018

ThierryA commented Mar 1, 2018

kienstra commented Mar 1, 2018

ThierryA commented Mar 5, 2018

List AMP support for all native WordPress widgets #864

List AMP support for all native WordPress widgets #864

Comments

kienstra commented Jan 16, 2018 • edited Loading

kienstra commented Jan 16, 2018 • edited Loading

Update: this list is now updated, and located in the project wiki

westonruter commented Jan 27, 2018

kienstra commented Jan 29, 2018

kienstra commented Feb 21, 2018 • edited Loading

kienstra commented Feb 26, 2018

csossi commented Feb 27, 2018

ThierryA commented Mar 1, 2018

kienstra commented Mar 1, 2018

ThierryA commented Mar 5, 2018

kienstra commented Jan 16, 2018 •

edited

Loading

kienstra commented Jan 16, 2018 •

edited

Loading

kienstra commented Feb 21, 2018 •

edited

Loading