-
Notifications
You must be signed in to change notification settings - Fork 15k
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: propagates pointer events in draggable regions for frameless windows on macOS #38208
base: main
Are you sure you want to change the base?
fix: propagates pointer events in draggable regions for frameless windows on macOS #38208
Conversation
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.
What would the long-term plan for this be? This patch looks like it would need to be floated indefinitely, and also doesn't look like something we could upstream. Have you considered a solution more along the lines of #37386?
// instantly draggable without asking (tracked at https://crbug.com/830962). | ||
- (NSView*)hitTest:(NSPoint)point { | ||
+ return [super hitTest:point]; | ||
+} |
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.
No need to override hitTest:
anymore.
@codebytere yeah, this patch would needed to be kept around until we are relying on BridgedContentView and the other underlying Chromium view subclasses for frameless views. |
I'd argue it's the other way around: the quoted context menu hack is made redundant by the changes in this PR. |
@tzahola Electron has a patch policy. Whether or not the new patch removes the need for the non-patch similar solution does not address the issue at hand, which is that patches that don't have a clear path to removal or alternate non-patch solution typically aren't accepted. |
Sure. On the other hand, implementing it as you've suggested would also imply patching Chromium, as the mechanism #37386 exploits is specific to right-clicks. So the situation would be the same regards to patching. Anyway, in my opinion the proper solution would be to somehow do the |
Was left clicking on draggable region working before the refactor? I remember the old implementation was also relying on overriding hitTest method. |
Yes, it was working. |
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 adding a patch to fix the issue since it is a regression.
patches/chromium/propagates_pointer_events_to_draggable_regions_in_frameless_windows.patch
Outdated
Show resolved
Hide resolved
The mouse can trigger the pointer event normally, but my handle is completely useless. The magic is that I use git bash or vscode to start the development mode of electron and I can use the handle to click It is invalid after using window shell or packaging... so. what should I do |
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 giving this PR an approval since it is fixing a regression and I think we can include the patch first before figuring out a future plan for this patch, but we should get approvals from @electron/wg-upgrades before merging.
Can we hold on merging this till I'm back from vacation and can check some things. I believe we found this during an upgrade and the old behavior was actually deemed to be a bug and the new behavior is correct. Instead of making this bug into a seesaw let's wait till I can check some things. |
Whether it was intended or not, the old behavior was very useful - in my case, I have a hidden titlebar with a set of tabs in the titlebar area. With the old behavior, the tabs could be clickable, but dragging in the titlebar area would still move the window. With the new behavior, this would require adding extra space to the titlebar to create a draggable region. |
@MarshallOfSound Welcome back from what I hope was a nice vacation. The current behavior is certainly a bug – maybe a different one than the old behavior had. I'll quote from the opened issue:
|
Note that the behavior on Windows is unchanged - it is like the original behavior on Mac. So following this logic, the current behavior on Mac is the odd one out. |
I had been struggling with this phenomenon for a long time. I spent an inordinate amount of time and exhaustion trying to put every possible twist on my code to see if there was a workaround. This phenomenon is not reproduced when building with Electron up to v22.x. It is reproduced from v23.x and later. I was relieved in a way, as it was clear that it was apparently a problem with Electron itself. |
@zcbenz Hi, This issue, which has not been resolved for a long time, seems to have been left unresolved ever since you went to the trouble of approving it. Could you tell us a little about the current situation? |
It is currently blocked by #38208 (comment). |
@MarshallOfSound is this able to be merged? It is keeping us on quite an old Electron version at this point, as there is simply no workaround for the functionality we need |
This patch changes Chromiums built-in hit testing so in frameless windows pointer events are propagated even in draggable regions. Patch taken from: electron#38208 Patch-Filename: cherry-pick_propagate_pointer_events_macos_draggable_regions.patch
This patch changes Chromiums built-in hit testing so in frameless windows pointer events are propagated even in draggable regions. Patch taken from: electron#38208 Patch-Filename: cherry-pick_propagate_pointer_events_macos_draggable_regions.patch
This patch changes Chromiums built-in hit testing so in frameless windows pointer events are propagated even in draggable regions. Patch taken from: electron#38208 Patch-Filename: cherry-pick_propagate_pointer_events_macos_draggable_regions.patch
Description of Change
Fixes #37789
This PR fixes the issue caused by a recent refactor about how frameless windows handle pointer events. Chromium's built-in mechanism ignores pointer events in draggable regions in frameless windows.
This PR changes Chromiums built-in hit testing so in frameless windows pointer events are propagated even in draggable regions.
Before:
before.mov
After:
after.mov
Checklist
npm test
passesRelease Notes
Notes: Pointer events are propagated in draggable regions for frameless windows on macOS.