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

Version 2.0.2 broke my navigation menus because of CSS sanitizer #5401

Closed
Netzberufler opened this issue Sep 18, 2020 · 4 comments · Fixed by #5403
Closed

Version 2.0.2 broke my navigation menus because of CSS sanitizer #5401

Netzberufler opened this issue Sep 18, 2020 · 4 comments · Fixed by #5403
Labels
Bug Something isn't working Changelogged Whether the issue/PR has been added to release notes. CSS P0 High priority WS:Core Work stream for Plugin core
Milestone

Comments

@Netzberufler
Copy link

Bug Description

I'm a theme developer and have added AMP support to several of my themes. I'm using tap:AMP.setState() to set a toggled-on class for multiple mobile dropdown menus and dropdown search fields.

Example CSS:

.main-navigation ul.menu {
	display: none;
}

.main-navigation.toggled-on > ul.menu {
	display: block;
}

My issue is that the .main-navigation.toggled-on class is missing in the AMP CSS since version 2.0.2 and seems to be removed by the Sanitizer. I guess that happens because the toggled-on class is not actually used by default in the HTML markup, only interactively added later.

Because of the missing class, mobile menus are broken with AMP active.

Expected Behaviour

I would like to manually control the CSS sanitizer so my toggled-on class is included in the AMP CSS. I did not find any filter or action hook, but maybe I missed it.

Steps to reproduce

Example of the implementation used:
https://github.com/ThemeZee/harrison/blob/master/template-parts/header/site-navigation.php
https://github.com/ThemeZee/harrison/blob/master/inc/addons.php#L93

Theme Demo with AMP active: https://preview.themezee.com/harrison/
When inspecting, you can see the toggled-on class is added, but has no effect because it is missing in the CSS.

Additional context

  • WordPress version: 5.5.1
  • Plugin version: 2.0.2
  • AMP plugin template mode: Standard / Traditional

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

Acceptance criteria

Implementation brief

QA testing instructions

Demo

Changelog entry

@Netzberufler
Copy link
Author

Btw not sure if this is really a bug, or if I'm doing things wrong. Any input on how I can solve that is highly appreciated :)

@westonruter
Copy link
Member

@Netzberufler Thank you for reporting! You have correctly identified a bug. I've created a fix here: #5403. Can you test to confirm it fixes the issue?

@westonruter westonruter added Bug Something isn't working CSS P0 High priority labels Sep 18, 2020
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Sep 18, 2020
@Netzberufler
Copy link
Author

@westonruter Yes everything works fine now. Thanks so much for fixing this so quickly

@westonruter
Copy link
Member

Excellent. I also just sent you a follow-up email.

@westonruter westonruter added the WS:Core Work stream for Plugin core label Oct 7, 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 Changelogged Whether the issue/PR has been added to release notes. CSS P0 High priority WS:Core Work stream for Plugin core
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants