Map Block AMP Compatibility Alternate Approach #13405

jeffersonrabb · 2019-09-03T16:34:23Z

Changes proposed in this Pull Request:

This PR is an alternate approach to #13321, which came about due to problems described in #13321 (comment). In this approach, a URL param (map-block-counter) will cause page requests to render a complete document containing only a single Map block. The value of map-block-counter determines the ordinal of the block to render. If no block at the provided value is found, the page renders normally. In AMP requests, Map block will render as an iFrame using the special URL as the src. This approach replaces the srcdoc attribute and length/browser compatibility constraints associated with it. DOMDocument is used to extract the desired block markup from the post content.

TODO:

Improve map height inside of iframe.
Determine if width/height of amp-iframe are correct, and whether intrinsic or responsive layout should be used.
How to sanitize the full page output?
If problematic, there may be viable alternatives to ordinal, for example a hash of the Map block markup.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

Adds AMP compatibility to Map block.

Testing instructions:

Activate AMP plugin.
Add Map block to content.
View post/page in AMP.
Verify Map block still renders on a valid AMP page.
Verify Map block renders in the loop, e.g. on homepage.
Verify pages with multiple Map blocks render correctly.
Inspect page and copy the src of the amp-iframes. Load these URLs in separate browser windows. Make sure they render correctly.
Alter the value of map-block-post-id and/or map-block-counter query params. This should cause the full page to load rather than the block alone.

Proposed changelog entry for your changes:

Add AMP-compatibility to the Map block.

jetpackbot · 2019-09-03T16:36:18Z

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: December 3, 2019.
Scheduled code freeze: November 26, 2019

Generated by 🚫 dangerJS against d9492d1

westonruter

This is a clever approach! It should work well when the map blocks are inside of post_content. Checks need to be added for when this is not the case, however (e.g. on archive templates that use the_content()). Also, I suppose the block will just have to show nothing when it is being rendered outside of post content, e.g. sidebar, header, footer?

extensions/blocks/map/map.php

jeffersonrabb · 2019-10-01T15:34:15Z

@westonruter the iframe sizing should be resolved in 3a0a117

extensions/blocks/map/map.php

jeherve

Sorry for the late review, this one kinda fell through the cracks.

extensions/blocks/map/map.php

…ML page with nothing but the block. For AMP requests, render an iFrame with the special URL as src.

…lay on non-single pages.

jeffersonrabb · 2019-11-21T04:58:15Z

Rebased.

matticbot · 2019-11-21T13:56:40Z

Caution: This PR has changes that must be merged to WordPress.com
Hello jeffersonrabb! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D35755-code before merging this PR. Thank you!

jeherve

This seems to work well overall, but I'm getting the following JS error when viewing maps on non-AMP views now:

mapbox-gl.1382b28d502145eaee66.js:formatted:28909 Uncaught TypeError: Cannot read property 'off' of undefined
    at Fi.onRemove (mapbox-gl.1382b28d502145eaee66.js:formatted:28909)
    at n.removeControl (mapbox-gl.1382b28d502145eaee66.js:formatted:28172)
    at t.setBoundsByMarkers (view.js?ver=1574345509:1)
    at view.js?ver=1574345509:1
    at e (lodash.min.js?ver=4.17.15:68)
    at o (lodash.min.js?ver=4.17.15:68)
    at i (lodash.min.js?ver=4.17.15:68)
Fi.onRemove @ mapbox-gl.1382b28d502145eaee66.js:formatted:28909
n.removeControl @ mapbox-gl.1382b28d502145eaee66.js:formatted:28172
(anonymous) @ view.js?ver=1574345509:1
(anonymous) @ view.js?ver=1574345509:1
e @ lodash.min.js?ver=4.17.15:68
o @ lodash.min.js?ver=4.17.15:68
i @ lodash.min.js?ver=4.17.15:68
setTimeout (async)
bo @ lodash.min.js?ver=4.17.15:85
f @ lodash.min.js?ver=4.17.15:69

Do you get that one too?

jeffersonrabb · 2019-11-21T22:56:21Z

This seems to work well overall, but I'm getting the following JS error when viewing maps on non-AMP views now:

Hmmm I'm not getting that. Is it possible to take a look at the exact map in question?

jeherve · 2019-11-22T13:50:29Z

I can't seem to reproduce anymore, with the same map I used yesterday. ¯_(ツ)_/¯

* 8.0 Release: running changelog * Changelog: add #13921 * Changelog: add #13980 * Changelog: add #13905 * Changelog: add #13971 * Changelog: add #13984 * Changelog: add #14009 * Changelog: add #13620 * Remove things that will ship in 7.9.1 * Changelog: add 7.9.1 release (#14044) * Changelog: add base for 7.9.1 release * Update release date and post link * Changelog: add #14066 * Update changelog for 7.9.1 * Changelog: add #13405 * Changelog: add #13841 * Changelog: add #13924 * Changelog: add #13986 * Changelog: add #14010, #14028, #14053, #14055. * Changelog: add #14054 * Changelog: add #14031 * Changelog: add #14039 * Changelog: add #14050 * Changelog: add #14070 * Changelog: add #14082 * Changelog: add #14084 * Changelog: add #14111 * Changelog: add #13961 * Changelog: add #14047 * Changelog: add #14091 * Changelog: add #14108 * Changelog: add #14121

