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

Width attribute on table col elements causes validation errors #1062

Closed
westonruter opened this issue Apr 6, 2018 · 4 comments · Fixed by #1075
Closed

Width attribute on table col elements causes validation errors #1062

westonruter opened this issue Apr 6, 2018 · 4 comments · Fixed by #1075
Assignees
Milestone

Comments

@westonruter
Copy link
Member

When post_content contains HTML such as:

<table class="auto-style1 sws_custom_table" style="width: 100%; display: table; background-color: #ffffff; border: 2px solid #eaeaea;" border="0" width="630" cellspacing="0"><colgroup> <col style="mso-width-source: userset; mso-width-alt: 8988; width: 190pt;" width="253" /> <col style="mso-width-source: userset; mso-width-alt: 6826; width: 144pt;" width="192" /> <col style="mso-width-source: userset; mso-width-alt: 8135; width: 172pt;" width="229" /></colgroup>
...</table>

This is getting converted to AMP as:

<table class="auto-style1 sws_custom_table amp-wp-inline-976a106f7543a9a2e8e3c8a97560be03" width="630"><colgroup><col width="253" class="amp-wp-inline-16106abec734b745f40158cc486aa0fe"/><col width="192" class="amp-wp-inline-91e2d2d529220418b62e99acb85f0928"/><col width="229" class="amp-wp-inline-43ca072c586bb2eaad4c6de773864f95"/></colgroup>
...</table>

This results in a validation error:

DISALLOWED_HTML: The attribute 'width' may not appear in tag 'col'.

First of all, maybe it is a bug in AMP that this attribute is not allowed. We should double check that first.

Otherwise, if it is really not allowed, then whitelist sanitizer may be incorrectly merging the width layout attribute among the attributes allowed on an element:

https://github.com/Automattic/amp-wp/blob/4e1cd227b10f2fcd6add44a24b078158aad41e53/includes/sanitizers/class-amp-allowed-tags-generated.php#L10573

See here:

https://github.com/Automattic/amp-wp/blob/4e1cd227b10f2fcd6add44a24b078158aad41e53/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php#L1468

layout_allowed_attributes is not globally-allowed it seems. Maybe that condition needs to be removed.

As reported on https://wordpress.org/support/topic/not-a-valid-amp-page-7/

@westonruter
Copy link
Member Author

I can see that the problem here goes beyond just col element. The sanitizer is also not stripping width attribute from elements that don't even allow in regular HTML, for example:

<span width="123">not right</span>

The sanitizer erroneously lets the width persist.

@westonruter
Copy link
Member Author

westonruter commented Apr 15, 2018

I can see that the problem here goes beyond just col element. The sanitizer is also not stripping width attribute from elements that don't even allow in regular HTML

Fix for this is in #1075

@westonruter
Copy link
Member Author

Re-opening since #1064 is not merged yet.

@westonruter westonruter reopened this Apr 15, 2018
@westonruter westonruter added this to the v1.0 milestone Apr 17, 2018
@kienstra
Copy link
Contributor

Moving To "Ready for Merging"

If it's alright, I'm moving this from "Ready for QA" to "Ready for Merging." This probably doesn't need functional testing.

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

Successfully merging a pull request may close this issue.

3 participants