Skip to content
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

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

Merged
merged 28 commits into from
Jan 24, 2018

Conversation

kienstra
Copy link
Contributor

@kienstra kienstra commented Jan 17, 2018

This is the work in progress to support all native WordPress widgets (#864). The build will fail now, as the filtering in widget() isn't present.

There isn't yet a way to instantiate the class AMP_Widgets(). And feel free to comment on the location of these files. I thought it'd be good to place them in a new includes/widgets/ directory.

This unregisters the 'Archives' and 'Categories' widgets. And registers new widgets that subclass them.

  • Filter the output of widget() in these subclasses.
  • Add support for the remaining widgets, mainly with subclasses.
  • Handle the widgets that depend on scripts, and dequeue the remaining scripts.

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 kienstra changed the title [WIP] Issue #864: Begin to handle scripts, and create subclasses [WIP] Issue #864: Native WordPress widget support, including subclasses Jan 17, 2018
public function dequeue_scripts() {
wp_dequeue_script( 'wp-mediaelement' );
wp_dequeue_script( 'mediaelement-vimeo' );
wp_dequeue_style( 'wp-mediaelement' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is necessary. The plugin is already unhooking wp_print_footer_scripts. It may be useful to keep track of when certain scripts are enqueued, as that can be a signal that certain functionality needs to be incorporated using AMP components.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @westonruter. This commit deletes dequeue_scripts().

public function init() {
add_action( 'widgets_init', array( $this, 'register_widgets' ) );
add_action( 'show_recent_comments_widget_style', '__return_false' );
add_action( 'wp_footer', array( $this, 'dequeue_scripts' ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per below, this shouldn't be necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this commit deletes this add_action() call.

*/
public function init() {
add_action( 'widgets_init', array( $this, 'register_widgets' ) );
add_action( 'show_recent_comments_widget_style', '__return_false' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually remove_filter here. Nevertheless, should this be added inside of a subclass of WP_Widget_Recent_Comments?

Copy link
Contributor Author

@kienstra kienstra Jan 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @westonruter,
Thanks for reviewing this. That's a good point to move this into a subclass of WP_Widget_Recent_Comments. How does this subclass look? It might be better if I avoided adding a filter on the __construct() method, though.

Actually remove_filter here

I initially used add_action instead of add_filter:

add_action( 'show_recent_comments_widget_style', '__return_false' );

But wouldn't this be enough to prevent outputting the styling:

add_filter( 'show_recent_comments_widget_style', '__return_false' );

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The subclass looks good.

Ryan Kienstra added 6 commits January 17, 2018 15:46
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.
Only implement the render_media() function.
@todo: filter the markup in it.
This includes a PHPUnit test class for it, which will now fail.
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.
@kienstra
Copy link
Contributor Author

kienstra commented Jan 18, 2018

#875 might handle sanitizing the entire page, making some of these commits unnecessary. But I haven't looked at it enough yet to say for sure. And this will still need special handling for some items, like the onchange attribute.

public function widget( $args, $instance ) {
ob_start();
parent::widget( $args, $instance );
$output = ob_get_clean();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kienstra Here we need to handle the dropdown behavior. I think this should strip out the script tag here via regex:

$output = preg_replace( '#<script.+?</script>#', '', $output );

And then what it needs to do is inject two additional things:

  1. Add am id attribute to the <form> with a random value, or an auto-incremented one: widget-categories-dropdown-%d, e.g. widget-categories-dropdown-123
  2. Inject an on attribute onto the <select> element like on="change:widget-categories-dropdown-123.submit".

Reference: https://www.ampproject.org/docs/reference/components/amp-form#input-events

👉 N.B. WordPress 4.9 first introduced the <form> element here: WordPress/wordpress-develop@b7c70ca

So if no <form> is present, then it should wrap the <select> with one.

On second thought, instead of output-buffering the output and modifying it, just copy the existing logic from \WP_Widget_Categories::widget() in WP 4.9 and modify it here to be valid AMP.

Copy link
Contributor Author

@kienstra kienstra Jan 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied Suggestion
Thanks For The Explanation

Hi @westonruter,
Thanks for your great explanation of how to make the dropdown work with AMP. How does this commit look?

It mainly copies \WP_Widget_Categories::widget() from 4.9, and makes the changes you suggested.

It's worked locally, with valid AMP. There are still Travis errors that I need to fix, not directly related to the commit.

Ryan Kienstra added 2 commits January 19, 2018 02:59
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.
@ThierryA
Copy link
Collaborator

ThierryA commented Jan 19, 2018

@kienstra this PR may be helpful to look at as it uses the WP core widgets in the static templates with the necessary HTML modifications be AMP valid.

Reply from Ryan: Thanks for linking to that, @ThierryA. I made a comment on that PR, asking if Koop has begun applying the static widget templates to dynamic files. Should that PR or this PR apply those widget templates?

'WP_Widget_Media_Audio' => 'AMP_Widget_Media_Audio',
'WP_Widget_Media_Gallery' => 'AMP_Widget_Media_Gallery',
'WP_Widget_Media_Image' => 'AMP_Widget_Media_Image',
'WP_Widget_Media_Video' => 'AMP_Widget_Media_Video',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see Travis is failing due to these media widgets not existing in 4.7. So I suggest before returning this array to loop over the keys and remove any for which ! class_exists( $key ).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @weston,
That's a good idea. Here's the class_exists() check, and the function is slightly refactored.

*/
public function widget( $args, $instance ) {
static $first_dropdown = true;
// @codingStandardsIgnoreLine
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this for? Is it for the text domain? If so, I suggest editing the phpcs.xml to include default, such as:

<property name="text_domain" value="amp,default" />

And then add 'default' as the text domain for __( 'Categories' ) and others.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @westonruter. That @codingStandardsIgnoreLine was for the text domain. Thanks for suggesting a solution. It's applied here.

* @since 4.9.0 Added the `$instance` parameter.
*
* @param array $cat_args An array of Categories widget options.
* @param array $instance Array of settings for the current widget.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of copying the phpdoc, you can /** This filter is documented in ... */

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this line applies your suggestion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do the same with the widget_categories_args filter here.

amp.php Outdated
@@ -71,6 +71,7 @@ function amp_after_setup_theme() {
}

add_action( 'init', 'amp_init' );
add_action( 'init', 'amp_add_widget_support' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this only be done if amp_is_canonical()?

Copy link
Contributor Author

@kienstra kienstra Jan 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @westonruter, good catch. It should at least check that it is_amp_endpoint(). We wouldn't want these subclass widgets on a standard WP page.

If by chance we're going to use the widget templates in this theme pull request, we should probably check that amp_is_canonical().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The theme should be styling whatever markup the the widgets in core (or the plugin) output. The AMP widget functionality you're adding here should only apply if is_amp_endpoint() because this will true both when viewing AMP in paired mode and when canonical.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @westonruter. You probably saw it, but this commit applies your suggestion.

I had tried to wrap the action hook in the conditional:

add_action( 'init', 'amp_add_widget_support' )
But this produced a notice:

is_amp_endpoint was called <strong>incorrectly</strong>. is_amp_endpoint() was called before the &#39;parse_query&#39; hook was called.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed a suggested change in dc3f5b4 to fix.

* @return string $dropdown The filtered markup of the dropdown.
*/
public function modify_select( $dropdown ) {
$new_select = sprintf( '<select on="change:widget-categories-dropdown-%d.submit"', esc_attr( $this->number ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brilliant use of $this->number here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Weston!

Ryan Kienstra and others added 8 commits January 19, 2018 21:27
…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 &#39;parse_query&#39; hook was called.
@kienstra
Copy link
Contributor Author

kienstra commented Jan 23, 2018

Question About Widgets In Theme

Hi @westonruter,
Thanks for your commit to handle widget numbers.

Should this pull request apply the theme widget templates? I'd imagine those were intended to be separate. But it might be strange having widget subclasses in the plugin and the theme.

@@ -154,6 +154,8 @@ public static function register_paired_hooks() {
*/
public static function register_hooks() {

add_action( 'widgets_init', array( __CLASS__, 'register_widgets' ) );
Copy link
Contributor Author

@kienstra kienstra Jan 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Order Of Actions

Hi @westonruter,
It looks like 'widgets_init' has already fired by the time register_hooks() executes.

AMP_Theme_Support::init() is called on the 'wp' action, which is after 'widgets_init'.

I had ordered the actions incorrectly, even before your commits today. And my suggestion of checking is_amp_endpoint() probably led to this. That function needs to be called before 'parse_query', which is already after 'widgets_init'.

Of course, I'm happy to debug this. I'm working on verifying the @todos in the widgets now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kienstra great point. It looks like maybe the theme-support logic needs to be removed from amp_maybe_add_actions() to instead be called earlier? But yeah, tough spot with the dependency on the parse_query action.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved register_widgets()

Hi @westonruter,
What do you think of this commit, which ensures that register_widgets() fires in the right place?

Ryan Kienstra added 2 commits January 22, 2018 20:43
The markup is filtered with AMP_Theme_Support::filter_the_content().
Still, I need to apply the dropdown to the 'Archives' widget.
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.
@westonruter
Copy link
Member

Any of the widgets that are subclassed to merely do output buffering would be able to be removed once #888 is merged.

Ryan Kienstra added 2 commits January 22, 2018 23:10
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' ).
@kienstra
Copy link
Contributor Author

kienstra commented Jan 23, 2018

Regression: amp-form extension

It looks like the 'Archives' and 'Categories' widgets need the amp-form extension:

The tag 'FORM [method=GET]' requires including the 'amp-form' extension JavaScript

This edit might be related to it. But I'll look at this more tomorrow (Tuesday).

Ryan Kienstra added 3 commits January 23, 2018 10:36
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.
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.
@kienstra
Copy link
Contributor Author

kienstra commented Jan 23, 2018

Will Look At 'Categories' and 'Archives' Regression

Hi @westonruter,
The full-page sanitization in #888 simplifies this a lot. Now that's it's merged into develop and this feature branch, the commit above removes the widget subclasses that only sanitized the output.

There looks to be an issue with the 'Archives' and 'Categories' widget 'dropdowns' being stripped in sanitization. I'll look at that.

* Supply target=_top to forms to prevent removal.
* Use AMP.navigateTo(url=event.value) in archives widget instead of submitting form with URL as query param.
* Replace modify_select filter with simpler force of echo=false, re-using form ID in single method.
*/
public function form_script( $scripts ) {
if ( ! isset( $scripts['amp-form'] ) ) {
$scripts['amp-form'] = 'https://cdn.ampproject.org/v0/amp-form-latest.js';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unnecessary once you merged from develop in 62a924f

@westonruter westonruter changed the title [WIP] Issue #864: Native WordPress widget support, including subclasses Issue #864: Native WordPress widget support, including subclasses Jan 24, 2018
*/
public function widget( $args, $instance ) {
if ( ! is_amp_endpoint() ) {
parent::widget( $args, $instance );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent move to defer the is_amp_endpoint() check to when the widget is actually called.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants