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

Fix DOMDocument issues with AMP boilerplate and encodings #4141

Merged
merged 10 commits into from Sep 8, 2020

Conversation

swissspidy
Copy link
Collaborator

@swissspidy swissspidy commented Aug 20, 2020

Summary

See #4140 for details. This is a somewhat hacky workaround for libxml < 2.8, but it's also what the AMP plugin does:

  1. Remove <noscript> part from markup
  2. Parse markup into DOMDocument
  3. Re-add <noscript>

Relevant Technical Choices

  • Ensure UTF-8 encoding

To-do

User-facing changes

N/A

Testing Instructions


Fixes #4140
Fixes #4166

@swissspidy swissspidy added Type: Enhancement New feature or improvement of an existing feature Group: WordPress Changes related to WordPress or Gutenberg integration Pod: WP & Infra labels Aug 20, 2020
@google-cla google-cla bot added the cla: yes label Aug 20, 2020
@swissspidy swissspidy changed the title Fi AMP boilerplate edge case Fix AMP boilerplate edge case Aug 20, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Aug 20, 2020

Size Change: 0 B

Total Size: 1.24 MB

ℹ️ View Unchanged
Filename Size Change
assets/css/edit-story.css 296 B 0 B
assets/css/stories-dashboard.css 328 B 0 B
assets/css/web-stories-embed-block.css 515 B 0 B
assets/js/chunk-web-stories-template-0-********************.js 10.2 kB 0 B
assets/js/chunk-web-stories-template-1-********************.js 10.3 kB 0 B
assets/js/chunk-web-stories-template-2-********************.js 10 kB 0 B
assets/js/chunk-web-stories-template-3-********************.js 10.5 kB 0 B
assets/js/chunk-web-stories-template-4-********************.js 10.6 kB 0 B
assets/js/chunk-web-stories-template-5-********************.js 6.86 kB 0 B
assets/js/chunk-web-stories-template-6-********************.js 9.91 kB 0 B
assets/js/chunk-web-stories-template-7-********************.js 9.78 kB 0 B
assets/js/edit-story.js 497 kB 0 B
assets/js/stories-dashboard.js 586 kB 0 B
assets/js/web-stories-activation-notice.js 63.6 kB 0 B
assets/js/web-stories-embed-block.js 16.9 kB 0 B

compressed-size-action

Copy link
Collaborator

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

I assume most of the HTML class here won't be necessary once the common lib from the AMP plugin is added as a dependency?

includes/Story_Renderer/HTML.php Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 20, 2020

Codecov Report

Merging #4141 into main will increase coverage by 47.35%.
The diff coverage is 70.16%.

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #4141       +/-   ##
===========================================
+ Coverage   35.45%   82.81%   +47.35%     
===========================================
  Files         715      792       +77     
  Lines       12680    13948     +1268     
===========================================
+ Hits         4496    11551     +7055     
+ Misses       8184     2397     -5787     
Flag Coverage Δ
#karmatests 70.28% <62.63%> (+41.60%) ⬆️
#unittests 65.86% <58.18%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...activation-notice/app/components/messageContent.js 0.00% <ø> (ø)
...sets/src/activation-notice/app/components/step1.js 88.88% <ø> (ø)
...activation-notice/app/components/successMessage.js 90.90% <ø> (ø)
assets/src/activation-notice/app/index.js 0.00% <0.00%> (ø)
.../activation-notice/app/utils/usePrefersDarkMode.js 0.00% <0.00%> (ø)
assets/src/activation-notice/index.js 0.00% <0.00%> (ø)
assets/src/animation/components/AMPAnimations.js 100.00% <ø> (ø)
assets/src/animation/components/AMPKeyframes.js 44.44% <ø> (ø)
assets/src/animation/components/AMPWrapper.js 100.00% <ø> (ø)
assets/src/animation/components/WAAPIWrapper.js 100.00% <ø> (ø)
... and 785 more

@swissspidy
Copy link
Collaborator Author

I assume most of the HTML class here won't be necessary once the common lib from the AMP plugin is added as a dependency?

@westonruter Yeah we'd remove all redundant parts for sure.

includes/Story_Renderer/HTML.php Outdated Show resolved Hide resolved
includes/Story_Renderer/HTML.php Outdated Show resolved Hide resolved
@swissspidy swissspidy changed the title Fix AMP boilerplate edge case Fix DOMDocument issues with AMP boilerplate and encodings Sep 8, 2020
Copy link
Contributor

@spacedmonkey spacedmonkey left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Collaborator

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

LGTM, but I presume this will be very short-lived if we copy the Document class from the AMP plugin.

@swissspidy swissspidy merged commit 7cfe4bc into main Sep 8, 2020
@swissspidy swissspidy deleted the fix/amp-boilerplate-edge-case branch September 8, 2020 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Group: WordPress Changes related to WordPress or Gutenberg integration Type: Enhancement New feature or improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Encoding of special characters broken on frontend Incompatibility with older libxml versions
3 participants