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

feat: expose window.invalidateShadow() #32452

Merged
merged 5 commits into from
Dec 1, 2022
Merged

Conversation

codebytere
Copy link
Member

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. in webContents 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

Release Notes

Notes: Exposed window.invalidateShadow() to clear residual visual artifacts on macOS.

@nornagon
Copy link
Member

nornagon commented Jan 13, 2022

Hmm, this seems quite obscure and is fairly easily achievable with the objc module, e.g.

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!

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jan 20, 2022
Copy link
Contributor

@zcbenz zcbenz left a 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.

shell/browser/native_window.h Outdated Show resolved Hide resolved
@jkleinsc
Copy link
Contributor

jkleinsc commented Feb 9, 2022

@codebytere can you rebase this? that should get rid of the linux failure

@codebytere
Copy link
Member Author

@jkleinsc done!

Copy link
Member

@samuelmaddock samuelmaddock left a 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?

@nornagon
Copy link
Member

nornagon commented Mar 3, 2022

Let's investigate whether there's a way we can automatically behave correctly instead of requiring the user to give this signal.

@nornagon nornagon self-assigned this Mar 7, 2022
Copy link
Contributor

@RaisinTen RaisinTen left a 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.

docs/api/browser-window.md Outdated Show resolved Hide resolved
@nornagon
Copy link
Member

I haven't been able to find a way that this could be automatically triggered, so API LGTM.

Copy link
Contributor

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

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

API LGTM

@RaisinTen
Copy link
Contributor

I haven't been able to find a way that this could be automatically triggered, so 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?

@nornagon
Copy link
Member

I haven't been able to find a way that this could be automatically triggered, so 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.

@RaisinTen
Copy link
Contributor

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.

@nornagon
Copy link
Member

nornagon commented Mar 31, 2022

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.

@RaisinTen
Copy link
Contributor

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?

@xiajingren
Copy link

Any progress?

@nornagon
Copy link
Member

@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.

@nornagon nornagon marked this pull request as ready for review November 30, 2022 19:42
Co-authored-by: Darshan Sen <raisinten@gmail.com>
Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

API LGTM

@nornagon
Copy link
Member

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.

@nornagon nornagon added target/23-x-y PR should also be added to the "23-x-y" branch. and removed target/17-x-y labels Dec 1, 2022
@nornagon nornagon merged commit d092e6b into main Dec 1, 2022
@nornagon nornagon deleted the expose-invalidate-shadow branch December 1, 2022 18:24
@release-clerk
Copy link

release-clerk bot commented Dec 1, 2022

Release Notes Persisted

Exposed window.invalidateShadow() to clear residual visual artifacts on macOS.

trop bot added a commit that referenced this pull request Dec 1, 2022
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>
@trop
Copy link
Contributor

trop bot commented Dec 1, 2022

I have automatically backported this PR to "23-x-y", please check out #36533

@trop trop bot added in-flight/23-x-y and removed target/23-x-y PR should also be added to the "23-x-y" branch. labels Dec 1, 2022
@lyswhut
Copy link

lyswhut commented Dec 2, 2022

Can this be merged into 20.xy and 21.xy ?

@trop trop bot removed the in-flight/23-x-y label Jan 18, 2023
@lyswhut
Copy link

lyswhut commented Jan 18, 2023

@codebytere Can this be merged into 20.xy and 21.xy ?

@lyswhut
Copy link

lyswhut commented Feb 11, 2023

@codebytere @RaisinTen Electron 23 no longer supports Windows 7, can this PR be merged into 22.x.y? I want to try this API :)

khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
Co-authored-by: Jeremy Rose <jeremya@chromium.org>
Co-authored-by: Darshan Sen <raisinten@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-review/approved ✅ semver/minor backwards-compatible functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: macOS transparent window font shadow residual
9 participants