Skip to content

Correctly handle siblings_disallowed from spec#7597

Merged
swissspidy merged 15 commits intodevelopfrom
fix/siblings-disallowed
Aug 3, 2023
Merged

Correctly handle siblings_disallowed from spec#7597
swissspidy merged 15 commits intodevelopfrom
fix/siblings-disallowed

Conversation

@swissspidy
Copy link
Collaborator

@swissspidy swissspidy commented Aug 3, 2023 •
edited
Loading

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

@swissspidy swissspidy added Bug Something isn't working Validation Sanitizers labels Aug 3, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2023 •
edited
Loading

Plugin builds for 7fca058 are ready 🛎️!

@swissspidy
Copy link
Collaborator Author

swissspidy commented Aug 3, 2023

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

thelovekesh commented Aug 3, 2023

@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

swissspidy 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?

@westonruter
Copy link
Member

westonruter commented Aug 3, 2023 •
edited
Loading

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
westonruter
westonruter reviewed Aug 3, 2023
View reviewed changes
westonruter
westonruter reviewed Aug 3, 2023
View reviewed changes
westonruter
westonruter reviewed Aug 3, 2023
View reviewed changes
westonruter
westonruter reviewed Aug 3, 2023
View reviewed changes
westonruter
westonruter reviewed Aug 3, 2023
View reviewed changes
westonruter
westonruter reviewed Aug 3, 2023
View reviewed changes
@swissspidy @westonruter
Fix condition
588e7ff 
Co-authored-by: Weston Ruter <[email protected]>
@swissspidy swissspidy requested a review from westonruter August 3, 2023 18:42
westonruter
westonruter reviewed Aug 3, 2023
View reviewed changes
@swissspidy @westonruter
Remove unnecessary check
7fca058 
Co-authored-by: Weston Ruter <[email protected]>
westonruter
westonruter approved these changes Aug 3, 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.

LGTM!

@swissspidy swissspidy merged commit 1aa9a63 into develop Aug 3, 2023
@swissspidy swissspidy deleted the fix/siblings-disallowed branch August 3, 2023 19:55
@swissspidy swissspidy mentioned this pull request Aug 7, 2023
Merged
@westonruter westonruter mentioned this pull request Aug 7, 2023
Closed
@westonruter westonruter mentioned this pull request Aug 17, 2023
Closed
4 tasks
@thelovekesh thelovekesh mentioned this pull request Oct 17, 2023
Merged
6 tasks
@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

@thelovekesh thelovekesh Awaiting requested review from thelovekesh

Assignees

No one assigned

Labels

Bug Something isn't working Changelogged Whether the issue/PR has been added to release notes. Sanitizers Validation

Projects

None yet

Milestone

v2.5.0

Development

Successfully merging this pull request may close these issues.

3 participants

@swissspidy @thelovekesh @westonruter