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

[J4] Fix Favicon Appereance in Chrome #37713

Closed
wants to merge 7 commits into from
Closed

[J4] Fix Favicon Appereance in Chrome #37713

wants to merge 7 commits into from

Conversation

coolcat-creations
Copy link
Contributor

Pull Request for Issue #34767.

Sorry for the wrong commit message

Summary of Changes

The Issue describes a problem in Chrome that the favicon svg is not displayed correctly when using darkmode.
Confirming this issue I researched at officially Chrome should meanwhile support the prefers-color-scheme media query. But altought I updated to the latest Chrome Version Version 101.0.4951.41 (Official Build) (arm64) it does not.

So I found another workaround where the specific favicon can be added by using media as attribute in the link.

Testing Instructions

Enable Dark mode in Chrome
Check if Joomla Default Favicon is white on dark ground in Frontend (using Cassiopeia) and Backend (using atum) and on login, error and offline page

Actual result BEFORE applying this Pull Request

The Default Joomla Favicon shows black on black in Frontend (using Cassiopeia) and Backend (using atum)
grafik

Expected result AFTER applying this Pull Request

grafik

Documentation Changes Required

no

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.1-dev labels May 1, 2022
@pritam825
Copy link
Contributor

pritam825 commented May 1, 2022

@coolcat-creations I think the issue in the logo when opening in microsoft edge , not in chrome because in chrome there is no any direct features (system default, light, dark) like microsoft edge and github logo is also not changing with changing the theme , but when applying the patch it's working fine for microsoft edge but the logo is coming, different from the expected result.
This logo is coming after applying the patch and with the dark theme of microsoft edge.
Screenshot (2124)
Thanks :)

@dgrammatiko
Copy link
Contributor

@coolcat-creations the bug seems that is already fixed in chrome https://bugs.chromium.org/p/chromium/issues/detail?id=1311553#c12

Also this PR breaks badly B/C for people that have created their own favicons, so in that sense it's wrong

@coolcat-creations
Copy link
Contributor Author

Did you check in chrome ? Because I did and it seems to work for normal displayed svg on a website) but not for svg used as favicon.

@dgrammatiko
Copy link
Contributor

Did you check in chrome ?

Yes, Chrome Canary 103 works for me

@coolcat-creations
Copy link
Contributor Author

@dgrammatiko there is no Version 103 available for Mac... And the issue is still there. But if you are saying thats purely a chrome issue and even if this PR fixes it - we have to close both: issue and PR

@coolcat-creations
Copy link
Contributor Author

@pritam825 your issue is bit unrelated to this one. Your Browser does not support svg at all and so it shows the ico format.

@pritam825
Copy link
Contributor

@coolcat-creations ok got it mam, and my According pr and issue both should be closed, because if we will used dark theme in chrome, GitHub logo is also not changing as we are comparing to GitHub, Thanks :)

@coolcat-creations
Copy link
Contributor Author

My github logo is changing, my Joomla Logo not.
grafik
Funny is that github delivers a separate dark icon like I proposed here. :-)

@dgrammatiko
Copy link
Contributor

there is no Version 103 available for Mac.

Nightly build for developers: https://www.google.com/chrome/canary/

And the issue is still there

You have to test the canary version

But if you are saying thats purely a chrome issue and even if this PR fixes it

It is a Chrome issue, Safari and Firefox obey the CSS of the SVG and that PR seems to solve the the issue. Also, again, this PR is introducing a B/C break on both the templates...

@coolcat-creations
Copy link
Contributor Author

How comes you think it's a bc break? Its a new file and did not change an existing file.

@dgrammatiko
Copy link
Contributor

How comes you think it's a bc break? Its a new file and did not change an existing file.

So, if I have created an svg file for both cassiopeia and atum that have css to control the dark/light theme then that file will be available only on the light theme and on dark theme I will get the standard joomla logo. In other words you broke existing functionality.

Also please understand that this IS a chrome issue and THEY have to fix it. Patching Joomla is not the right path

@Bakual
Copy link
Contributor

Bakual commented May 2, 2022

It's not a B/C break per our definition (Design/HTML markup is explicitely excluded). Just saying 😄

@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on 768793c

Confirmed issue on windows with chrome Version 102.0.5005.63 (Official Build) (64-bit)


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/37713.

@Quy
Copy link
Contributor

Quy commented Jun 16, 2022

In Windows Firefox 101.0.1 (not dark mode), it displays the white icon.
The first tab is from the prebuilt packpage of this PR.
The second tab is from the nightly build.

37713-firefox

@HLeithner HLeithner changed the base branch from 4.1-dev to 4.2-dev June 27, 2022 13:05
@HLeithner
Copy link
Member

This pull requests has automatically rebased to 4.2-dev.

@joomla-bot
Copy link
Contributor

This pull requests has been automatically converted to the PSR-12 coding standard.

@rdeutz rdeutz removed the PR-4.1-dev label Aug 1, 2022
@coolcat-creations coolcat-creations closed this by deleting the head repository Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Information Required NPM Resource Changed This Pull Request can't be tested by Patchtester
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants