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

Preserve whitespace when serializing the DOM as HTML #1309

Merged
merged 2 commits into from Aug 3, 2018

Conversation

Projects
None yet
4 participants
@westonruter
Member

westonruter commented Aug 2, 2018

Add workaround to prevent PHP from adding whitespace to format the serialization of HTML. This prevents bugs related to spaces being added between elements which can break layouts. For example, without this fix, the following HTML:

<style>
.cols-test > li {
    width: 50%;
    display: inline-block;
    outline: solid 1px red;
}
</style>
<ul class="cols-test"><li>column 1</li><li>column 2</li></ul>

Which should get rendered as:
image

Will get serialized instead as:

<ul class="cols-test">
<li>column 1</li>
<li>column 2</li>
</ul>

And displayed as (with text selected to show whitespace insertion):

image

For some more background on the changes here, see https://stackoverflow.com/q/51660286/93579

Fixes #1304.

@westonruter westonruter added this to the v1.0 milestone Aug 2, 2018

@westonruter westonruter force-pushed the fix/html-whitespace-serialization branch from df030fb to 95dc406 Aug 2, 2018

@westonruter westonruter changed the title from [WIP] Preserve whitespace when serializing the DOM as HTML to Preserve whitespace when serializing the DOM as HTML Aug 2, 2018

@westonruter westonruter requested review from kienstra and removed request for kienstra Aug 2, 2018

@westonruter westonruter changed the title from Preserve whitespace when serializing the DOM as HTML to [WIP] Preserve whitespace when serializing the DOM as HTML Aug 2, 2018

@westonruter westonruter requested a review from kienstra Aug 2, 2018

@westonruter westonruter changed the title from [WIP] Preserve whitespace when serializing the DOM as HTML to Preserve whitespace when serializing the DOM as HTML Aug 2, 2018

@westonruter

This comment has been minimized.

Member

westonruter commented Aug 2, 2018

Note that to test, do not use the <ul> example in post content. There is a the_content filter which adds line breaks for that. Instead try the markup originally reported in #1304.

@westonruter

This comment has been minimized.

Member

westonruter commented Aug 2, 2018

For anyone who wants, here is a build to test: amp.zip (v1.0-beta1-95dc4069-20180802T220041Z)

To test, deactivate and uninstall the existing AMP plugin and then go to the plugins admin page and click Upload Plugin:

image

Then select the amp.zip file to upload and then activate.

Please test this on a non-production environment.

@kienstra

kienstra approved these changes Aug 2, 2018 edited

Approved

Hi @westonruter,
This looks good. Using your code example, I also saw that this PR fixed the issue of spaces being inserted between tags.

Before this PR:
before-pr

With this PR:
after-pr

@westonruter westonruter merged commit 9460502 into develop Aug 3, 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 fix/html-whitespace-serialization branch Aug 3, 2018

@Frenchomatic

This comment has been minimized.

Frenchomatic commented Aug 4, 2018

@westonruter Just to confirm in case you missed it in my post - this fixes it all for me both in your plugin and the AMP for WP one too which uses the utils folder in particular - thank you.

@Zabi09

This comment has been minimized.

Zabi09 commented Aug 4, 2018

@westonruter
We really appreciate your hard work in fixing the spaces issue from the source code and we have tested the commits on PHP Version 7.2.5 and it was working perfectly,

Will you please tell us how to test the same with the 7.3 version of PHP and for your information we have downloaded the beta version of 7.3 version but unable to check it

Please let us know So that we can check it and update the code on our AMPforWP plugin

@westonruter

This comment has been minimized.

Member

westonruter commented Aug 4, 2018

@Zabi09 From what I can tell the issue is no longer present in PHP 7.3: https://3v4l.org/TS2tX

This being the case we can bypass the whole logic in:

https://github.com/Automattic/amp-wp/blob/95dc40692df5f3216d5b0ffa35136ab6f0b1383d/includes/utils/class-amp-dom-utils.php#L413-L414

I'll be updating the Travis build matrix to include PHP 7.3 once it is available: travis-ci/travis-ci#9717

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment