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
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
3faab47
Issue #864: Dequeue some disallowed scripts, and begin subclasses.
Jan 17, 2018
be58d02
Issue #864: Remove dequeuing of scripts, and create comment subclass.
Jan 17, 2018
436722d
Issue #864: Subclass 'Gallery' widget, to output valid AMP markup.
Jan 18, 2018
44287d8
Issue #864: Subclass 'Image' widget to output valid AMP markup.
Jan 18, 2018
c7268bf
Issue #864: Subclass the 'Video' widget, in order to sanitize markup.
Jan 18, 2018
16caae9
Merge branch 'develop' into add/864-native-widget-support
Jan 18, 2018
9bd0815
Issue #864: Add filtering to widget output, more subclasses.
Jan 18, 2018
fb9b451
Issue #864: Support 'Categories' widget dropdown.
Jan 19, 2018
cbdbd4a
Issue #864: Address Travis errors, including PHPCS issue.
Jan 19, 2018
6ee75ee
Issue #864: Address failed Travis build by adding 'default' to textdo…
Jan 20, 2018
e703850
Issue #864: Remove copied filter doc, and remove non-existent widgets.
Jan 20, 2018
a355243
Issue #864: Only declare widget classes if their parents exist.
Jan 20, 2018
99dd862
Issue #864: Skip tests if the classes aren't declared.
Jan 20, 2018
74d727b
Issue #864: Only add AMP widget support if is_amp_endpoint().
Jan 22, 2018
fc48208
Make sure widget numbers start at 2
westonruter Jan 22, 2018
da6cee6
Merge branch 'develop' of https://github.com/Automattic/amp-wp into a…
westonruter Jan 23, 2018
dc3f5b4
Merge AMP_Widgets into AMP_Theme_Support
westonruter Jan 23, 2018
28b5cdd
Issue #864: Remove @todo tags from widgets, as they're implemented.
Jan 23, 2018
a0b9848
Issue #864: Make 'Archives' widget dropdown AMP-compliant.
Jan 23, 2018
6053f9a
Issue #864: Ensure register_widgets() fires on 'widget_init.'
Jan 23, 2018
e0456e1
Issue #864: Fix PHPUnit tests, accomodating is_amp_endpoint().
Jan 23, 2018
40a73c6
Issue #864: Workaround to include amp-form extension JS.
Jan 23, 2018
62a924f
Issue #864: Merge in develop, resolve conflicts.
Jan 23, 2018
c28578c
Issue #864: Remove widget subclasses that used filter_the_content().
Jan 23, 2018
6f82cc7
Fix obtaining initial widget number in add-test-widgets-to-sidebar
westonruter Jan 24, 2018
2081ab0
Remove manual enqueue of amp-form since whitelist sanitizer now handles
westonruter Jan 24, 2018
0a78830
Fix form sanitization and archives widget submission
westonruter Jan 24, 2018
0e26178
Prevent parse_query notice in is_amp_endpoint() by deferring recent c…
westonruter Jan 24, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions includes/class-amp-autoloader.php
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,9 @@ class AMP_Autoloader {
'AMP_Image_Dimension_Extractor' => 'includes/utils/class-amp-image-dimension-extractor',
'AMP_String_Utils' => 'includes/utils/class-amp-string-utils',
'AMP_WP_Utils' => 'includes/utils/class-amp-wp-utils',
'AMP_Widget_Archives' => 'includes/widgets/class-amp-widget-archives',
'AMP_Widget_Categories' => 'includes/widgets/class-amp-widget-categories',
'AMP_Widgets' => 'includes/widgets/class-amp-widgets',
'WPCOM_AMP_Polldaddy_Embed' => 'wpcom/class-amp-polldaddy-embed',
'AMP_Test_Stub_Sanitizer' => 'tests/stubs',
'AMP_Test_World_Sanitizer' => 'tests/stubs',
Expand Down
31 changes: 31 additions & 0 deletions includes/widgets/class-amp-widget-archives.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
<?php
/**
* Class AMP_Widget_Archives
*
* @package AMP
*/

/**
* Class AMP_Widget_Archives
*
* @package AMP
*/
class AMP_Widget_Archives extends WP_Widget_Archives {

/**
* Echoes the markup of the widget.
*
* @todo filter $output, to strip the onchange attribute
* @see https://github.com/Automattic/amp-wp/issues/864
* @param array $args Widget display data.
* @param array $instance Data for widget.
* @return void.
*/
public function widget( $args, $instance ) {
ob_start();
parent::widget( $args, $instance );
$output = ob_get_clean();
echo $output; // WPCS: XSS ok.
}

}
31 changes: 31 additions & 0 deletions includes/widgets/class-amp-widget-categories.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
<?php
/**
* Class AMP_Widget_Categories
*
* @package AMP
*/

/**
* Class AMP_Widget_Categories
*
* @package AMP
*/
class AMP_Widget_Categories extends WP_Widget_Categories {

/**
* Echoes the markup of the widget.
*
* @todo filter $output, to strip the <script> tag.
* @see https://github.com/Automattic/amp-wp/issues/864
* @param array $args Widget display data.
* @param array $instance Data for widget.
* @return void.
*/
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.

echo $output; // WPCS: XSS ok.
}

}
63 changes: 63 additions & 0 deletions includes/widgets/class-amp-widgets.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
<?php
/**
* Register widgets, and add actions.
*
* @package AMP
*/

