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 AMP compatibility and implement rendering on archive/home pages #32

Merged
merged 7 commits into from
Oct 1, 2020

Conversation

westonruter
Copy link
Contributor

Fixes #5.

This PR adds support for AMP by conditionally rendering amp-mathml on AMP pages. This will allow us to feature the plugin on the amp-wp.org plugin ecosystem page, and we can move forward with deprecating the AMP MathML block in the AMP plugin (see ampproject/amp-wp#4556).

Before After
Screen Shot 2020-09-15 at 09 53 18 Screen Shot 2020-09-15 at 10 16 56

It also ensures that MathML blocks are rendered on archive pages, whereas previously they were only rendered on singular templates:

Before After
image image

Other changes:

  • On non-AMP pages, the MathJax script is printed when the first MathML block is printed. This script is also printed with async. These two changes ensure no render blocking.
  • Construction and registration of MathJax script URL is made more DRY.
  • MathML block is registered server-side, though this can be further improved to reduce duplication in JS.

Some of the changes here are based on some learnings from the Syntax-highlighting Code Block plugin. For printing scripts and styles just-in-time with the rendering of blocks, see also WordPress/gutenberg#21838 (comment).

@adamsilverstein
Copy link
Owner

Excellent, thank you for your contribution @westonruter - I was just thinking about working on this and found your PR!

I'll give this a test.

*/
function is_amp() {
return (
( function_exists( 'amp_is_request' ) && \amp_is_request() )
Copy link
Owner

Choose a reason for hiding this comment

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

@westonruter is this the newer 2.0+ preferred way to detect is_amp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In reality it is identical. We just renamed is_amp_endpoint() to amp_is_request() and then added a soft-deprecated is_amp_endpoint() which calls amp_is_request(). This was done just to start using prefixed functions everywhere. We're not going to hard-deprecate it.

Copy link
Owner

Choose a reason for hiding this comment

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

👍 ah, ok so purely a naming convention based change. thanks for clarifying.

* @return string Rendered block.
*/
function render_block( $attributes, $content = '' ) {
if ( is_admin() || ! preg_match( '#^(?P<start_div>\s*<div.*?>)(?P<formula>.+)(?P<end_div></div>\s*)$#s', $content, $matches ) ) {
Copy link
Owner

Choose a reason for hiding this comment

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

What does this regex do? The goal is to only enqueue the mathjax script on the front end when there are (inline) tags or mathml blocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The regex is for parsing the $content, in particular to isolate the formula from the start tag and end tag for the enclosed div. This is done not only to extract the formula to supply into the amp-mathml tag, but also in the non-AMP version to be able to inject the scripts inside the block wrapper. This ensures that styles that target blocks that have a certain sibling won't get tripped up by the injected script. The scripts are printed right away instead of waiting for wp_footer in order to minimize the flash of unstyled content.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently the regex needs to be adapted to support inline <mathml> tags per below.

@adamsilverstein
Copy link
Owner

@westonruter Excellent work, thank you for all of the improvements included here!

I left a few code comments/questions and have some additional feedback here:

  • The script tag is inserted inside the (first) standalone math element on the page, is that intentional? it seems like it should appear immediately before or after the math tag:

image

  • This doesn't currently support inline formulas:

image

  • Note: Inline formulas are easiest to add in the source code view, to test paste this inline inside a block of text:

<mathml>\( \cos(θ+φ)=\cos(θ)\cos(φ)−\sin(θ)\sin(φ) \)</mathml>

@westonruter
Copy link
Contributor Author

  • The script tag is inserted inside the (first) standalone math element on the page, is that intentional? it seems like it should appear immediately before or after the math tag:

This was intentional, yes. It's to prevent sibling selectors from breaking when trying to target one block followed by another. See #32 (comment). It didn't seem to cause any problem for the script to be immediately next to the text node containing the formula.

  • This doesn't currently support inline formulas:

Is this only available when using the code editor? The mathml element is not in the allowlist for Kses, so won't it get stripped out for users who cannot unfiltered_html?

@adamsilverstein
Copy link
Owner

adamsilverstein commented Sep 30, 2020

Is this only available when using the code editor?

No, if you type the formula directly in a paragraph, select it and hit the shortcut key (cmd-m) or choose MathML from the ... menu that will insert a formula inline. or shortcut-type formula-shortcut also works. The reason I suggested source code view is that if you try to paste a formula inline in the wysiwyg mode, it will strip/interpret the encoding or slashing.

The mathml element is not in the allowlist for Kses, so won't it get stripped out for users who cannot unfiltered_html?

Good point, I had not thought of that. I will work on this in a separate issue, it should be safe to add the tag to kses allowed list when the plugin is active.

@adamsilverstein
Copy link
Owner

The mathml element is not in the allowlist for Kses, so won't it get stripped out for users who cannot unfiltered_html?

Good point, I had not thought of that. I will work on this in a separate issue, it should be safe to add the tag to kses allowed list when the plugin is active.

Something like #35 should work (untested).

… add/amp-compat

* 'master' of github.com:adamsilverstein/mathml-block:
  run on all tags
  Require PHP 5.6 in readme
  Require PHP 5.6
  Next version is 1.1.5 (adamsilverstein#34)
  Bump for wp 5.5, version 1.1.2. (adamsilverstein#33)
  Make JS translations work (adamsilverstein#29)
@westonruter
Copy link
Contributor Author

OK, I've added support for the inline Math in a72edfa. It seems that the non-AMP version doesn't properly show the Math inline:

image

@adamsilverstein
Copy link
Owner

@westonruter Great, thank you! I'll take a look at why the inline rendering isn't working as expected.

@adamsilverstein
Copy link
Owner

@westonruter it worked well in my testing, can you give me the source code for the post content you are testing? (... -> Code Editor). Mine is:

<!-- wp:mathml/mathmlblock -->
<div class="wp-block-mathml-mathmlblock">\[x = {-b \pm \sqrt{b^2-4ac} \over 2a}\]</div>
<!-- /wp:mathml/mathmlblock -->

<!-- wp:paragraph -->
<p>Scelerisque fringilla magna a dapibus class sapien mauris, hendrerit praesent cras diam vivamus ridiculus bibendum, condimentum platea luctus nostra suspendisse lacinia. Cum metus sem nostra sapien urna magna, leo laoreet inceptos vulputate maecenas mattis placerat, tristique est tincidunt magnis tellus. <mathml>\( \cos(θ+φ)=\cos(θ)\cos(φ)−\sin(θ)\sin(φ) \)</mathml> Tempor purus sociis blandit vel cubilia ut vivamus quisque platea eleifend scelerisque, ligula mauris nisi aliquam lacus sapien curabitur mollis lacinia inceptos eros quis, condimentum sem metus placerat et vulputate ornare dui litora non.</p>
<!-- /wp:paragraph -->

<!-- wp:mathml/mathmlblock -->
<div class="wp-block-mathml-mathmlblock"></div>
<!-- /wp:mathml/mathmlblock -->

<!-- wp:paragraph -->
<p></p>
<!-- /wp:paragraph -->

Anyway, output looks good to me!

@westonruter
Copy link
Contributor Author

Humm. Your example looks good to me as well.

My post_content is:

<!-- wp:paragraph -->
<p>This is inline: <mathml>\[x = 2a\]</mathml> This is after</p>
<!-- /wp:paragraph -->

Maybe my formula is just buggy.

@adamsilverstein
Copy link
Owner

My test preview:
image

@westonruter
Copy link
Contributor Author

OK! 🤷‍♂️

@adamsilverstein
Copy link
Owner

Oh, I see what it is - for inline math you need to use slightly different format: start with \( end with \) and it works:

<!-- wp:paragraph -->
<p>This is inline: <mathml>\(x = 2a\)</mathml> This is after</p>
<!-- /wp:paragraph -->

image

See http://docs.mathjax.org/en/latest/input/tex/delimiters.html

@adamsilverstein
Copy link
Owner

Thanks again for all your work here @westonruter 🎉 !

@adamsilverstein adamsilverstein changed the title Add AMP compatibility and implement rendering on archive pages Add AMP compatibility and implement rendering on archive/home pages Oct 1, 2020
@adamsilverstein adamsilverstein merged commit cdc8138 into adamsilverstein:master Oct 1, 2020
@westonruter
Copy link
Contributor Author

Ah, ok. It's what I get for trying to write an example for a syntax I don't know!

@adamsilverstein
Copy link
Owner

Ah, ok. It's what I get for trying to write an example for a syntax I don't know!

Ha! My knowledge extends only slightly further than yours... Enough to copy examples to test and build all these components. Getting inline working correctly was tricky when building the amp-mathml component, so I knew it was important to support.

Comment on lines +154 to +159
// Add same margins as .MJXc-display.
?>
<style class="amp-mathml">
.wp-block-mathml-mathmlblock amp-mathml { margin: 1em 0; }
</style>
<?php
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was wrong. It caused a bug in legacy Reader mode. See #38.

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.

Add support for AMP
2 participants