-
Notifications
You must be signed in to change notification settings - Fork 25
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
#7622: MV3: Add support for SVG theme icons #7925
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7925 +/- ##
=======================================
Coverage 72.91% 72.91%
=======================================
Files 1291 1291
Lines 40206 40206
Branches 7482 7482
=======================================
Hits 29315 29315
Misses 10891 10891 ☔ View full report in Codecov by Sentry. |
Current status:
|
Can we just setup a proxy? This appears to have too many small issues to be maintainable. If we spun up a small server/lambda/vercel we could pass an SVG URL and receive a PNG, preserving most of the existing code here + if (url.endsWith('svg')) {
+ url = `https://svg-to-png.pixiebrix.com/?width=32&height=32&url=${url}`
+ }
const { data } = await axios.get<Blob>(url, { responseType: "blob" });
return await blobToImageData(data); The proxy itself can use any off-the-shelves SVG-to-PNG converter. Example:
We'll need it until it's natively supported: |
Most of the code improvements were extracted to a non-MV3-specific PR that is ready to be reviewed and merged #7935 |
@@ -25,6 +25,8 @@ export default async function setToolbarIconFromTheme({ | |||
toolbarIcon, | |||
themeName, | |||
}: Pick<ThemeAssets, "logo" | "toolbarIcon" | "themeName">) { | |||
toolbarIcon = | |||
"https://upload.wikimedia.org/wikipedia/commons/f/fd/Ghostscript_Tiger.svg"; | |||
if (toolbarIcon) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoded demo image. This image is SVG and lacks a height/width attribute, which further complicates things without a proxy.
await image.decode(); | ||
// `createImageBitmap` will fail on SVGs that lack width/height attributes, so we must use <img> | ||
return image; | ||
return blobToImageData(blob, width, height); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Boom. This is all that's needed to fully support SVGs both in MV2 and MV3.
Compare it to the chrome.offscreen
-based version (which still had edge cases): https://github.com/pixiebrix/pixiebrix-extension/pull/7925/files/12865f341294d30f965c0cd9ae342ba8fc0e9d97..1a024cf3ce400684bf73ddd0e60be282083cff0b
} | ||
|
||
function getSvgProxyUrl(url: string): string { | ||
return `https://svg-to-png.mrproper.dev/${url}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rust-based demo proxy working on CloudFlare Workers. We need to deploy our own before merging this PR
https://github.com/GewoonJaap/svg-to-png-cf-worker
It's unfortunate Heroku doesn't support serverless functions. Let me discuss with @johnnymetz what the best approach would be for us. I don't see why we couldn't. Alternatively, we might just setup an endpoint on App. |
We also have an account on Vercel already so we could set that up. CloudFlare might be even safer (more generous free plan, as well as a repo ready to be deployed) |
@johnnymetz will create a separate Extension issue to look into this. We're going to pursue the cf worker solution initially. |
Here's the issue: #7982 |
Closing out this PR because we're going to be creating/hosting the PNG based icon: https://www.notion.so/pixiebrix/Support-custom-branding-MV3-browser-action-icon-f43f5cf790ed4776b15e23638f5ce580?pvs=4 |
If we don't support SVG here, we still need to apply most changes in this PR |
Ready for review, but not complete.
What does this PR do?
setToolbarIcon
viachrome.offscreen
#7634Checklist