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

Click handler doesn't work properly in Safari #92

Open
dominikbulaj opened this issue Dec 13, 2017 · 5 comments
Open

Click handler doesn't work properly in Safari #92

dominikbulaj opened this issue Dec 13, 2017 · 5 comments

Comments

@dominikbulaj
Copy link

dominikbulaj commented Dec 13, 2017

Hi, I'm developer working for NetMoms.de and we create new PWA app using AMP for WordPress contents (AMP in PWA pattern). Since we're SPA (build in ReactJS) we depend on amp-document.

We use amp-document's click listener to load other AMP pages without escaping from PWA (basically we check clicked link's URL and take WP post slug from it and push it to React Router's history - so PWA URL changes, new AMP document is being requested but we're still inside SPA/PWA).

However we've noticed some issues with Safari (iOS as well on macOS).

It looks like click listener doesn't see a anything from ShadowDOM.
It receives event which target is shadow root and not exactly clicked element. Since Safari doesn't support event.path you use pollyfill https://github.com/ampproject/amp-publisher-sample/blob/master/amp-pwa/src/components/amp-document/amp-document.js#L208 while debugging inside while-statement it confirmed that event.target is shadow root element (I clicked img element inside figure, article, a (and other parents) but in Safari I only see nodes from shadowRoot up to html root (in fact 6x div, body and html).

amp-document-1

ShadowDOM in our case is open as I've commented out closing it in https://github.com/ampproject/amp-publisher-sample/blob/master/amp-pwa/src/components/amp-document/amp-document.js#L222 due to some other requirements.

Tested on: Safari v11.0.2 & macOS High Sierra 10.13.2 but it also happens in iOS devices.

@sebastianbenz
Copy link
Collaborator

sebastianbenz commented Dec 14, 2017

@cvializ - can you take a look?

@cvializ
Copy link

cvializ commented Dec 14, 2017

@dominikbulaj Hi, thanks for reporting this! Can you help me reproduce and post a link to your AMP PWA or a minimal repro case?

@dominikbulaj
Copy link
Author

dominikbulaj commented Dec 15, 2017

@cvializ Hi, yes of course.
Here you got one link that is our entry point (has lot of teasers that aren't correctly linking in Safari) https://pwa.netmoms.de/cms/baby
Unfortunately you need to be signed into account to access it.

Recently we had some talks with Martin Schierle from Google (Munich) and he suggested we can try
document.getElementsByClassName('amp-container'[0].children[0].shadowRoot.elementFromPoint()

I've implemented it and added following code just after event.path polyfill (actually it is modified version of this polyfill):

if (navigator.userAgent.includes('Safari')) {
  let node = document.getElementsByClassName('amp-container')[0].children[0].shadowRoot.elementFromPoint(e.clientX, e.clientY)
  while (node && node.tagName !== 'A') {
    node = node.parentNode
  }
  a = node
}

This is not the greatest solution out there but at least it works.

Hope it helps.

@roelvanhoof
Copy link

@dominikbulaj Are you still using this solution? I tested this and this doesn't seem to work on (older) iphones.

@dominikbulaj
Copy link
Author

@roelvanhoof yes but we don't support too old devices.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants