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

Correctly handle siblings_disallowed from spec #7597

Merged
merged 15 commits into from
Aug 3, 2023

Conversation

swissspidy
Copy link
Collaborator

@swissspidy swissspidy commented Aug 3, 2023

Summary

Elements like <amp-story> use siblings_disallowed: true to ensure they don't have any sibling elements.

https://github.com/ampproject/amphtml/blob/db9d5dbd9a1b5e21eba496e791829308dbb19c7e/extensions/amp-story/validator-amp-story.protoascii#L104

Due to a bug report for the Web Stories plugin we noticed that the existing sanitizers were not removing any disallowed siblings of <amp-story>

Turns out, this information was not being used at all in bin/amphtml-update.py and AMP_Tag_And_Attribute_Sanitizer.

This PR is an attempt at fixing that.

I had to tweak the Python script a bit to get the generation working again, as Bento has now been removed and also amp-redbull-player was deprecated.

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

@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2023

Plugin builds for 7fca058 are ready 🛎️!

@swissspidy
Copy link
Collaborator Author

No idea why those amp-facebook tests are failing 🤷

The Bento tests and the ones for amp-redbull-player can be removed I suppose.

Any help appreciated.

@thelovekesh
Copy link
Collaborator

@swissspidy I think failing tests can be the result of the removal of bento extension from allowed tags. When the expected string is bento optimized (or bento component markup) but bento is not actually present in the allowed tags and attributes, the results of a few tests may differ.

We can re-generate includes/sanitizers/class-amp-allowed-tags-generated.php by not removing bento extensions from bin/amphtml-update.py to check if tests are passing?

@swissspidy
Copy link
Collaborator Author

Hmm I guess, but that's tricky unless I do it manually, because bundles.config.bento.json no longer exists.

Maybe now's simply the time to remove Bento here?

@westonruter
Copy link
Member

westonruter commented Aug 3, 2023

Hmm I guess, but that's tricky unless I do it manually, because bundles.config.bento.json no longer exists.

Maybe now's simply the time to remove Bento here?

Yes, that should be done. However, that would probably add too much scope to this PR. How about reverting the changes to Bento in bin/amphtml-update.py and includes/sanitizers/class-amp-allowed-tags-generated.php (e.g. via git checkout -p develop -- bin/amphtml-update.py includes/sanitizers/class-amp-allowed-tags-generated.php)?

Then we can update the spec generator and remove Bento in another PR.

@westonruter westonruter added this to the v2.4.3 milestone Aug 3, 2023
swissspidy and others added 3 commits August 3, 2023 20:06
Co-authored-by: Weston Ruter <westonruter@google.com>
Co-authored-by: Weston Ruter <westonruter@google.com>
Co-authored-by: Weston Ruter <westonruter@google.com>
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.

LGTM!

@swissspidy swissspidy merged commit 1aa9a63 into develop Aug 3, 2023
@swissspidy swissspidy deleted the fix/siblings-disallowed branch August 3, 2023 19:55
@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
Labels
Bug Something isn't working Changelogged Whether the issue/PR has been added to release notes. Sanitizers Validation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants