brave/brave-talk

Low resolution image for Brave Talk on wallet connect panel

srirambv opened this issue · 31 comments

srirambv commented

Description

Low resolution image for Brave Talk on wallet connect panel

Steps

  1. Create Web3 Talk room
  2. Wait for Wallet panel to open
  3. Has a very pixelated image for Brave

Actual Result

image

Expected Result

Good resolution image on wallet panel

Additional Information

cc: @mrose17 @brave/design-team

mrose17 commented

@srirambv - i can't reproduce this. could you create a video for me to watch? thank you!

srirambv commented

Sure here's a recording for both moderator and participant

Moderator Participant
824moderator.mp4
824participant.mp4
tackley commented

Hmm I wonder where it's picking up the icon from. Is it the favicon, one of the images from the manifest, or taken from the opengraph meta tags?

srirambv commented

This is a Wallet UI issue not related to Talk. I see low resolution images even when connecting to other dapps as well. Logged brave/brave-browser#29969 for it and closing this one

mrose17 commented

@srirambv - thanks for submitting the issue. i've re-opened it and tagged it with the wallet-support label, so we can keep an eye on it.

  • ❤️1
mrose17 commented

@srirambv - is this reproducing? did the wallet folks indicate where they are getting the favicon?

srirambv commented

It still reproduces. Here's what @Douglashdaniel said about the image resolution in the issue

We cant control the resolution that a DApp serves for the fav icon on their own site, it's usually just enough for browsers tab.

The src in our front-end code is using chrome://favicon/size/64@1x/ and the style uses 40px or 48px depending on the screen, so 64 should be more than sufficient. I've attempted other sizes and it doesn't make a difference.

I think this is something out of our control, unless we have a mapping of popular DApps with their logo's, but that would also probably come with it's own security risks, since a domain could change their fav icon at any time.

mrose17 commented

ok, here is the fix:

  1. create /public/favicon.ico that is a 64x64 icon of the Brave Talk icon
  2. add public/images/brave-talk-64x64.png and put it as the first <link rel="icon" .../> element in the HTML <head>
  3. update public/manifest.json to include reference to the new png

for folks wondering about the redundancy: neither favicon.ico (directly) or maniest.json (for indirection) are universally implemented.

srirambv commented

Brave Talk icon seems fine but still a little pixelated around the edges
Sl26xlUqK51

mrose17 commented

@srirambv - i can't see the issue, but my eyes aren't all that good these days.

@johnhalbert - can you remind me which image, exactly are we using for this?

@Douglashdaniel - your thoughts?

mrose17 commented

also, are we using an image with a transparent background? in looking at the video attached to #917 ... i don't think that's the case! sorry for the nuance...

johnhalbert commented

@mrose17 I used Gimp to do the editing on these. I'm seeing transparent backgrounds on all the images above. And in the video from #917 you can see the favicon is updated and appears to have a transparent background as well.

If I've made a mistake in how they were saved, however, it might be a good idea to have someone with actual graphic design experience update these with the appropriate background, settings, etc.

aguscruiz commented

Hi @johnhalbert you can export the icons from this Figma File if you want, I created a preset to export the same sizes you listed above

https://www.figma.com/file/ycXqw8euy6z41uXrlDni6K/Desktop-Brave-Talk?type=design&node-id=3099-57242

Also added a .svg export so you can create the .ico from it

johnhalbert commented

@aguscruiz ok on the preset sizes. Can you help me with the .ico? I'm probably not generating these correctly and would rather defer to someone who had expertise on this.

aguscruiz commented

Sure. I think this one should work, let me know how it goes

brave-talk.ico.zip

johnhalbert commented

@srirambv this is ready for re-test

srirambv commented

@johnhalbert still seeing the low resolution image on dev.

brave_Qun4JdM0s3.mp4
bradleyrichter commented

can we not just use the SVG that is already in place?

image
mrose17 commented

@bradleyrichter - i don't think we can use an svg directly for a favicon thing...

@johnhalbert - can you re-re-verify that the images we think are on dev are actually there and being referenced in the <head/> element?

@srirambv - can you verify exactly which icons (i.e., what exactly image URLs) are showing up pixelated and include only a screenshot (and not a video) for those? (i'm not sure that it possible to get the URL, but i thought i'd be ask)

johnhalbert commented

@srirambv this is ready for re-test in dev.

srirambv commented

Still seeing a slightly pixelated image on the connect wallet screen. More discussion here

image image
mrose17 commented

@Douglashdaniel - the current thinking here in "Web3 Brave Talk" land is that the wallet code is actually hard-wired to look for the 64x64 icon, rather than using the first one (128x128) in the <head/> and in the manifest.json file.

can you confirm one way or another? thank you!

Douglashdaniel commented

As far as the Front-end code goes, it is set like so

https://github.com/brave/brave-core/blob/master/components/brave_wallet_ui/components/extension/connect-with-site-panel/connect-with-site-header/connect-with-site-header.tsx#L101

https://github.com/brave/brave-core/blob/master/components/brave_wallet_ui/components/extension/connect-with-site-panel/connect-with-site-header/connect-with-site-header.tsx#L111

<FavIcon
   src={`chrome://favicon/size/64@1x/${originInfo.originSpec}`}
   isReadyToConnect={isReadyToConnect}
/>

In my testing changing to chrome://favicon/size/128@1x/${originInfo.originSpec} or any other resolution wasn't making any difference.

I'm unsure what chrome://favicon is deciding to return behind the scenes. Maybe we can try to bumping it again to 128@1x and see if it makes a difference this time! :)

srirambv commented

@Douglashdaniel does bumping it to 120@1x fix it at least for Brave Talk if not for other Dapps. There's brave/brave-browser#29969 where you mentioned it can't be controlled for icons and is ok to set it to 64@1x/ for Dapps. I think in this case, just making sure the image resolution isn't pixelated for Brave Talk would be a good first step

Douglashdaniel commented

@srirambv I tested last night with it set to chrome://favicon/size/128@1x/${originInfo.originSpec} and it didn't make any difference, is still pixelated..

Screenshot 36 Screenshot 37

You can test this manually in the browser, the icon size will change, but the resolution does not improve.

brave://favicon/size/64@1x/https://talk.brave.com
Screenshot 42

brave://favicon/size/128@1x/https://talk.brave.com
Screenshot 43

Discussed with @petemill and he is under the impression that chrome will not give us a higher resolution favicon.

Douglashdaniel commented

Maybe we could just do a check for Brave Talk origin in the Wallet Front-end and display a higher resolution icon!
@kdenhartog would you see any issues with us doing that?

kdenhartog commented

Nah no concerns from me because it's a Brave origin. I wouldn't want to do this for others though at this point because it means we need to maintain another list.

  • 👍1
mrose17 commented

I have no objections to adding a check for the Brave Talk origin, though I don't think it particularly necessary...