jeherve · 2020-01-23T12:57:31Z

r202026-wpcom

jeffersonrabb requested a review from a team September 3, 2019 16:34

matticbot added the Touches WP.com Files label Sep 3, 2019

jeffersonrabb added DO NOT MERGE don't merge it! [Status] In Progress AMP [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Block] Map labels Sep 3, 2019

jeffersonrabb mentioned this pull request Sep 3, 2019

Add initial AMP-compatibility to the Map block #13321

Closed

4 tasks

westonruter reviewed Sep 3, 2019

View reviewed changes

jeffersonrabb force-pushed the add/amp-block-amp-without-srcdoc branch from e651d6f to 89cd41a Compare September 29, 2019 20:50

westonruter reviewed Oct 1, 2019

View reviewed changes

jeffersonrabb added [Status] Needs Review This PR is ready for review. and removed DO NOT MERGE don't merge it! [Status] In Progress labels Oct 1, 2019

jeherve added this to the 8.0 milestone Nov 8, 2019

jeherve assigned jeffersonrabb Nov 8, 2019

jeherve added the [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it label Nov 8, 2019

jeherve requested changes Nov 20, 2019

View reviewed changes

jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Nov 20, 2019

jeffersonrabb added 8 commits November 20, 2019 23:40

Query param that causes post with Map Blocks to render as complete HT…

dbf9efe

…ML page with nothing but the block. For AMP requests, render an iFrame with the special URL as src.

Increment Map Block counter after render.

de8993d

Switch to 1-based map block counter.

e62dc6c

Use XPath to file Map Block instances by class.

6123add

Post ID in Map Rendering URLs. Track instances by post to enable disp…

0f0e617

…lay on non-single pages.

Map block ordinal as a static variable rather than global.

35d3599

Map block sizing in AMP requests.

8ca36bf

Rework iframe URL to work correctly in the loop.

9fd9c0f

jeffersonrabb added 7 commits November 20, 2019 23:40

Suppress libxml warnings.

a07566e

Check for DOMDocument class.

4d44e46

Remove filter_input, access $_GET object directly.

0733c86

Remove short array syntax.

9c4d794

Resolve PHP notice.

f7d08cb

Avoid calling get_the_ID() unnecessarily, escape the counter.

1000d17

Comment for the_content filter. Pass post_html by reference to libxml.

d9492d1

jeffersonrabb force-pushed the add/amp-block-amp-without-srcdoc branch from dd6f3fd to d9492d1 Compare November 21, 2019 04:54

jeffersonrabb added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Nov 21, 2019

jeherve reviewed Nov 21, 2019

View reviewed changes

jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Nov 21, 2019

jeherve approved these changes Nov 22, 2019

View reviewed changes

jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Nov 22, 2019

jeffersonrabb merged commit 0788e1b into master Nov 22, 2019

jeffersonrabb deleted the add/amp-block-amp-without-srcdoc branch November 22, 2019 17:18

matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Nov 22, 2019

jeherve added [Status] Has Changelog and removed [Status] Needs Changelog labels Nov 25, 2019

jeherve added a commit that referenced this pull request Nov 25, 2019

Changelog: add #13405

6aa1c01

tbradsha mentioned this pull request Feb 5, 2025

Map block: Prevent PHP fatal when content is empty #41583

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Map Block AMP Compatibility Alternate Approach #13405

Map Block AMP Compatibility Alternate Approach #13405

jeffersonrabb commented Sep 3, 2019 •

edited

Loading

jetpackbot commented Sep 3, 2019 •

edited

Loading

westonruter left a comment

jeffersonrabb commented Oct 1, 2019

jeherve left a comment

jeffersonrabb commented Nov 21, 2019

matticbot commented Nov 21, 2019

jeherve left a comment

jeffersonrabb commented Nov 21, 2019

jeherve commented Nov 22, 2019

jeherve commented Jan 23, 2020

Map Block AMP Compatibility Alternate Approach #13405

Map Block AMP Compatibility Alternate Approach #13405

Conversation

jeffersonrabb commented Sep 3, 2019 • edited Loading

Changes proposed in this Pull Request:

Is this a new feature or does it add/remove features to an existing part of Jetpack?

Testing instructions:

Proposed changelog entry for your changes:

jetpackbot commented Sep 3, 2019 • edited Loading

westonruter left a comment

Choose a reason for hiding this comment

jeffersonrabb commented Oct 1, 2019

jeherve left a comment

Choose a reason for hiding this comment

jeffersonrabb commented Nov 21, 2019

matticbot commented Nov 21, 2019

jeherve left a comment

Choose a reason for hiding this comment

jeffersonrabb commented Nov 21, 2019

jeherve commented Nov 22, 2019

jeherve commented Jan 23, 2020

jeffersonrabb commented Sep 3, 2019 •

edited

Loading

jetpackbot commented Sep 3, 2019 •

edited

Loading