Amphtml: amp-geo appends classes to wrong <body> in PWA

Created on 20 Feb 2019  路  12Comments  路  Source: ampproject/amphtml

What's the issue?

When an AMP page utilizing amp-geo is loaded into a PWA, the classes created by amp-geo (e.g. amp-geo-group-____ and amp-iso-country-__) are appended to the appshell body instead of the AMP document body. Since styles are encapsulated in a shadow document, these classes are unavailable for styling elements in the AMP document.

How do we reproduce the issue?

I have created a minimal example that demonstrates the issue. The AMP document has styles defined to show the message _This message is visible if any amp-geo class is added to the body._ if any amp-geo class is attached to the body. If no amp-geo class is attached, then the message _No amp-geo class is attached to the body._ appears.

The AMP page shows _This message is visible if any amp-geo class is added to the body._
The AMP page viewed in the PWA shows _No amp-geo class is attached to the body._

What browsers are affected?

The following browsers have been tested and both exhibit the same behavior:

  • Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/72.0.3626.109 Safari/537.36
  • Mozilla/5.0 (iPhone; CPU iPhone OS 11_3 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/11.0 Mobile/15E148 Safari/604.1

Which AMP version is affected?

1902081532110

Soon Bug
Source
picture mattwomple

Most helpful comment

FYI @torch2424 the primary owner of amp-geo is now @zhouyx

picture jpettitt on 22 Feb 2019
🎉2 😄1 👍1

All 12 comments

cc @jpettitt @zhouyx @prateekbh ? Don't quite know who would be the owner on this, my apologies.

torch2424 picture torch2424 on 22 Feb 2019

I can take this. can reproduce, will take a look soon.

zhouyx picture zhouyx on 22 Feb 2019

FYI @torch2424 the primary owner of amp-geo is now @zhouyx

jpettitt picture jpettitt on 22 Feb 2019
🎉2 😄1 👍1

@zhouyx thoughts/questions:

  1. Should the class be added to all body elements? (appshell and AMP document?)
  2. Are there other extensions that add classes or attributes (amp-experiment comes to mind ?) that might have similar issues?
jpettitt picture jpettitt on 22 Feb 2019

Thank you @jpettitt. First thought I believe the class should be added to only the AMP document as amp-geo itself is a document level service. And as @mattwomple pointed out styles are encapsulated in a shadow document.
It's a great point that amp-experiment is another similar extension that applies class to body, I'll definitely see what can be done there as well.

zhouyx picture zhouyx on 22 Feb 2019
👍1

Thanks @jpettitt 馃槃

torch2424 picture torch2424 on 22 Feb 2019

Found why, amp-geo tries to add classList to the window.document instead of the ampdoc.rootNode. I'll work on the fix next week.

I've also tested the amp-experiment behavior. It is working as expected and I can verify amp-experiment is apply change to the ampdoc.rootnode, which points to the shadowRoot or document in different environment.

zhouyx picture zhouyx on 23 Feb 2019

@zhouyx @jpettitt If one or both of you would be willing to review #21282 it would be appreciated. Thanks.

mattwomple picture mattwomple on 6 Mar 2019

Done, I'll merge it once the tests complete.

jpettitt picture jpettitt on 6 Mar 2019

Thank you for the quick attention and review @jpettitt

mattwomple picture mattwomple on 6 Mar 2019

Fix is merged, it should hit the canary/dev channel build next week and prod the week after. @mattwomple please check it when it hits canary (typically late Tuesday California time)

jpettitt picture jpettitt on 6 Mar 2019
👍1

@jpettitt I tested build 1903080058260 today and this change is working as expected. Thanks.

mattwomple picture mattwomple on 11 Mar 2019
Was this page helpful?
0 / 5 - 0 ratings

Related issues

akshaylive picture akshaylive  路  3Comments
westonruter picture westonruter  路  3Comments
aghassemi picture aghassemi  路  3Comments
devanes picture devanes  路  3Comments
edhollinghurst picture edhollinghurst  路  3Comments