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.

Sign up for GitHub

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

Jump to bottom

Update spec to include img in allowed-descendants for amp-mega-menu #7652

Merged
merged 2 commits into from
Nov 1, 2023
Merged

Update spec to include img in allowed-descendants for amp-mega-menu #7652

merged 2 commits into from
Nov 1, 2023

Conversation

thelovekesh
Copy link
Collaborator

@thelovekesh thelovekesh commented Nov 1, 2023 •
edited
Loading

Summary

Previously #7615
Fixes #7029

Checklist

  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

thelovekesh
thelovekesh commented Nov 1, 2023
View reviewed changes
bin/amphtml-update.py Outdated
Comment on lines 425 to 431
# Skip tags specific to transformed AMP.
if 'I-AMPHTML-SIZER' == val:
continue

# The img tag is currently exclusively to transformed AMP, except as descendant of amp-story-player.
if 'IMG' == val and 'amp-story-player-allowed-descendants' != _list.name:
continue
Copy link
Collaborator Author

@thelovekesh thelovekesh Nov 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Considering these two points:

  • native img is supported by AMPHTML now.
  • i-amphtml-sizer can be present in SSR-transformed HTML.

Copy link
Member

Choose a reason for hiding this comment

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

  • i-amphtml-sizer can be present in SSR-transformed HTML.

Yes, but the AMP plugin only takes non-transformed AMP as input. So our spec should be devoid of any SSR-specific elements. In the past, this included img. However, now that img is allowed in AMP instead of amp-img, it should no longer be skipped.

So yeah, keep the i-amphtml-sizer skip logic in place.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 1, 2023 •
edited
Loading

Plugin builds for 6fbbf16 are ready 🛎️!

Checksums 
# Development build checksums
54b9e7c309228d101bab57e3edd91f660b42e2cec397d25f4b8888c193a566d2 *amp.zip

# Production build checksums
d0685e56e25e34f6c61ea8061a8de54c7a631e04c9983ff4181254c00e58c12d *amp.zip

Warning

These builds are for testing purposes only and should not be used in production.

@westonruter westonruter added this to the v2.5.0 milestone Nov 1, 2023
westonruter
westonruter requested changes Nov 1, 2023
View reviewed changes
Copy link
Member

@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.

See #7652 (comment)

@thelovekesh thelovekesh force-pushed the update/amp-mega-menu-allowed-descendants branch from 8d162af to 6a7fccb Compare November 1, 2023 17:55
westonruter
westonruter reviewed Nov 1, 2023
View reviewed changes
includes/sanitizers/class-amp-allowed-tags-generated.php Outdated
@@ -527,6 +530,7 @@ class AMP_Allowed_Tags_Generated {
'hr',
'i',
'image',
'img',
Copy link
Member

Choose a reason for hiding this comment

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

I just tried adding an img to an amp-story-page-attachment and I get a validation error. See playground.

It looks like perhaps the AMP Story components haven't been updated to support native images?

Copy link
Member

Choose a reason for hiding this comment

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

See spec for amp-story-page-attachment: https://github.com/ampproject/amphtml/blob/4ce3cd79520dbeaf5ed5364cbff58d3d71dee40e/extensions/amp-story/validator-amp-story.protoascii#L1056-L1256

westonruter
westonruter reviewed Nov 1, 2023
View reviewed changes
includes/sanitizers/class-amp-allowed-tags-generated.php Outdated
@@ -194,6 +195,7 @@ class AMP_Allowed_Tags_Generated {
'hkern',
'hr',
'i',
'img',
Copy link
Member

Choose a reason for hiding this comment

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

Also apparently not allowed in AMP Stories, see https://github.com/ampproject/amphtml/blob/4ce3cd79520dbeaf5ed5364cbff58d3d71dee40e/extensions/amp-story/validator-amp-story.protoascii#L598-L711

westonruter
westonruter reviewed Nov 1, 2023
View reviewed changes
includes/sanitizers/class-amp-allowed-tags-generated.php Outdated
@@ -339,6 +341,7 @@ class AMP_Allowed_Tags_Generated {
'hr',
'i',
'image',
'img',
Copy link
Member

Choose a reason for hiding this comment

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

Also apparently not allowed yet: https://github.com/ampproject/amphtml/blob/4ce3cd79520dbeaf5ed5364cbff58d3d71dee40e/extensions/amp-story/validator-amp-story.protoascii#L712-L868

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Humm seems like it's only allowed for transformed AMP in most of the extensions. In that case, I have to add a condition to only add img to allowed extensions.

@thelovekesh thelovekesh force-pushed the update/amp-mega-menu-allowed-descendants branch from 6a7fccb to 6fbbf16 Compare November 1, 2023 18:50
thelovekesh
thelovekesh commented Nov 1, 2023
View reviewed changes
bin/amphtml-update.py
@@ -426,8 +432,8 @@ def ParseRules(repo_directory, out_dir):
if 'I-AMPHTML-SIZER' == val:
continue

# The img tag is currently exclusively to transformed AMP, except as descendant of amp-story-player.
if 'IMG' == val and 'amp-story-player-allowed-descendants' != _list.name:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

amp-story-player-allowed-descendants was allowed to keep native img but spec adds it for transformed AMP 🤔

https://github.com/ampproject/amphtml/blob/4ce3cd79520dbeaf5ed5364cbff58d3d71dee40e/extensions/amp-story-player/validator-amp-story-player.protoascii#L32

thelovekesh
thelovekesh commented Nov 1, 2023
View reviewed changes
bin/amphtml-update.py
Comment on lines +365 to +368
allow_img_as_descendant = [
'amp-story-player-allowed-descendants',
'amp-mega-menu-allowed-descendants',
]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Currently, allowing these to keep native img.

@thelovekesh thelovekesh changed the title Update spec to include img and i-amphtml-sizer in allowed-descendants Update spec to include img in allowed-descendants for amp-mega-menu Nov 1, 2023
@westonruter westonruter merged commit c831a3e into develop Nov 1, 2023
@westonruter westonruter deleted the update/amp-mega-menu-allowed-descendants branch November 1, 2023 20:07
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@westonruter westonruter westonruter approved these changes

Assignees
No one assigned
Labels
Changelogged Whether the issue/PR has been added to release notes.
Projects
None yet
Milestone
v2.5.0
Development

Successfully merging this pull request may close these issues.

Skip rendering child amp-img tags of amp-mega-menu
2 participants
@thelovekesh @westonruter