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

data-amp-no-unwrap effectiveness restricted to <body> element, disregarded in <head> #7234

Open
anrghg opened this issue Aug 30, 2022 · 7 comments
Labels
Bug Something isn't working P2 Low priority

Comments

@anrghg
Copy link

anrghg commented Aug 30, 2022

Bug Description

<noscript> elements with the data-amp-no-unwrap property in the <head> are unwrapped regardless.

Expected Behaviour

<noscript data-amp-no-unwrap><style> /* This CSS applies only if JS is off. */ </style></noscript>

Screenshots

In the body, there is this test code:

<noscript data-amp-no-unwrap>JavaScript is off.</noscript><br />
<noscript>This page is served as AMP if this message shows up while JavaScript is on.</noscript>

On a page served as AMP: https://anrghg.sunsite.fr/test-amp-compat/features/helpers/#127-amp-compatibility
On a non-AMP page: https://anrghg.sunsite.fr/publishing-helper/features/helpers/#127-amp-compatibility
(before the next heading).

In the head, there is this selector on both pages:

.anrghg-display-toggle:not(:checked)+.anrghg-tocontents .anrghg-contents-list .anrghg-contents-heading:target

On the non-AMP page, it is wrapped into <noscript data-amp-no-unwrap>.
On the AMP page, it survives tree-shaking and is in the valid CSS because it is unwrapped.

The adverse effect on the AMP is that a TOC item shows up below the TOC label after clicking a heading number, then reloading the page, while JS is on.

PHP Version

8.1

Plugin Version

2.3.0

AMP plugin template mode

Standard

WordPress Version

6.0.1

Site Health

https://anrghg.sunsite.fr/test-amp-compat/wp-content/uploads/sites/3/2022/08/site-health_2022-08-30T22500200.txt

Gutenberg Version

OS(s) Affected

Linux

Browser(s) Affected

Brave

Device(s) Affected

desktop

Acceptance Criteria

No response

Implementation Brief

No response

QA Testing Instructions

No response

Demo

No response

Changelog Entry

No response

@anrghg anrghg added the Bug Something isn't working label Aug 30, 2022
@westonruter
Copy link
Member

I believe this would be addressed by implementing support for head > noscript > style[amp-noscript]. See #6603.

@anrghg
Copy link
Author

anrghg commented Aug 31, 2022

@westonruter Thank you for opening and reopening #6603.

At my end I’ve moved the style amp-noscript to the body, still in a noscript data-amp-no-unwrap, but it gets processed despite its mandatory_ancestor is head. (So an empty noscript data-amp-no-unwrap is left over.)

I’ve vainly tried to set up a playground for the topic.

@westonruter
Copy link
Member

The issue is that the style sanitizer goes through and gathers up all styles and moves them all into the one style[amp-custom]. So what we need to do is keep track of the fact that the style was inside of a noscript and then put it into a head > style[amp-noscript] element when putting the styles back into the document.

I’ve vainly tried to set up a playground for the topic.

Note that the data-amp-no-unwrap attribute is specific to the AMP plugin for WordPress. It's not understood by the core AMP library and validator.

@anrghg
Copy link
Author

anrghg commented Aug 31, 2022

@westonruter Thank you for the information!

@anrghg
Copy link
Author

anrghg commented Sep 1, 2022

@westonruter Although the cited edge case prompted me to add a paragraph (“There is a small likelihood for the same to happen while JavaScript is on. On AMP pages, the related style rule is always valid, but after clicking a heading number (which is when it would come into play) it is overridden by another one, activated using the AMP framework. Reloading the page in that state would result in a lone heading as pointed in the question.”) to https://wordpress.org/plugins/anrghg/#what%20is%20the%20lone%20heading%20after%20the%20table%20of%20contents%20label%3F — it is overshadowed by a bug in scroll offset affecting AMP-HTML, reported in ampproject/amphtml#38423. On desktop, that bug is IMO an absolute deal breaker as stated in https://anrghg.sunsite.fr/publishing-helper/features/helpers/#127-amp-compatibility-is-tested-and-showcased-on-a-dedicated-site-based-on-the

Both the scroll offset bug and the noscript style issue are overwhelming me just as I need to pause the plugin project while resuming my main project due since the nineties, and now in october/november 2022.

@anrghg
Copy link
Author

anrghg commented Sep 1, 2022

@westonruter The scroll offset issue has already been fixed! ampproject/amphtml#35366

The only thing we need to know is that with AMP restricting support to scroll-padding-*, scroll-margin-* is de facto deprecated. ampproject/amphtml#34378 (comment)

@anrghg
Copy link
Author

anrghg commented Sep 2, 2022

@westonruter I’ve looked into includes/sanitizers/class-amp-style-sanitizer.php trying to implement the fix you pointed out.

It seems tricky and would require reengineering the parsing as it should register what parent the style element is in, and sort rules into two groups.

Alongside there is a new issue #7238, while in the cited use case, the present issue is rather an insignificant edge case. Probably not on other websites though.

@westonruter westonruter added the P2 Low priority label Nov 30, 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 P2 Low priority
Projects
None yet
Development

No branches or pull requests

2 participants