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

fix: propagates pointer events in draggable regions for frameless windows on macOS #38208

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

gerhardberger
Copy link
Contributor

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

Release Notes

Notes: Pointer events are propagated in draggable regions for frameless windows on macOS.

Copy link
Member

@codebytere codebytere left a 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];
+}
Copy link
Contributor

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.

@gerhardberger
Copy link
Contributor Author

@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 looked the referenced PR but didn't see a good solution there yet.

@tzahola
Copy link
Contributor

tzahola commented May 8, 2023

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?

I'd argue it's the other way around: the quoted context menu hack is made redundant by the changes in this PR.

@codebytere
Copy link
Member

codebytere commented May 9, 2023

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

@tzahola
Copy link
Contributor

tzahola commented May 9, 2023

@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 GetIsDraggableBackground check in render_widget_host_view_cocoa.mm instead of reaching out to the Electron-specific [self.window.contentView isDraggableBackgroundAt:]. If it was implemented this way, it could be upstreamed IMO, but I haven't looked into the exact relationships between the various widgethost/viewhost/windowhost/etc. objects yet, to say for certain whether it could work or not.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label May 15, 2023
@zcbenz
Copy link
Member

zcbenz commented May 22, 2023

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.

Was left clicking on draggable region working before the refactor? I remember the old implementation was also relying on overriding hitTest method.

@tzahola
Copy link
Contributor

tzahola commented May 22, 2023

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.

Copy link
Member

@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 adding a patch to fix the issue since it is a regression.

@liees
Copy link

liees commented May 24, 2023

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

Copy link
Member

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

@MarshallOfSound
Copy link
Member

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.

@PalmerAL
Copy link
Contributor

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.

@pacocoursey
Copy link

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

Elements with -webkit-app-region: drag seem to act as z-index: Infinity, blocking all children from interaction and even becoming un-selectable via Chrome DevTools!

@ivanggq
Copy link

ivanggq commented Jun 14, 2023

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.

@sdr0x07b6
Copy link

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.
I knew that if the Electron team would fix the bug, I would be free from the phenomenon.

@sdr0x07b6
Copy link

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

@zcbenz
Copy link
Member

zcbenz commented Nov 21, 2023

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

@deanylev
Copy link

deanylev commented Jan 9, 2024

@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

markh-discord pushed a commit to discord/electron that referenced this pull request Feb 6, 2024
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
markh-discord pushed a commit to discord/electron that referenced this pull request Mar 15, 2024
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
markh-discord pushed a commit to discord/electron that referenced this pull request Apr 9, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Can not fire click(or double click) event if set a '-webkit-app-region: drag;' style