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

Add theme support for Twenty Twenty (and WordPress 5.3) #3342

Closed
westonruter opened this issue Sep 24, 2019 · 18 comments · Fixed by #3403 or #3663
Closed

Add theme support for Twenty Twenty (and WordPress 5.3) #3342

westonruter opened this issue Sep 24, 2019 · 18 comments · Fixed by #3403 or #3663
Labels
Enhancement New feature or improvement of an existing one P0 High priority QA passed Has passed QA and is done
Milestone

Comments

@westonruter
Copy link
Member

WordPress 5.3 will have a new default core theme: Twenty Twenty. We need to make sure that it works as well as possible in AMP.

Note that Twenty Nineteen support (#1587) did not have 100% feature parity in AMP specifically due to the navigation+ menu on desktop and the sidebar menu on mobile. Ideally we'd be able to achieve complete parity. Note that this may involve submitting patches to Twenty Twenty itself to reduce dependence on JavaScript for implementing the desired functionality.

@westonruter westonruter added Enhancement New feature or improvement of an existing one P0 High priority labels Sep 24, 2019
@westonruter westonruter added this to the v1.3.1 milestone Sep 24, 2019
@schlessera schlessera self-assigned this Sep 27, 2019
@schlessera
Copy link
Collaborator

An initial validation run on a site with the new theme in its latest master and the Theme Unit Test data set gives me this:

Crawling the site for AMP validity.
Validating 239 URLs...  100% [======================================] 0:41 / 0:40
Success: 0 crawled URLs have unaccepted issue(s) out of 239 total with AMP validation
issue(s); 239 URLs were crawled.
+--------------------------+-----------+---------------+
| Template or content type | URL Count | Validity Rate |
+--------------------------+-----------+---------------+
| home                     | 1         | 100%          |
| post                     | 49        | 100%          |
| page                     | 18        | 100%          |
| attachment               | 39        | 100%          |
| category                 | 66        | 100%          |
| post_tag                 | 61        | 100%          |
| author                   | 3         | 100%          |
| date                     | 1         | 100%          |
| search                   | 1         | 100%          |
+--------------------------+-----------+---------------+

Here's the "Hello World" post:
Image 2019-09-28 at 9 37 30 AM

It's pretty much the same validation errors on all pages, except for those that contain the VideoPress block. They have additional JS validation issues due to the embed.

I'll dive deeper know to understand what that means and how to fix it.

@schlessera
Copy link
Collaborator

Here's the manifest from the frontpage, which has visible defects in terms of styling some of the blocks:

The style[amp-custom] element is populated with:
--
  | 0 B: style[type=text/css]
  | 81 B: style
  | 196 B (49%): link#amp-default-css[rel=stylesheet][href=https://amp-wp.localhost/wp-content/plugins/amp/assets/css/amp-default.css?ver=1.3.1-alpha][type=text/css][media=all]
  | 46595 B (76%): link#twentytwenty-style-css[rel=stylesheet][href=https://amp-wp.localhost/wp-content/themes/twentytwenty/style.css?ver=1.0][type=text/css][media=all]
  | 39 B: style[type=text/css][media=print]
  | 78 B: p.amp-wp-200a92a
  | 164 B: div.wp-block-cover has-background-dim alignleft amp-wp-758017c
  | 150 B: div.wp-block-cover has-pale-pink-background-color has-background-dim has-left-content aligncenter amp-wp-9418096
  | 151 B: div.wp-block-cover has-background-dim alignwide amp-wp-16a1d9a
  | 87 B: div.wp-block-cover has-background-dim-60 has-background-dim amp-wp-661b5da
  | 74 B: div.wp-block-spacer amp-wp-833a708[aria-hidden=true]
  | 83 B: figure.wp-block-pullquote amp-wp-d51f7b5
  | 79 B: p.amp-wp-f6e3d7f
  | 80 B: p.amp-wp-13076d9
  | 77 B: p.amp-wp-f14de1e
  | 87 B: p.has-text-color has-background has-very-light-gray-color amp-wp-3f44715
  | 76 B: p.has-text-color amp-wp-2800f28
  | 151 B: div.wp-block-cover has-background-dim amp-wp-8a2632a
  | Total included size: 48,248 bytes (76% of 62,963 total after tree shaking)
  |  
  | The following stylesheets are too large to be included in style[amp-custom]:
  | 13404 B (78%): link#wp-block-library-css[rel=stylesheet][href=https://amp-wp.localhost/wp-includes/css/dist/block-library/style.min.css?ver=5.2.3][type=text/css][media=all]
  | 2099 B (95%): style#twentytwenty-style-inline-css[type=text/css]
  | Total excluded size: 15,503 bytes (80% of 19,235 total after tree shaking)
  |  
  | Total combined size: 63,751 bytes (77% of 82,198 total after tree shaking)

These defects are gone once you open these posts in singular view.

@westonruter
Copy link
Member Author

westonruter commented Sep 28, 2019

What I see from looking at the Gutenberg demo post:

  • The theme is using meta[http-equiv=Content-Type] when it should be using meta[charset]. This conversion could be handled by the post-processor instead. Update: See Use HTML5 meta charset WordPress/twentytwenty#640.
  • The theme is enqueueing a construct.js which handles all of the interactive features. This needs to be evaluated.
  • The theme includes some standard no-js => js class name replacement. This we handle in the core theme sanitizer by just unhooking wp_head actions like twentyseventeen_javascript_detection.

Interestingly I wasn't seeing excessive_css. My CSS manifest included:

The style[amp-custom] element is populated with:
   220 B: style[type=text/css]
    81 B: style
   583 B (91%): link#amp-default-css[rel=stylesheet][href=https://wordpressdev.lndo.site/content/plugins/amp/assets/css/amp-default.css?ver=1.3.1-alpha][type=text/css][media=all]
  5149 B (33%): link#wp-block-library-css[rel=stylesheet][href=https://wordpressdev.lndo.site/content/plugins/gutenberg/build/block-library/style.css?ver=1569601791][type=text/css][media=all]
     0 B: link#contact-form-7-css[rel=stylesheet][href=https://wordpressdev.lndo.site/content/plugins/contact-form-7/includes/css/styles.css?ver=5.1.4][type=text/css][media=all]
 24216 B (65%): link#twentytwenty-style-css[rel=stylesheet][href=https://wordpressdev.lndo.site/content/themes/twentytwenty/style.css?ver=0.1][type=text/css][media=all]
    39 B: style[type=text/css][media=print]
   119 B: div.wp-block-cover has-background-dim alignwide amp-wp-d28a8c9
    88 B: img.wp-smiley amp-wp-843f19c[src=https://s.w.org/images/core/emoji/12.0.0-1/72x72/1f44b.png][alt=👋][width=72][height=72]
Total included size: 30,495 bytes (56% of 54,032 total after tree shaking)

I suppose it's just because I don't have enough content on the page. The excessive CSS issue would be addressed by #2326 on Standard mode sites. We should evaluate whether the amount of CSS is going to be too much in a normal case. I wasn't able to see excessive_css when using the Block Unit Test data either:

The style[amp-custom] element is populated with:
    81 B: style
   139 B (41%): link#amp-default-css[rel=stylesheet][href=https://wordpressdev.lndo.site/content/plugins/amp/assets/css/amp-default.css?ver=1.3.1-alpha][type=text/css][media=all]
  8764 B (52%): link#wp-block-library-css[rel=stylesheet][href=https://wordpressdev.lndo.site/content/plugins/gutenberg/build/block-library/style.css?ver=1569601791][type=text/css][media=all]
     0 B: link#contact-form-7-css[rel=stylesheet][href=https://wordpressdev.lndo.site/content/plugins/contact-form-7/includes/css/styles.css?ver=5.1.4][type=text/css][media=all]
 24058 B (65%): link#twentytwenty-style-css[rel=stylesheet][href=https://wordpressdev.lndo.site/content/themes/twentytwenty/style.css?ver=0.1][type=text/css][media=all]
    39 B: style[type=text/css][media=print]
   179 B: div.wp-block-cover has-background-dim amp-wp-6730970
Total included size: 33,260 bytes (60% of 55,004 total after tree shaking)

@schlessera How many posts do you have set to display on the front page? And I assume full content is being displayed?

@schlessera
Copy link
Collaborator

How many posts do you have set to display on the front page? And I assume full content is being displayed?

It is set to display 10 posts, however it is displaying 11 because of the sticky post.

@markbiernat
Copy link

I can confirm that using AMP+"Twenty Twenty" + 3.5 without any special plugins.
The mobile menu and search tool at the top does not work.
Jetpack mobile is not used.
I do not know if this is the right place to post that. However, I am trying to test as much as AMP+Twenty Twenty as I can and thought I would post it as I know you want to clean it up as much as possible before release.

@schlessera
Copy link
Collaborator

The CSS issue on my site is produced because my front page shows the full post for testing the core blocks. So, if a site uses many core blocks, the block stylesheet provided by core will blow up (almost hitting the 50kb on its own here). The more blocks that are used, the less the tree shaking can remove from that big stylesheet, of course.

@westonruter
Copy link
Member Author

westonruter commented Sep 30, 2019

@markbiernat Thanks! This is the exact place to submit your testing feedback. The mobile menu not working is due to this point above:

  • The theme is enqueueing a construct.js which handles all of the interactive features. This needs to be evaluated.

This file has since been named to index.js. From looking at the source, at the moment we need to account for:

@schlessera
Copy link
Collaborator

the block stylesheet provided by core will blow up

Correction: It's the stylesheet provided by twentytwenty that is blowing up, and therefore the block stylesheet from Core is pushed out of the <style amp-custom> block.

Looking at the generated styles, one big part of that deals with the custom font, "Inter".

Also, some styling is overly verbose because the Gutenberg markup is missing vital information for targeting things like "any block" or "no alignment set", like this example:

.wp-block-columns:not(.alignwide):not(.alignfull),.wp-block-cover:not(.alignwide):not(.alignfull):not(.alignleft):not(.alignright):not(.aligncenter),.wp-block-embed:not(.alignwide):not(.alignfull):not(.alignleft):not(.alignright):not(.aligncenter),.wp-block-gallery:not(.alignwide):not(.alignfull):not(.alignleft):not(.alignright):not(.aligncenter),.wp-block-image:not(.alignwide):not(.alignfull):not(.alignleft):not(.alignright):not(.aligncenter),.wp-block-media-text:not(.alignwide):not(.alignfull),.wp-block-pullquote:not(.alignwide):not(.alignfull):not(.alignleft):not(.alignright):not(.aligncenter),.wp-block-quote,.wp-block-quote.is-style-large,.wp-block-video:not(.alignwide):not(.alignfull):not(.alignleft):not(.alignright):not(.aligncenter){margin-bottom:5rem;margin-top:5rem}

@csossi
Copy link

csossi commented Oct 29, 2019

"Search" isn't working on mobile. I can call up the Search field by tapping the magnifying glass, but if I tap within the field to enter a search term, the field disappears

@schlessera schlessera reopened this Oct 29, 2019
@schlessera
Copy link
Collaborator

I can confirm the issue and will create a new PR to fix it.

@schlessera
Copy link
Collaborator

The culprit seems to be this change: c9b9afb#diff-5d05e505f93786061c2078486784c46dR1744-R1748

I'll need to think of a way to fix this without reintroducing the initial problem it was meant to fix.

@westonruter
Copy link
Member Author

"Search" isn't working on mobile. I can call up the Search field by tapping the magnifying glass, but if I tap within the field to enter a search term, the field disappears

@csossi Fix is ready for QA.

@csossi
Copy link

csossi commented Nov 11, 2019

verified in qa

@markbiernat
Copy link

Is there any update to when the AMP menu and Search will work with the Twenty Twenty theme?

@westonruter
Copy link
Member Author

They work just fine.

Given Twenty Twenty with a header looking like so:

image

The Search modal opens:

image

And the mobile menu opens:

image

@markbiernat
Copy link

@westonruter thank you for the reply.
https://political-economy.com/
is an example of a site of mine which it does not work on. I tried various setups and I know it works on the twenty nineteen theme, but as soon as I switch it to Twenty Twenty it does not.

@westonruter
Copy link
Member Author

Humm. For some reason the button elements for the nav menu and search do not have the expected on attribute added.

On your site:

<button class="toggle search-toggle" data-toggle-target=".search-modal" data-toggle-screen-lock="true" data-toggle-body-class="showing-search-modal" data-set-focus=".search-modal .search-field" aria-expanded="false">...</button>

On my machine:

<button class="toggle search-toggle desktop-search-toggle" data-toggle-target=".search-modal" data-toggle-body-class="showing-search-modal" data-set-focus=".search-modal .search-field" aria-expanded="false" on="tap:amp-wp-id.open,amp-wp-id.toggleClass(class=show-modal,force=true),body.toggleClass(class=showing-modal,force=true),amp-wp-id.toggleClass(class='active'),AMP.setState({amp_wp_id: !amp_wp_id}),amp-wp-id-3.toggleClass(class='active'),amp-wp-id-4.toggleClass(class='active'),amp-wp-id-5.toggleClass(class='active'),body.toggleClass(class='showing-search-modal'),search-form-1.focus" id="amp-wp-id-4" data-amp-bind-aria-expanded="amp_wp_id ? 'true' : 'false'">...</button>

I think it's because your theme slug is twentytwenty-master instead of the expected twentytwenty. Rename the theme directory in wp-content/themes to twentytwenty and that should fix the issue. (Switch back to Twenty Nineteen while doing this.)

@markbiernat
Copy link

Thank you very much. It is appreciated. I am surprised I did not see that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or improvement of an existing one P0 High priority QA passed Has passed QA and is done
Projects
None yet
5 participants