Prevent Firefox from using apple-touch-icon #10402
Conversation
Codecov Report
@@ Coverage Diff @@
## master #10402 +/- ##
==========================================
- Coverage 43.7% 43.68% -0.02%
==========================================
Files 586 586
Lines 81354 81354
==========================================
- Hits 35552 35540 -12
- Misses 41401 41412 +11
- Partials 4401 4402 +1
Continue to review full report at Codecov.
|
GiteaBot
added
the
lgtm/need 2
routers/routes/routes.go
Outdated
| @@ -1002,6 +1002,12 @@ func RegisterRoutes(m *macaron.Macaron) { | |||
| } | |||
| }) | |||
|
|
|||
| m.Get("/apple-touch-icon.png", func(ctx *context.Context) { | |||
| file := path.Join(setting.StaticRootPath, "public/img/apple-touch-icon.png") | |||
There was a problem hiding this comment.
This is not right because the file maybe embedded into binary. You should use functions from modules/public to serve file.
There was a problem hiding this comment.
Changed it to a redirect. Ideally, it should be a "rewrite" but I'm not sure how I would do that. I would like to avoid having to set all the caching headers and such which seems to be needed with public.Asset which only returns bytes without metadata.
There was a problem hiding this comment.
Open to suggestions on how to delegate the request directly to the static file hander.
GiteaBot
added
lgtm/need 1
GiteaBot
added
lgtm/done
|
Maybe you should change appsuburl to StaticUrlPrefix so that it's possible to be serve by CDN. |
|
What's the difference between AppSubUrl and StaticUrlPrefix exactly? On a default installation, both are empty string and on gitea.com StaticUrlPrefix is on a second domain. Both seem to serve assets fine. |
|
@silverwind on gitea.com |
The opaque background does not work well in Firefox which uses the icon as a "rich icon". Prevent this by not specifying it in HTML. Real Apple devices will still request the icon on the static path. Fixes: go-gitea#10394 Also adjust gitignore so app.ini.sample becomes searchable and fixed a variable name in app.ini.sample.
|
Ok, using Pushed two additional tweaks:
|
|
gitignore fixed, it needs to be a bit verbose to work (taken from https://stackoverflow.com/a/16318111/808699). |
| /custom/* | ||
| !/custom/conf | ||
| /custom/conf/* | ||
| !/custom/conf/app.ini.sample |
There was a problem hiding this comment.
Why add those to .gitignore?
There was a problem hiding this comment.
/custom/conf/app.ini.sampleis tracked in git so should not be in.gitignore- so I can search
/custom/conf/app.ini.sampleusingagorrgtools which respect.gitignore
There was a problem hiding this comment.
And no, these 4 lines can not be reduced to 2 because of how .gitignore works.
There was a problem hiding this comment.
@silverwind so those lines are required?
There was a problem hiding this comment.
Yes, it will not work with just 2 lines, that was my first failed attempt.
Those 4 lines essentially say "exclude everything in /custom except /custom/conf/app.ini.sample"
| @@ -217,7 +217,7 @@ FILE_EXTENSIONS = .md,.markdown,.mdown,.mkd | |||
| PROTOCOL = http | |||
| DOMAIN = localhost | |||
| ROOT_URL = %(PROTOCOL)s:https://%(DOMAIN)s:%(HTTP_PORT)s/ | |||
| ; when STATIC_URL_PREFIX is empty it will follow APP_URL | |||
| ; when STATIC_URL_PREFIX is empty it will follow ROOT_URL | |||
There was a problem hiding this comment.
Why following ROOT_URL instead?
There was a problem hiding this comment.
ROOT_URL is correct in config file context. The name AppURL is only used in code:
AppURL = sec.Key("ROOT_URL").MustString(defaultAppURL)
|
I would say ready for merge? |
The opaque background does not work well in Firefox which uses the icon as a "rich icon". Prevent this by not specifying it in HTML. Real Apple devices will still request the icon on the static path.
Fixes: #10394