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

prefers-color-scheme for custom panel favicons #5657

Merged
merged 3 commits into from
Feb 23, 2024

Conversation

afbora
Copy link
Member

@afbora afbora commented Sep 18, 2023

This PR …

Related discussion is here #4691 (comment)

Not sure about images. I've created images from SVG icon πŸ™‚

πŸŽ‰ Features

  • prefers-color-scheme for custom panel favicons

Breaking changes

AFAIK, none. Coded backward compatibility but I would appreciate it if you check it too.

Ready?

  • Unit tests for fixed bug/feature
  • In-code documentation (wherever needed)
  • Tests and checks all pass

For review team

@afbora afbora added the type: feature πŸŽ‰ Adds a feature (requests β†’ feedback.getkirby.com) label Sep 18, 2023
@afbora afbora added this to the 4.0.0 milestone Sep 18, 2023
@afbora afbora requested a review from a team September 18, 2023 11:55
@afbora afbora self-assigned this Sep 18, 2023
@afbora afbora force-pushed the v4/feature/favicon-prefers-color-scheme branch from 200b3a9 to fc63ba7 Compare September 18, 2023 11:55
@afbora afbora mentioned this pull request Sep 18, 2023
5 tasks
@afbora afbora force-pushed the v4/feature/favicon-prefers-color-scheme branch 2 times, most recently from ee91595 to 8fe6e0a Compare September 18, 2023 14:29
@distantnative
Copy link
Member

Is it correct that the apple-touch-icon in light has a solid background but in dark is transparent?

@afbora
Copy link
Member Author

afbora commented Sep 23, 2023

That's correct. Feel free to replace images πŸ‘

@distantnative
Copy link
Member

I have no understanding of these things, so I won't touch anything.

It just seemed odd to me that one time it needs a solid fill, then in fair transparency.

@lukasbestle
Copy link
Member

My feeling is that the transparent background makes sense for the favicon (the shade of "dark" varies), but AFAIK the apple-touch-icon needs to have a background. Can't find a source that confirms that at the moment. Have you tested it @afbora?

@afbora afbora marked this pull request as draft September 26, 2023 11:40
@afbora
Copy link
Member Author

afbora commented Sep 26, 2023

I've tested on my Macbook. Favicon working but apple-touch-icon doesn't work for me.

@grommasdietz could you also test this PR, please? Download kirby.zip, replace kirby folder and delete media folder.

@afbora afbora force-pushed the v4/feature/favicon-prefers-color-scheme branch from 64a5fb3 to f4fa14e Compare September 26, 2023 12:08
@lukasbestle
Copy link
Member

Neither the favicon nor the apple-touch-icon work for me. Or to be specific: When I reorder the <link> tags so that the dark variants are listed before their default light variants, both Safari 16.6 on macOS 13.5.2 and iOS 16.4 in the Simulator choose the dark variants (and they look really really great 😍). However then the dark variants are chosen even in light mode.

So it looks like Safari on macOS and iOS don't respect the media="(prefers-color-scheme: dark)" attribute in the way we use it.

@afbora
Copy link
Member Author

afbora commented Sep 26, 2023

At least we can keep this PR structure without dark-mode for the future. What do you think @lukasbestle ?

Copy link
Member

@lukasbestle lukasbestle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, the refactoring by itself is already pretty nice.

But maybe we can also get the dark-mode icons working with @grommasdietz's help.

src/Panel/Assets.php Outdated Show resolved Hide resolved
@grommasdietz
Copy link

Thanks for all of your work. I’m currently in vacation, but had a quick look. Only tested locally with valet on a project so far, it seems that Safari's cache could be a root of the problem. After switching the OS color scheme and clearing Safari's cache it works for me on version 16.6 (18615.3.12.11.2) as well as 17.0 (18617.1.8.1) with this order:

<link rel="apple-touch-icon" href="/assets/icons/dark/apple-touch-icon.png">
<link rel="icon" href="/assets/icons/dark/favicon.ico" sizes="any">
<link rel="apple-touch-icon" href="/assets/icons/light/apple-touch-icon.png" media="(prefers-color-scheme: dark)">
<link rel="icon" href="/assets/icons/light/favicon.ico" sizes="any" media="(prefers-color-scheme: dark)">
<link rel="icon" href="data:image/svg+xml,%3Csvg%20viewBox…" type="image/svg+xml">

I’ll investigate further next week back in the office with a deeper look into your PR.

@lukasbestle
Copy link
Member

