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

List AMP support for all native WordPress widgets #864

Closed
kienstra opened this Issue Jan 16, 2018 · 11 comments

Comments

Projects
4 participants
@kienstra
Copy link
Collaborator

kienstra commented Jan 16, 2018

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.

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

@kienstra kienstra self-assigned this Jan 16, 2018

@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Jan 16, 2018

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

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.

@kienstra kienstra added this to In Progress in v0.7 Jan 16, 2018

@ThierryA ThierryA removed this from In Progress in v0.7 Jan 16, 2018

kienstra added a commit that referenced this issue Jan 17, 2018

Issue #864: Dequeue some disallowed scripts, and begin subclasses.
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.

kienstra added a commit that referenced this issue Jan 17, 2018

Issue #864: Remove dequeuing of scripts, and create comment subclass.
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.

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

Issue #864: Subclass 'Gallery' widget, to output valid AMP markup.
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 added a commit that referenced this issue Jan 18, 2018

Issue #864: Subclass 'Image' widget to output valid AMP markup.
Like the 'Gallery' widget, only implement the render_media().
@todo: filter markup.
The test currently fails because of this.

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

Issue #864: Subclass the 'Video' widget, in order to sanitize markup.
A PHPUnit test fails, as it's not yet sanitized.
It needs to have an <amp-video>, and remove the 'style' attribute.

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

Issue #864: Add filtering to widget output, more subclasses.
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.

kienstra added a commit that referenced this issue Jan 19, 2018

Issue #864: Support 'Categories' widget dropdown.
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.

kienstra added a commit that referenced this issue Jan 19, 2018

Issue #864: Address Travis errors, including PHPCS issue.
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.

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

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

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

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

Issue #864: Remove copied filter doc, and remove non-existent widgets.
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.

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

Issue #864: Only declare widget classes if their parents exist.
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.

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

Issue #864: Skip tests if the classes aren't declared.
Media widgets won't be declared on WP 4.8 and earlier.
So don't test them.
Simply mark them as skipped.

kienstra added a commit that referenced this issue Jan 22, 2018

Issue #864: Only add AMP widget support if is_amp_endpoint().
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 &#39;parse_query&#39; hook was called.

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

Issue #864: Remove @todo tags from widgets, as they're implemented.
The markup is filtered with AMP_Theme_Support::filter_the_content().
Still, I need to apply the dropdown to the 'Archives' widget.

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

Issue #864: Make 'Archives' widget dropdown AMP-compliant.
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.

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

Issue #864: Ensure register_widgets() fires on 'widget_init.'
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.

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

Issue #864: Fix PHPUnit tests, accomodating is_amp_endpoint().
The widgets now exit from their methods is is_amp_endpoint().
So set this to true, with amp_theme_support( 'amp' ).

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

Issue #864: Workaround to include amp-form extension JS.
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.

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

Issue #864: Merge in develop, resolve conflicts.
There were 2 files with conflicts,
Retain both of the edits.
They were merely from adding 2 different functions in the same place.

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

Issue #864: Remove widget subclasses that used filter_the_content().
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

This comment has been minimized.

Copy link
Member

westonruter commented Jan 27, 2018

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

@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Jan 29, 2018

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:
gallery-widget

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

kienstra added a commit that referenced this issue Jan 30, 2018

Issue #864: Support <amp-carousel> in 'Gallery' widget.
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.

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

Issue #864: Remove 'sizes' attribute for <amp-iframe> and <amp-video>.
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.

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

Issue #864: Remove extra line in set_layout().
I had copied this line into the block above.
But it's not needed there.

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

Issue #864: Remove the 'sizes' attribute from <amp-img>.
On Paul Baukus and Weston Ruter's suggestion.
The sizes attribute isn't relevant to this WP plugin,
as Paul Baukus mentioned.

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

Issue #864: Remove layout="responsive" from <amp-iframe>.
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.

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

Issue #864: Add comment after addition of 'style.'
This was present earlier.
Weston added this, and it describes where it will appear.

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

Issue #864: Remove <amp-img> sizes attribute if present.
An earlier commit only prevented adding it.
This removes the attribute it it's present.

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

Issue #864: Add layout="responsive" to <amp-iframe>.
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

This comment has been minimized.

Copy link
Collaborator

kienstra commented Feb 21, 2018

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 kienstra reopened this Feb 21, 2018

@kienstra kienstra removed their assignment Feb 21, 2018

@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Feb 26, 2018

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

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

@csossi csossi removed their assignment Feb 27, 2018

@csossi

This comment has been minimized.

Copy link

csossi commented Feb 27, 2018

verified in QA

@ThierryA

This comment has been minimized.

Copy link
Collaborator

ThierryA commented Mar 1, 2018

@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

This comment has been minimized.

Copy link
Collaborator

kienstra commented Mar 1, 2018

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.

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

Issue #864: Convert 'data-amp-layout' to 'layout.'
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.

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

Issue #864: Allow 'data-amp-layout' in wp_kses() for 'post.'
Add this attribute to the allowed list for <img>.
The sanitizer now converts this to 'layout.'

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

Issue #864: Change logic for adding 'data-amp-layout'.
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.

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

Issue #864: Move add_layout() to AMP_Theme_Support.
Also, remove $context_type parameter.
It's no longer used.
Otherwise, there's no change to the function.
@ThierryA

This comment has been minimized.

Copy link
Collaborator

ThierryA commented Mar 5, 2018

Awesome, thanks @kienstra

@ThierryA ThierryA closed this Mar 8, 2018

@ThierryA ThierryA added the Sprint 5 label Mar 8, 2018

@ThierryA ThierryA added this to Beta Release in v0.7 Mar 8, 2018

@ThierryA ThierryA moved this from Beta Release to Ready for Merging in v0.7 Mar 8, 2018

@kevincoleman kevincoleman moved this from Ready for Merging to Production Release in v0.7 May 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment