ampproject / amphtml

The AMP web component framework.
https://amp.dev
Apache License 2.0
14.89k stars 3.89k forks source link

amp-social-share Displaying incorrect SVG icons on iOS8 #4277

Closed jonnyasmar closed 7 years ago

jonnyasmar commented 8 years ago

When using the amp-social-share extension, every button is rendered exactly as the first defined amp-social-share element on the page (in my case, they all appear to be a facebook button).

Making any sort of change to the amp-social-share element using Safari's inspector, causes it to re-render with the correct image and background color.

This appears to be only affecting iOS 8 devices on both Safari & Chrome and running AMP version 1469143772973.

The issue should be reproduceable here: http://www.autoblog.com/amp/2016/07/29/toyota-prius-plug-in-lawsuit-claims-ev-range-false-advertising/

I've also attached a screenshot for reference.

screen shot 2016-07-29 at 2 45 44 pm

dvoytenko commented 8 years ago

/to @mkhatib

ericlindley-g commented 8 years ago

@mkhatib Put into "pending triage", but please update when you have a sense of when you can work on this. Thanks!

mkhatib commented 8 years ago

The browser doesn't seem to be triggering redraw properly. It's a bit weird because if you toggle any CSS properties in devtools it would redraw the backgrounds and icons correctly.

jridgewell commented 8 years ago

@mkhatib: Use transform: translateZ(0) on whatever is screwing up.

mkhatib commented 8 years ago

@jridgewell just tried it, didn't help. Created this test page - when viewed on iOS 8. It doesn't seem to draw correctly until a style is toggled.

mkhatib commented 8 years ago

Ok, one more weird observation, this seems to happen only when layout=responsive when I drop that attribute things seem to work normally.

mkhatib commented 8 years ago

Actually the transform hack works but not when applied directly in CSS. Is that the expectation?

When I triggered the transform property in JS it fixes this.

jridgewell commented 8 years ago

Looking into it now.

jridgewell commented 8 years ago

Maybe this has to do with the attribute selectors being incorrect?

/* bad */
element[attr=value] {}
/* good */
element[attr="value"] {}
mkhatib commented 8 years ago

Yup. That seems to fix it. Thanks for debugging! Sending a PR.

mkhatib commented 8 years ago

Sorry that was false positive. One experiment that worked is switching to using a class-based selector instead of tag[attr=value].

Alejo99 commented 7 years ago

The problem persist. Google Chrome v60.0.31112.107, Android 7.0.0, AMP version 1503591987101.

Some examples here: http://www.lecturas.com/actualidad/romantica-velada-bertin-osborne-fabiola-sevilla_28251/amp http://www.cuerpomente.com/alimentacion/intolerancias/que-es-intolerancia-lactosa-sintomas_280/amp https://elpais.com/ccaa/2017/08/28/catalunya/1503911879_087944.amp.html

Also, a screenshot: image

regards,

ericlindley-g commented 7 years ago

I can't repro on a more recent version of Nougat, 7.1.... Let us know if you upgrade your Android version, @Alejo99 and if that fixes the issue, so we can understand the scope of the problem). In the meantime /cc @aghassemi (and @cvializ while @aghassemi is out of pocket)

Alejo99 commented 7 years ago

Tried on iPhone 6 plus and Android v4.4.4. Can not reproduce the issue, it looks like Android is the culprit. Other Android versions might be affected (EDIT: reproduced on Android 6.0.1).

regards,

jridgewell commented 7 years ago

None of those pages caused an issue for me on a 6.0.1 phone (Chrome and "Internet" browsers)

Alejo99 commented 7 years ago

Hello,

Yesterday we did some tests because some of our websites had this issue while others didn't. We reproduced on 7.0.0 on this website: http://www.cuerpomente.com/blogs/come-limpio/8-recomendaciones-para-una-piel-perfecta-sin-necesidad-de-usar-productos-caros_664/amp.

One of the few differences we spotted was the logo on the top of the page, so we switched the SVG in the amp-img (inside nav#menu) for a PNG and now the social icons work fine. The issue can still be reproduced by changing the extension in the src attribute to SVG using remote debugging (https://developers.google.com/web/tools/chrome-devtools/remote-debugging/) and then scrolling the page. The icons should re-render and the facebook icon should turn into a twitter icon. Unfortunately I only have access to android 7.0.0 and 4.4.4, so it is possible that 6.0.1 version is working fine (user either lied or did not give us the correct version of his/her phone), I apologize for giving misleading information.

Moreover, in these other two websites the html markup is very similar (almost the same than http://www.cuerpomente.com/xxxxx/amp) and the logos are amp-img's with SVG as src. Surprisingly all social icons are ok: -http://www.mentesana.es/psicologia/desarrollo-personal/se-autentico-llegaras-mas-lejos_1244/amp -http://www.mundotubebe.com/crianza/nadie-pide-que-no-necesita_1139/amp

In this other website (http://www.lecturas.com/actualidad/jorge-miri-masterchef-esconden_28334/amp), the logo is an SVG html element instead of an amp-img. Removing the logo seems to fix the issue as well.

Hope to have helped!

regards,

jridgewell commented 7 years ago

Can you take a screencast of this? I'm not able to reproduce in 7.1.2 (the only 7 phone I have access to). Having a unrelated image (the logo) cause this seems spectacular.

ampprojectbot commented 7 years ago

This is a high priority issue but it hasn't been updated in awhile. @mkhatib Do you have any updates?

ampprojectbot commented 7 years ago

This issue doesn't have a category which makes it harder for us to keep track of it. @mkhatib Please add an appropriate category.

dvoytenko commented 7 years ago

AMP is not launched on iOS8. So should we close?

cvializ commented 7 years ago

Ali is OOO today but I vote we close since we no longer support iOS 8.

jridgewell commented 7 years ago

Wait, when did we drop iOS 8? We still have Viewport hacks for it.

dvoytenko commented 7 years ago

I think the more appropriate term "phasing out" :)