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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add sanitizer to fixup issues with core themes #1074

Merged
merged 33 commits into from Jun 4, 2018

Conversation

Projects
None yet
4 participants
@westonruter
Copy link
Member

westonruter commented Apr 14, 2018

  • Add base sanitizer framework for fixing up core themes.
  • Force SVG support in Twenty Seventeen
  • Force fixed background attachment in Twenty Seventeen
  • Ensure custom backgrounds work.
  • Ensure color schemes work.
  • Ensure header video works in AMP across core themes; see #1078
  • Remove enqueued JS and replace with AMP equivalents where possible
  • Ensure header image works in AMP across core themes; see #1094
  • Twenty Seventeen: Implement setQuotesIcon().
  • Twenty Seventeen: Add support for sticky nav, that is adjustScrollClass().
  • Improve accept_validation_errors to do partial checks.
  • Twenty Seventeen+Sixteen: Try to implement belowEntryMetaClass().
  • Try to figure out a way to implement onResizeARIA() (may be impossible due to lack of trusted actions in AMP).

Initial Themes to Check

  • Twenty Seventeen
  • Twenty Sixteen
  • Twenty Fifteen

Previous core themes would be good to test as well, but they could be looked at in a separate PR.

馃憠 Commits pushed up to this PR (in the core-themes branch) will be automatically deployed to https://core-themes-ampconfdemo.pantheonsite.io/

AMP can be disabled on this environment for a given URL to see what it looks like normally by adding a ?amp_disabled query param, for example: https://core-themes-ampconfdemo.pantheonsite.io/?amp_disabled

Fixes #1060
Fixes #1036
Fixes #1078

@westonruter westonruter added this to the v1.0 milestone Apr 14, 2018

