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

Non-AMP element with height=100% results in invalid layout=fill added #4036

Closed
westonruter opened this issue Jan 8, 2020 · 3 comments · Fixed by #4111
Closed

Non-AMP element with height=100% results in invalid layout=fill added #4036

westonruter opened this issue Jan 8, 2020 · 3 comments · Fixed by #4111
Labels
Bug Something isn't working QA passed Has passed QA and is done Validation
Milestone

Comments

@westonruter
Copy link
Member

westonruter commented Jan 8, 2020

Bug Description

Given this markup generated by the Co-Blocks plugin' coblocks/shape-divider block:

<div class="wp-block-coblocks-shape-divider alignfull coblocks-shape-divider-11595047829 mb-0 mt-0" style="color:#111" aria-hidden="true">
	<div class="wp-block-coblocks-shape-divider__svg-wrapper" style="height:100px">
		<svg class="divider--wavy" height="100%" viewbox="0 0 100 10" width="100%" xmlns="http://www.w3.org/2000/svg" preserveaspectratio="none">
			<path d="m42.19.65c2.26-.25 5.15.04 7.55.53 2.36.49 7.09 2.35 10.05 3.57 7.58 3.22 13.37 4.45 19.26 4.97 2.36.21 4.87.35 10.34-.25s10.62-2.56 10.62-2.56v-6.91h-100.01v3.03s7.2 3.26 15.84 3.05c3.92-.07 9.28-.67 13.4-2.24 2.12-.81 5.22-1.82 7.97-2.42 2.72-.63 3.95-.67 4.98-.77z" fill-rule="evenodd" transform="matrix(1 0 0 -1 0 10)"></path>
		</svg>
	</div>
	<div class="wp-block-coblocks-shape-divider__alt-wrapper" style="height:50px"></div>
</div>

This is resulting in a validation error:

image

I believe this is a regression introduced in #3728.

Expected Behaviour

The layout attribute should only be added to AMP components. Handling a height=100% attribute on a non-AMP should get converted to a style="height:100%".


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation brief

QA testing instructions

  1. Add the following HTML to a Custom HTML block:
<svg class="divider--wavy" width="100%" height="100%" viewbox="0 0 100 10" xmlns="http://www.w3.org/2000/svg" preserveaspectratio="none">
  1. Go to the AMP page

  2. Verify that the height and width attributes on the svg element have the value 100% and there is no layout attribute added.

Demo

Changelog entry

@westonruter westonruter added Bug Something isn't working Validation labels Jan 8, 2020
@westonruter westonruter added this to the v1.5 milestone Jan 8, 2020
@pierlon pierlon self-assigned this Jan 11, 2020
@pierlon
Copy link
Contributor

pierlon commented Jan 11, 2020

Any reason why height=100% attribute on a non-AMP should get converted to a style="height:100%"?

@westonruter
Copy link
Member Author

@pierlon It should.

So the SVG above should get sanitized as follows:

- <svg class="divider--wavy" width="100%" height="100%" viewbox="0 0 100 10" xmlns="http://www.w3.org/2000/svg" preserveaspectratio="none">
+ <svg class="divider--wavy" style="width:100%; height:100%" viewbox="0 0 100 10" xmlns="http://www.w3.org/2000/svg" preserveaspectratio="none">

Any percentage-based width or height should get converted from width/height attributes into style properties.

If a non-100% value is supplied, this wouldn't trigger any special AMP layout even for AMP elements. In such cases I think mapping to style properties should be done. So if width="50%" is present, it should get converted to style="width:50%" even for AMP elements, perhaps.

@csossi
Copy link

csossi commented Jan 31, 2020

Verified in QA

@csossi csossi added the QA passed Has passed QA and is done label Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working QA passed Has passed QA and is done Validation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants