-
Notifications
You must be signed in to change notification settings - Fork 15.4k
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
feat: expose window.invalidateShadow() #32452
Conversation
9256749
to
3d9fe73
Compare
Hmm, this seems quite obscure and is fairly easily achievable with the const objc = require('objc')
const nativeWindowHandle = mainWindow.getNativeWindowHandle().readPointer()
const nsview = objc.wrap(nativeWindowHandle)
nsview.window().invalidateShadow() I'm not necessarily against adding this API but I'd like to understand a bit more about whether we can avoid adding it! |
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.
I'm good with this API since a large number of BrowserWindow's APIs are just simple wrappers of native APIs.
3d9fe73
to
c314335
Compare
@codebytere can you rebase this? that should get rid of the linux failure |
c314335
to
d17b2b9
Compare
@jkleinsc done! |
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.
I've noticed this before when moving a window between screens. Could we call this internally at the end of dragging/moving a window to solve some amount of problems without requiring developers to know about this method?
Let's investigate whether there's a way we can automatically behave correctly instead of requiring the user to give this signal. |
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.
How often are users expected to call this method during an animation? If it is required for every redraw, I think it's something that should be done internally.
I haven't been able to find a way that this could be automatically triggered, so API LGTM. |
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.
API LGTM
@nornagon why can't we call this internally whenever users are expected to call it? I'm not sure I fully understand when / how frequently users are expected to call this though, could you please explain that? |
Users should call this method when the outline of their window has changed. The vast majority of apps never need to call it. |
@nornagon then we should be able to run an event handler when the outline of the window changes only for transparent windows that just calls this function. That way, we can abstract away this functionality from our users. |
Yep! The only catch is, there's no such event, as far as I can tell. Chromium doesn't tell us when the outline of the window changes. |
@nornagon ah, but then how are users going to figure out when this API should be called if we can't expose a listener for the outline changing event? |
Any progress? |
@RaisinTen you've misunderstood when the function needs to be called; it's not just when the window is moved or resized, but whenever the outline of the window changes. in this example, that happens whenever the number is updated, or when the window scrolls. |
Co-authored-by: Darshan Sen <raisinten@gmail.com>
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.
API LGTM
I think this is reasonable to land; we've looked for a different way to do it and haven't found one. It makes sense to allow apps to invalidate their window shadows when they know the window shape has changed. |
Release Notes Persisted
|
Co-authored-by: Jeremy Rose <jeremya@chromium.org> Co-authored-by: Darshan Sen <raisinten@gmail.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
I have automatically backported this PR to "23-x-y", please check out #36533 |
Can this be merged into 20.xy and 21.xy ? |
@codebytere Can this be merged into 20.xy and 21.xy ? |
@codebytere @RaisinTen Electron 23 no longer supports Windows 7, can this PR be merged into 22.x.y? I want to try this API :) |
Co-authored-by: Jeremy Rose <jeremya@chromium.org> Co-authored-by: Darshan Sen <raisinten@gmail.com>
Description of Change
Closes #32450.
This PR exposes a new macOS-only method
window.invalidateShadow()
. This method can be used to clear residual visual artifacts when, for example, performing an animation.BrowserWindows
that are transparent and frameless can sometimes leave behind visual artifacts on macOS. To remedy this, the shadow can be forcibly cleared on an NSWindow by calling-[NSWindow invalidateShadow]
, but within the constraints of how animations are performed e.g. inwebContents
right now, the only way to solve this within the codebase without exposing a new method would be to override- (void)drawRect:(NSRect)rect
. This has significant negative perf implications - even if we only invalidated the shadow for frameless windows simply the act of calling this method for every single redraw would slow down applications tenfold. As such, the simplest way to remedy this issue is to expose this method to end-users and allow them to invoke it according to their needs.However, if someone has an alternate approach for this i'd be more than open to it!
Checklist
npm test
passesRelease Notes
Notes: Exposed
window.invalidateShadow()
to clear residual visual artifacts on macOS.