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

www.google.com - Missing "Email" icon when sharing an AMP document #8733

Closed
softvision-oana-arbuzov opened this issue Aug 7, 2017 · 15 comments
Labels
browser-firefox-mobile engine-gecko The browser uses the Gecko rendering engine sci-exclude Bugs to exclude from out Top Site Compat Index metrics severity-minor The site has a cosmetic issue. type-GWS-interop Google properties interoperability testing type-GWS-interop-tracked Google properties interoperability testing type-uaoverride Require a UA override for working
Milestone

Comments

@softvision-oana-arbuzov
Copy link
Member

softvision-oana-arbuzov commented Aug 7, 2017

URL: https://www.google.com/amp/s/www.forbes.com/sites/insertcoin/2017/08/07/game-of-thrones-did-two-very-smart-things-in-last-nights-fantastic-episode/amp/

Browser / Version: Firefox Mobile Nightly 57.0a1 (2017-08-07) - Chrome UA
Operating System: LG G5 (Android 7.0) - Resolution 1440 x 2560 pixels (~554 ppi pixel density)
Tested Another Browser: Yes

Problem type: Design is broken
Description: Missing email icon when sharing an AMP document

Steps to Reproduce:

  1. Navigate to: https://www.google.com/amp/s/www.forbes.com/sites/insertcoin/2017/08/07/game-of-thrones-did-two-very-smart-things-in-last-nights-fantastic-episode/amp/.
  2. Tap the “Share” button.
  3. Observe first icon.

Expected Behavior:
Email icon is displayed.

Actual Behavior:
A black icon is displayed.

Note:

  1. Not reproducible on Chrome (Mobile) 59.0.3071.125, on Firefox 54.0.1 Release and Firefox Nightly 57.0a1 (2017-08-02) without Chrome UA.
  2. Screenshot attached.

Watchers:
@softvision-sergiulogigan
@softvision-oana-arbuzov

sv; gs

Screenshot Description

From webcompat.com with ❤️

@softvision-oana-arbuzov softvision-oana-arbuzov changed the title www.google.com - design is broken www.google.com - Missing "Email" icon when sharing an AMP document Aug 7, 2017
@wellington1993
Copy link

Hi,

working fine at Desktop - WIN10 +Firefox 57
Anyone can reproduce the problem?

Thanks...

@MDTsai
Copy link

MDTsai commented Aug 8, 2017

Confirmed, the email link rule is:

.email-share-link {
	background-image: url('data: image/svg+xml;charset=utf8,%3Csvg%20xmlns%3D%22http%3A%2F%2Fwww.w3.org%2F2000%2Fsvg%22%20viewBox%3D%220%200%20512%20512%22%3E%3Cpath%20fill%3D%22%23ffffff%22%20d%3D%22M101.3%20141.6v228.9h0.3%20308.4%200.8V141.6H101.3zM375.7%20167.8l-119.7%2091.5%20-119.6-91.5H375.7zM127.6%20194.1l64.1%2049.1%20-64.1%2064.1V194.1zM127.8%20344.2l84.9-84.9%2043.2%2033.1%2043-32.9%2084.7%2084.7L127.8%20344.2%20127.8%20344.2zM384.4%20307.8l-64.4-64.4%2064.4-49.3V307.8z%22%2F%3E%3C%2Fsvg%3E');
	background-color: #000000;
	background-repeat: no-repeat;
	background-position: center;
	background-size: contain;
	text-decoration: none;
	cursor: pointer;
}

Not the hash sign in data url problem. Move to needsdiagnosis.

@dtapuska
Copy link

I cannot reproduce on Firefox 54.0.1 on a Nexus 5 device.

@kereliuk
Copy link

I was able to reproduce this on mobile:

  • Download firefox nightly and enable usb debugging
  • Enable usb debugging on your device
  • Download "phony" add-on in firefox nightly to change UA
  • Configure "phony" to be android

@adamopenweb
Copy link
Collaborator

adamopenweb commented Aug 10, 2017

So I can reproduce this issue as well. But I think it might be out of scope. The issue actually happens with the Forbes code, but only with the UA spoofed to Chrome. They are actually serving Firefox Android an icon vs. the svg Chrome gets, it's even visually different.

It appears Forbes is working around a bug though. If we take the inline SVG data URI and dump into a JSFiddle, the SVG will not display in Firefox but it does in Chrome. https://jsfiddle.net/1yqafsxL/1/

If we decode the URI:

%3Csvg%20xmlns%3D%22http%3A%2F%2Fwww.w3.org%2F2000%2Fsvg%22%20viewBox%3D%220%200%20512%20512%22%3E%3Cpath%20fill%3D%22%23ffffff%22%20d%3D%22M101.3%20141.6v228.9h0.3%20308.4%200.8V141.6H101.3zM375.7%20167.8l-119.7%2091.5%20-119.6-91.5H375.7zM127.6%20194.1l64.1%2049.1%20-64.1%2064.1V194.1zM127.8%20344.2l84.9-84.9%2043.2%2033.1%2043-32.9%2084.7%2084.7L127.8%20344.2%20127.8%20344.2zM384.4%20307.8l-64.4-64.4%2064.4-49.3V307.8z%22%2F%3E%3C%2Fsvg%3E