/**
* Register the widgets.
*/
class AMP_Widgets {

/**
* Add the actions.
*
* @return void.
*/
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.

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.

}

/**
* Add the filters.
*
* @return void.
*/
public function register_widgets() {
$widgets = self::get_widgets();
foreach ( $widgets as $native_wp_widget => $amp_widget ) {
unregister_widget( $native_wp_widget );
register_widget( $amp_widget );
}
}

/**
* Get the widgets to unregister and register.
*
* @return array $widgets An associative array, with the previous WP widget mapped to the new AMP widget.
*/
public function get_widgets() {
return array(
'WP_Widget_Archives' => 'AMP_Widget_Archives',
'WP_Widget_Categories' => 'AMP_Widget_Categories',
);
}

/**
* Dequeue widget scripts and styles, which aren't allowed in AMP.
*
* Uses the action 'wp_footer' in order to prevent
* 'wp_print_footer_scripts' from outputting the scripts.
*
* @return void.
*/
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().

}

}
71 changes: 71 additions & 0 deletions tests/test-class-amp-widget-archives.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
<?php
/**
* Tests for class AMP_Widget_Archives.
*
* @package AMP
*/

/**
* Tests for class AMP_Widget_Archives.
*
* @package WidgetLiveEditor
*/
class Test_AMP_Widget_Archives extends WP_UnitTestCase {
/**
* Instance of the widget.
*
* @var object
*/
public $instance;

/**
* Setup.
*
* @inheritdoc
*/
public function setUp() {
parent::setUp();
$amp_widgets = new AMP_Widgets();
$amp_widgets->register_widgets();
$this->instance = new AMP_Widget_Archives();
}

/**
* Test construct().
*
* @see AMP_Widget_Archives::__construct().
*/
public function test_construct() {
global $wp_widget_factory;
$amp_archives = $wp_widget_factory->widgets['AMP_Widget_Archives'];
$this->assertEquals( 'AMP_Widget_Archives', get_class( $amp_archives ) );
$this->assertEquals( 'archives', $amp_archives->id_base );
$this->assertEquals( 'Archives', $amp_archives->name );
$this->assertEquals( 'widget_archive', $amp_archives->widget_options['classname'] );
$this->assertEquals( true, $amp_archives->widget_options['customize_selective_refresh'] );
$this->assertEquals( 'A monthly archive of your site&#8217;s Posts.', $amp_archives->widget_options['description'] );
}

/**
* Test widget().
*
* @see AMP_Widget_Archives::widget().
*/
public function test_widget() {
$arguments = array(
'before_widget' => '<div>',
'after_widget' => '</div>',
'before_title' => '<h2>',
'after_title' => '</h2>',
);
$instance = array(
'title' => 'Test Archives Widget',
'dropdown' => 1,
);
ob_start();
$this->instance->widget( $arguments, $instance );
$output = ob_get_clean();

$this->assertFalse( strpos( $output, 'onchange=' ) );
}
}
71 changes: 71 additions & 0 deletions tests/test-class-amp-widget-categories.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
<?php
/**
* Tests for class AMP_Widget_Categories.
*
* @package AMP
*/