'twentyseventeen' => array(
'force_svg_support' => array(),
'force_fixed_background_support' => array(),
// @todo Header video probably needs special support in the plugin generally.

This comment has been minimized.

Copy link
@westonruter

westonruter Apr 17, 2018

Author Member

See #1078

'force_svg_support' => array(),
'force_fixed_background_support' => array(),
// @todo Header video probably needs special support in the plugin generally.
// @todo Header image is not styled properly. Needs layout=responsive and other things?

This comment has been minimized.

Copy link
@westonruter
@westonruter

This comment has been minimized.

Copy link
Member Author

westonruter commented May 2, 2018

This PR adds amp theme support to _s, which could have some useful pieces, especially in regards to the nav menus: Automattic/_s#1286

@postphotos

This comment has been minimized.

Copy link
Contributor

postphotos commented May 17, 2018

Related #1094.

@postphotos postphotos added the Sprint 6 label May 17, 2018

@postphotos

This comment has been minimized.

Copy link
Contributor

postphotos commented May 17, 2018

We still would like testing (configure AMP in Customizer) and document what's clearly a problem.

kienstra and others added some commits May 17, 2018

Begin AMP header video implementation.
This is still WIP, free for someone to work on.
@todo: as Weston mentioned, this needs to support Vimeo.
Begin PHPUnit test of AMP header video implementation.
Also, simplify the conditionals at the
top of conditionally_output_header_video().
@todo: test the case where the header video
is an <amp-video>, not an <amp-youtube>.
@DavidCramer

This comment has been minimized.

Copy link
Contributor

DavidCramer commented May 30, 2018

@kienstra The image and video is almost working. It's a little blocked by #1175 as it requires those styles to correctly make the images and video looks the same. Still a little more to do and still need to update the unit tests to reflect all the changes.

DavidCramer added some commits May 31, 2018

$atts['layout'] = 'fill';
unset( $atts['width'] );
$place_holder = AMP_HTML_Utils::build_tag( 'img', $atts );

This comment has been minimized.

Copy link
@westonruter

westonruter May 31, 2018

Author Member

img or amp-img? I think this should be creating an amp-img here.

This comment has been minimized.

Copy link
@DavidCramer

DavidCramer May 31, 2018

Contributor

Oops, sorry, it was, I removed it for testing something, but forgot to revert 601c005

@westonruter

This comment has been minimized.

Copy link
Member Author

westonruter commented Jun 1, 2018

@DavidCramer in regards to the header image then, it's going to need to be part of the sanitizer here to add a rule with selector such as amp-img img { object-fit:contain } to ensure that the header is displayed properly.

@DavidCramer

This comment has been minimized.

Copy link
Contributor

DavidCramer commented Jun 1, 2018

@westonruter yes, I expected as much. however, not all headers for all core themes work that way, so I have made it part of the twentyseventeen core sanitiser here dc9e628#diff-5d05e505f93786061c2078486784c46dR97

@DavidCramer

This comment has been minimized.

Copy link
Contributor

DavidCramer commented Jun 1, 2018

@westonruter Feel free to review the header video and image for this, I think it works well now.

westonruter added some commits Jun 2, 2018

westonruter and others added some commits Jun 2, 2018

Add core theme sanitizer features for dequeueing scripts and removing鈥
鈥 actions

* Dequeue core themes' scripts for navigation, skip-link-focus-fix, keyboard-image-navigation.
* Remove actions that output unneeded scripts.
* Dequeue comments-reply unconditionally.
Simplify implementation of Twenty Seventeen's setQuotesIcon()
Also move the get_theme_config method to be closer to logically-related $theme_features class var

@westonruter westonruter requested a review from kienstra Jun 4, 2018

@westonruter westonruter changed the title [WIP] Add sanitizer to fixup issues with core themes Add sanitizer to fixup issues with core themes Jun 4, 2018

@westonruter

This comment has been minimized.

Copy link
Member Author

westonruter commented Jun 4, 2018

I believe we've done all that can be done here. I'm really happy with the result. Let's get this to testing.

@kienstra kienstra self-assigned this Jun 4, 2018

@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Jun 4, 2018

Thanks, Reviewing Now

Hi @westonruter,
Thanks, this looks great on the multidev sites. I'm reviewing this now.

@kienstra
Copy link
Collaborator

kienstra left a comment

Approved

Hi @westonruter,
This looks really good. There are some comments below about approach, but I think it's ready to merge if you're happy with them.

};
if ( ! has_header_video() ) {
return self::output_header_image( $atts );

This comment has been minimized.

Copy link
@kienstra

kienstra Jun 4, 2018

Collaborator

Nice use of these helper functions to simplify conditionally_output_header()

*
* This is JS-driven in Core themes like Twenty Sixteen and Twenty Seventeen.
* So in order for the header video to display,
* this replaces the markup of the header image.

This comment has been minimized.

Copy link
@kienstra

kienstra Jun 4, 2018

Collaborator

This isn't a blocker, but maybe this 3-line DocBlock description could be removed. It looks to be the same as in conditionally_output_header().

*
* This is JS-driven in Core themes like Twenty Sixteen and Twenty Seventeen.
* So in order for the header video to display,
* this replaces the markup of the header image.

This comment has been minimized.

Copy link
@kienstra

kienstra Jun 4, 2018

Collaborator

Similar to the suggestion above, maybe these 3 lines could be removed.

* @return string $html Filtered markup.
*/
public static function output_header_image( $atts ) {

This comment has been minimized.

Copy link
@kienstra

kienstra Jun 4, 2018

Collaborator

This isn't a blocker, but this might be cleaner as:

public static function output_header_image( $atts ) {
        return AMP_HTML_Utils::build_tag( 'amp-img', $atts );
}

This comment has been minimized.

Copy link
@westonruter

westonruter Jun 4, 2018

Author Member

Yeah, I'm going to just remove the method entirely. There's no need for it.

'twentyseventeen-global', // There are somethings not yet implemented in AMP. See todos below.
'jquery-scrollto', // Implemented via add_smooth_scrolling().
'twentyseventeen-navigation', // Handled by add_nav_menu_styles, add_nav_menu_toggle, add_nav_sub_menu_buttons.
'twentyseventeen-skip-link-focus-fix', // Only needed by IE11 and when admin bar is present.

This comment has been minimized.

Copy link
@kienstra

kienstra Jun 4, 2018

Collaborator

It's great how these comments describe the scripts.

@@ -0,0 +1,943 @@
<?php

This comment has been minimized.

Copy link
@kienstra

kienstra Jun 4, 2018

Collaborator

It'd be great to eventually go back and add tests for this class, and other new methods.

*/
protected static $theme_features = array(
// Twenty Seventeen.
'twentyseventeen' => array(

This comment has been minimized.

Copy link
@kienstra

kienstra Jun 4, 2018

Collaborator

This is a great idea to pass an array of arguments to each of the methods, depending on the theme.

* @link https://github.com/WordPress/wordpress-develop/blob/f4580c122b7d0d2d66d22f806c6fe6e11023c6f0/src/wp-content/themes/twentyseventeen/assets/js/global.js#L105-L108
*/
public static function set_twentyseventeen_quotes_icon() {
add_filter( 'the_content', function ( $content ) {

This comment has been minimized.

Copy link
@kienstra

kienstra Jun 4, 2018

Collaborator

This isn't relevant now, but maybe a method would be better than an anonymous function here, to enable unit testing.

* and the CSS parser will then convert the selectors to amp-img and amp-video respectively.
* Nevertheless, object-fit does not apply on amp-img and it needs to apply on an actual img.
*/
add_action( 'wp_enqueue_scripts', function() use ( $args ) {

This comment has been minimized.

Copy link
@kienstra

kienstra Jun 4, 2018

Collaborator

Similar to the comment above, how about passing a method, instead of an anonymous function? This might help with unit testing, but this isn't a blocker now.

</style>
<?php
$styles = str_replace( array( '<style>', '</style>' ), '', ob_get_clean() );
wp_add_inline_style( get_template() . '-style', $styles );

This comment has been minimized.

Copy link
@kienstra

kienstra Jun 4, 2018

Collaborator

It's nice how this passes the contents of the buffer to wp_add_inline_style()

@westonruter westonruter merged commit 76e1657 into develop Jun 4, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the core-themes branch Jun 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.