We get this, which will work as is in Firefox:

<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 512 512"><path fill="#ffffff" d="M101.3 141.6v228.9h0.3 308.4 0.8V141.6H101.3zM375.7 167.8l-119.7 91.5 -119.6-91.5H375.7zM127.6 194.1l64.1 49.1 -64.1 64.1V194.1zM127.8 344.2l84.9-84.9 43.2 33.1 43-32.9 84.7 84.7L127.8 344.2 127.8 344.2zM384.4 307.8l-64.4-64.4 64.4-49.3V307.8z"/></svg>

This does have a hash sign in there. Though removing it, replacing with white and encoding again doesn't fix our problem. I tried as base 64 as well, but it doesn't work.

@dholbert
Copy link

This is due to the space after "data: ". It loads just fine if I remove that space character.

If possible, we should do some outreach to ask if they can remove that. But in the meantime I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1389251 since it seems other browsers disagree with us on that handling (though I think we're more data-URI rfc/spec-compliant on this point, FWIW).

@adamopenweb
Copy link
Collaborator

Thank you @dholbert! We were actually just doing testing for Google Search in Firefox Android with a Chrome UA. The Forbes site is actually working OK with Firefox Android I think.

Let's close this report as invalid for now. If someone disagrees please open back up.

@karlcow karlcow added the type-uaoverride Require a UA override for working label Aug 15, 2017
@miketaylr miketaylr added type-GWS-interop Google properties interoperability testing type-GWS-interop-tracked Google properties interoperability testing labels Oct 25, 2017
@karlcow karlcow added this to the invalid milestone Oct 30, 2017
@miketaylr miketaylr modified the milestones: invalid, contactready Nov 1, 2017
@miketaylr
Copy link
Member

I don't think closed invalid is correct here. We're spoofing as Chrome, but we want to get served this content in the future. So let's move to contactready, but not reach out to google about this just yet (besides, they're tracking this as well. :))

@miketaylr miketaylr reopened this Nov 1, 2017
@foolip
Copy link
Member

foolip commented Jan 19, 2018

Something doesn't add up with this issue. In whatwg/fetch#579 and its tests we've found that a space like "data: image/svg,..." is handled by all browsers, see whatwg/fetch#579 (comment) for examples from the wild and a minified one.

https://bugzilla.mozilla.org/show_bug.cgi?id=1389251 is still open and I've tested https://software.hixie.ch/utilities/js/live-dom-viewer/saved/5761 in Firefox 32 (works there) so this is not a recent change.

Was spaces in the URL really the problem here?

@dholbert
Copy link

its tests we've found that a space like "data: image/svg,..." is handled by all browsers

Only when ";base64" is present, I think, right? I'm guessing that triggers a different codepath / mimetype-handling somethingsomething.

@dholbert
Copy link

dholbert commented Jan 20, 2018

(See https://bugzilla.mozilla.org/show_bug.cgi?id=1389251#c19 and my just-attached "testcase 3" over there, which has a data: image/svg+xml,... that is definitely still broken in Firefox. I think @foolip's observed-to-be-working data-URIs-with-spaces all have ;base64, and that must trigger a different parsing codepath that is more space-friendly (even for earlier spaces), or something. I think that answers your question, @foolip, but let me know if I'm misunderstanding.)

@foolip
Copy link
Member

foolip commented Jan 24, 2018

Aha, thanks @dholbert! https://software.hixie.ch/utilities/js/live-dom-viewer/saved/5767 confirms that there is a difference, add a space there and it doesn't work in Firefox, but it does work in Chrome.

@adamopenweb adamopenweb added the severity-minor The site has a cosmetic issue. label Apr 7, 2018
@adamopenweb
Copy link
Collaborator

Tested this issue again before the fix lands for https://bugzilla.mozilla.org/show_bug.cgi?id=1389251. Issue still reproduces as before. Will check back in a couple days.

@miketaylr miketaylr added the engine-gecko The browser uses the Gecko rendering engine label Apr 30, 2019
@miketaylr miketaylr added the sci-exclude Bugs to exclude from out Top Site Compat Index metrics label Jun 28, 2019
@karlcow
Copy link
Member

karlcow commented Dec 11, 2019

The layout has changed.

@karlcow karlcow closed this as completed Dec 11, 2019
@karlcow karlcow modified the milestones: contactready, invalid Dec 11, 2019
@softvision-oana-arbuzov
Copy link
Member Author

Indeed, I can't reproduce it either.
image

Tested with:
Browser / Version: Firefox Nightly 68.4a1 (2019-12-16)
Operating System: Huawei P20 Lite (Android 8.0.0) - 1080 x 2280 pixels, 19:9 ratio (~432 ppi density)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
browser-firefox-mobile engine-gecko The browser uses the Gecko rendering engine sci-exclude Bugs to exclude from out Top Site Compat Index metrics severity-minor The site has a cosmetic issue. type-GWS-interop Google properties interoperability testing type-GWS-interop-tracked Google properties interoperability testing type-uaoverride Require a UA override for working
Projects
None yet
Development

No branches or pull requests

12 participants