I have tried that exact order, unfortunately that always gives me the light images, even after clearing the cache on macOS/deleting website data on iOS.

Enjoy your vacation, it's not urgent at all.

@distantnative distantnative added the needs: changes πŸ” Implement any requested changes to proceed label Oct 8, 2023
@distantnative distantnative removed this from the 4.0.0 milestone Oct 19, 2023
@grommasdietz
Copy link

grommasdietz commented Nov 8, 2023

I tested your PR @afbora. Seems to work for me with custom favicons on Safari 17. Declared it like this in the config:

return [
  'panel' =>[
    'favicon' => [
      [
        'rel'   => 'apple-touch-icon',
        'type'  => 'image/png',
        'url'   =>  'assets/icons/dark/apple-touch-icon.png',
      ],
      [
        'rel'   => 'alternate icon',
        'type'  => 'image/x-icon',
        'url'   => 'assets/icons/dark/favicon.ico',
      ],
      [
        'rel'   => 'apple-touch-icon',
        'type'  => 'image/png',
        'url'   =>  'assets/icons/light/apple-touch-icon.png',
        'media' => '(prefers-color-scheme: dark)',
      ],
      [
        'rel'   => 'alternate icon',
        'type'  => 'image/x-icon',
        'url'   => 'assets/icons/light/favicon.ico',
        'media' => '(prefers-color-scheme: dark)',
      ],
      [
        'rel'   => 'shortcut icon',
        'type'  => 'image/svg+xml',
        'url'   => 'assets/icons/dynamic/icon.svg',
      ],
    ],
  ],
]

I actually never use apple-touch-icons actively by myself, but will investigate further, as well on the default icon set. After the current test, it seems to work in a new window, after some time and/or after deleting the cache and reloading multiple times. I don’t know how the caching of favicons works exactly in Safari (maybe unrelated to the rest of the pages, since it is used in the browser UI), but it seems to work somehow and should be fine for the current preferred color scheme without a switch.

@distantnative distantnative changed the base branch from v4/develop to develop November 28, 2023 09:18
@distantnative distantnative changed the base branch from develop-patch to develop-minor November 29, 2023 14:23
@bastianallgeier
Copy link
Member

It would be really cool to wrap this up. What's still missing to get it done?

@distantnative distantnative force-pushed the v4/feature/favicon-prefers-color-scheme branch from caee67c to 8772938 Compare February 6, 2024 18:20
@distantnative
Copy link
Member

@lukasbestle @afbora
My understanding it that this isn't 100% reliably working for all browsers (mainly Safari).

Could we add the order in that way that even if a Safari version has issues, for that Safari it would look like the current status quo. But all browser versions without issues would get the benefits from this change?

@afbora
Copy link
Member Author

afbora commented Feb 6, 2024

It didn't work properly in Safari, so we couldn't move forward. But Safari seems doesn't support SVG link icons anyway: https://caniuse.com/link-icon-svg

So I think we can keep PR as it is.

@distantnative
Copy link
Member

@afbora but the problem wasn't about the SVG icon, right? It was that Safari would also load the wrong PNG file if I understood @lukasbestle correctly.

@afbora
Copy link
Member Author

afbora commented Feb 19, 2024

I've tested the PR again on;

  • Windows 10 and MacOS Big Sur 11.7.10
  • Chrome, Safari* and Firefox
  • Light and dark mode

Works pretty well for me.

* Safari uses apple-touch-icon.png as default. That's fine since the image have good readability for both dark and light modes.

apple-touch-icon

Copy link
Member

@lukasbestle lukasbestle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I wrote, I think this a pretty great refactoring in itself, even if the prefers-color-scheme feature does not/would not fully work in Safari. In my tests it at least sometimes worked, so that's at least a massive improvement compared to the status quo.

@lukasbestle lukasbestle removed the needs: changes πŸ” Implement any requested changes to proceed label Feb 23, 2024
@lukasbestle lukasbestle marked this pull request as ready for review February 23, 2024 21:29
@lukasbestle lukasbestle added the needs: decision πŸ—³ Requires a decision to proceed label Feb 23, 2024
@distantnative distantnative merged commit 41712af into develop-minor Feb 23, 2024
16 checks passed
@distantnative distantnative deleted the v4/feature/favicon-prefers-color-scheme branch February 23, 2024 21:41
@afbora afbora added this to the 4.2.0 milestone Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: decision πŸ—³ Requires a decision to proceed type: feature πŸŽ‰ Adds a feature (requests β†’ feedback.getkirby.com)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants