-
Notifications
You must be signed in to change notification settings - Fork 384
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
Entire picture
element tree is removed when img
child is expected to persist
#6676
Comments
Steps for QA
<div>
Image 1
<picture><source srcset="https://interactive-examples.mdn.mozilla.net/media/cc0-images/surfer-240-200.jpg" media="(min-width: 800px)"><img src="https://interactive-examples.mdn.mozilla.net/media/cc0-images/painted-hand-298-332.jpg?image=1" alt="">
</picture>
Image 2
<picture><source srcset="https://interactive-examples.mdn.mozilla.net/media/cc0-images/surfer-240-200.jpg" media="(min-width: 800px)"><img src="https://interactive-examples.mdn.mozilla.net/media/cc0-images/surfer-240-200.jpg?image=2" alt="">
</picture>
</div>
----
<div>
Image 3
<picture><source srcset="https://interactive-examples.mdn.mozilla.net/media/cc0-images/surfer-240-200.jpg" media="(min-width: 800px)"><img src="https://interactive-examples.mdn.mozilla.net/media/cc0-images/painted-hand-298-332.jpg??image=3" alt="">
</picture>
</div>
----
<div>
Picture without img tag
<picture><source srcset="https://interactive-examples.mdn.mozilla.net/media/cc0-images/surfer-240-200.jpg?image=3" media="(min-width: 800px)"></picture>
</div>
<div>
<p>Image 1</p>
<img src="[https://interactive-examples.mdn.mozilla.net/media/cc0-images/painted-hand-298-332.jpg?image=1](view-source:https://interactive-examples.mdn.mozilla.net/media/cc0-images/painted-hand-298-332.jpg?image=1)" alt="" width="[298]()" height="[332]()" class="[amp-wp-enforced-sizes]()" decoding="[async]()">Image 2
<img src="[https://interactive-examples.mdn.mozilla.net/media/cc0-images/surfer-240-200.jpg?image=2](view-source:https://interactive-examples.mdn.mozilla.net/media/cc0-images/surfer-240-200.jpg?image=2)" alt="" width="[240]()" height="[200]()" class="[amp-wp-enforced-sizes]()" decoding="[async]()"></div>
<p>—-</p>
<div>
Image 3
<img src="[https://interactive-examples.mdn.mozilla.net/media/cc0-images/painted-hand-298-332.jpg??image=3](view-source:https://interactive-examples.mdn.mozilla.net/media/cc0-images/painted-hand-298-332.jpg??image=3)" alt="" width="[298]()" height="[332]()" class="[amp-wp-enforced-sizes]()" decoding="[async]()"></div>
<p>—-</p>
<div>
<p>Picture without img tag</p>
</div> |
@pooja-muchandikar It seems you're testing on the |
Hi @westonruter, Sorry for the confusion. I did check the issue with release branch that is |
Bug Description
In #1316 we were originally looking to support the
picture
element, but we ended up closing it. We should at least ensure that theimg
child of apicture
element is able to replace thepicture
element. Instead, as it stands right now, when the following markup as input:The result is the entire tree is removed, unexpectedly.
Expected Behaviour
Instead it is is expected that the parent
picture
would be removed only, with theimg
left so that it is then converted into anamp-img
like so:Screenshots
No response
PHP Version
No response
Plugin Version
2.2
AMP plugin template mode
Standard, Transitional, Reader
WordPress Version
No response
Site Health
No response
Gutenberg Version
No response
OS(s) Affected
No response
Browser(s) Affected
No response
Device(s) Affected
No response
Acceptance Criteria
No response
Implementation Brief
Update
AMP_Img_Sanitizer
so that if thenative_img_used
arg is false, it:sanitize
method, it loops over allpicture
elements.picture
, it finds the childimg
and replaces thepicture
with theimg
, deleting anysource
elements in the process.Otherwise, if
native_img_used
is true, then at the beginning of thesanitize
method, loop over allpicture
elements and pass each intoValidationExemption::mark_node_as_px_verified()
, along with the childsource
andimg
elements.QA Testing Instructions
No response
Demo
No response
Changelog Entry
No response
The text was updated successfully, but these errors were encountered: