ampproject / samples

Apache License 2.0
444 stars 191 forks source link

Click handler doesn't work properly in Safari #92

Open dominikbulaj opened 6 years ago

dominikbulaj commented 6 years ago

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 commented 6 years ago

@cvializ - can you take a look?

cvializ commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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

dominikbulaj commented 6 years ago

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