/**
* Tests for class AMP_Widget_Categories.
*
* @package WidgetLiveEditor
*/
class Test_AMP_Widget_Categories extends WP_UnitTestCase {
/**
* Instance of the widget.
*
* @var object
*/
public $instance;

/**
* Setup.
*
* @inheritdoc
*/
public function setUp() {
parent::setUp();
$amp_widgets = new AMP_Widgets();
$amp_widgets->register_widgets();
$this->instance = new AMP_Widget_Categories();
}

/**
* Test construct().
*
* @see AMP_Widget_Categories::__construct().
*/
public function test_construct() {
global $wp_widget_factory;
$amp_categories = $wp_widget_factory->widgets['AMP_Widget_Categories'];
$this->assertEquals( 'AMP_Widget_Categories', get_class( $amp_categories ) );
$this->assertEquals( 'categories', $amp_categories->id_base );
$this->assertEquals( 'Categories', $amp_categories->name );
$this->assertEquals( 'widget_categories', $amp_categories->widget_options['classname'] );
$this->assertEquals( true, $amp_categories->widget_options['customize_selective_refresh'] );
$this->assertEquals( 'A list or dropdown of categories.', $amp_categories->widget_options['description'] );
}

/**
* Test widget().
*
* @see AMP_Widget_Categories::widget().
*/
public function test_widget() {
$arguments = array(
'before_widget' => '<div>',
'after_widget' => '</div>',
'before_title' => '<h2>',
'after_title' => '</h2>',
);
$instance = array(
'title' => 'Test Categories Widget',
'dropdown' => 1,
);
ob_start();
$this->instance->widget( $arguments, $instance );
$output = ob_get_clean();

$this->assertFalse( strpos( $output, '<script type=' ) );
}
}
67 changes: 67 additions & 0 deletions tests/test-class-amp-widgets.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
<?php
/**
* Tests for class AMP_Widgets.
*
* @package AMP
*/

/**
* Tests for class AMP_Widgets.
*/
class Test_AMP_Widgets extends WP_UnitTestCase {
/**
* Instance of AMP_Widgets.
*
* @var object
*/
public $instance;

/**
* Setup.
*
* @inheritdoc
*/
public function setUp() {
parent::setUp();
$this->instance = new AMP_Widgets();
}

/**
* Test init().
*
* @see AMP_Widgets::init().
*/
public function test_init() {
$this->instance->init();
$this->assertEquals( 10, has_filter( 'widgets_init', array( $this->instance, 'register_widgets' ) ) );
$this->assertEquals( 10, has_filter( 'show_recent_comments_widget_style', '__return_false' ) );
$this->assertEquals( 10, has_action( 'wp_footer', array( $this->instance, 'dequeue_scripts' ) ) );
}

/**
* Test register_widgets().
*
* @covers AMP_Widgets::get_widgets().
* @see AMP_Widgets::register_widgets().
*/
public function test_register_widgets() {
global $wp_widget_factory;
$this->instance->register_widgets();
$this->assertFalse( isset( $wp_widget_factory->widgets['WP_Widget_Archives'] ) );
$this->assertEquals( 'AMP_Widget_Archives', get_class( $wp_widget_factory->widgets['AMP_Widget_Archives'] ) );
$this->assertFalse( isset( $wp_widget_factory->widgets['WP_Widget_Categories'] ) );
$this->assertEquals( 'AMP_Widget_Categories', get_class( $wp_widget_factory->widgets['AMP_Widget_Categories'] ) );
}

/**
* Test dequeue_scripts().
*
* @see AMP_Widgets::dequeue_scripts().
*/
public function test_dequeue_scripts() {
$this->assertFalse( wp_script_is( 'wp-mediaelement' ) );
$this->assertFalse( wp_script_is( 'mediaelement-vimeo' ) );
$this->assertFalse( wp_style_is( 'wp-mediaelement' ) );
}

}