-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Fix css injection on Chrome [improved but not perfect] #3883
Conversation
Since no related reports for Vimium-Fx in philc#3452 .
Aching to see this fix implemented in master. Great stuff @gdh1995 ! |
Tested on Chrome 94.0.4606.54 by installing from the source. It works like a charm! Thank you. 🙏 |
Also tested on Chromium 94.0.4606.71 and it resolves all race conditions. Thanks @gdh1995 for the fix! |
What's the status here? |
Any updates on when this might be merged? I'm still experiencing #3956. |
How is this not merged yet? It's a super annoying bug. I've inserted custom CSS to have blank link hints with white text. But now I get the yellow default hints with white text, making them barely readable. |
I hope a maintainer with merge access sees this PR. This code fixes #3956 |
return chrome.tabs.insertCSS(tabId, cssConf, () => chrome.runtime.lastError); | ||
const callback = () => chrome.runtime.lastError; | ||
chrome.tabs.insertCSS(tabId, cssConf, callback); | ||
if (Utils.isFirefox()) { |
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.
FWIW I see the same issue in Firefox.
Sorry for the delay here. I'd like to merge this, but I haven't been able to reproduce the underlying issue. Is there a reliable test case? |
@philc Chrome 102 and 115 on Windows 11 with Vimium master can reproduce this: |
OK, I was finally able to reproduce this. Thank you @gdh1995. My notes:
I also couldn't reproduce this in Vimium 2.0 on either Windows or MacOS. This may be because we're now using the chrome.scripting.insertCSS instead of chrome.tabs.insertCSS (chrome.tabs.insertCSS was removed in Manifest v3), and chrome.scripting.insertCSS somehow behaves differently such that this issue doesn't occur. I'm going to close this as I believe it should finally be resolved once Vimium 2.0 hits the store. Also, the fix in this PR won't work on the current manifest v3 codebase, since scripting.insertCSS doesn't have a run_at parameter. If anyone is still able to reproduce this on Vimium 2.0, please post a comment in #3452 indicating the procedure to do so, and your browser and OS. |
This ensures
userDefinedLinkHintCss
can override those invimium.css
, so it can replace #3421, fix #3418, fix #3452 and fix #3956.As said in #3418 (comment) , during my tests the document about
chrome.tabs.insertCSS
is not correct - the CSS may be used earlier than those defined inmanifest.json
.Updated (2021/11/16): as said in #3956, CSS inserted at
document_start
may be used earlier even on a top iframe on Chrome 95+Win10.