fix: create fake parent GtkWindow for file dialog #35110
Conversation
|
I need help testing this PR. CC @deepak1556 I keep running into this build issue: so I couldn't test my changes. I tried disabling the SSL check, but that didn't work either. In order to verify that this commit does what it says it does, be on a platform which is using the XDG Desktop Portal with GTK >= 3.20. Create an electron application and then run a file dialog. If the file dialog is movable or appears behind the parent window, you have found the bug. Then checkout this branch. Repeat the steps. If and only if the parent no longer appears behind the parent window and is no longer movable, the bug has been fixed. Presumably this is the check that Electron is failing on the |
|
This PR should be backported for all electron versions still supported that are >=14.0.0 |
|
Ping @zcbenz since you merged my last PR. |
|
Thanks @tristan957 , I can test this PR locally tomorrow. QQ: Does setting the fake parent as transient for the Electron window help with screen readers ? Or will that issue be not addressable with this fix ? Refs microsoft/vscode#121811 (comment) |
|
@tristan957 this is happening becuase your local Node.js version is too new for Webpack - it's not an Electron issue. Try using v16 LTS, it should work. See https://stackoverflow.com/questions/69394632/webpack-build-failing-with-err-ossl-evp-unsupported |
|
@codebytere thanks! I will now set my personal computer off to build electron! I have a fix for the asan failure locally, I believe. Will shoot that off if I can confirm the commit. Regarding the screen reader issue, maybe. One could assume that transiency has something to do with screen readers. If someone could point me at an example program, which just launches a window, and then a file dialog on a button press or something, that would be great. Otherwise, I'll read some electron docs. Should be able to confirm the fix today. |
|
The gist from #32857 (comment) can be used to test the fix. |
|
Another build failure 37000 artifacts later: Something else I am missing? |
Support for the XDG Desktop Portal first occurred in #19159, authored by me. Unfortunately there was a regression on the way that the file dialog was now presented. Normally, Chromium sets the "transient-for" property on the dialog using gtk::SetGtkTransientFor(). Unfortunately the work in the previous PR couldn't use this function because a GtkFileChooserNative is not a GtkWidget, which is required by the function, so the effects of window transiency were lost on platforms where GTK was >= 3.20. BUT! The lack of parent GTK window for gtk_file_chooser_native_new() can be corrected by creating a fake GtkWindow. Given a GdkScreen and a type of GTK_WINDOW_TOPLEVEL, we are able to create a GtkWindow that has an xid or Wayland handle of the Electron window. From here, we can pass the fake parent to the function. Instead of Chromium setting the transiency of the file chooser, the XDG Desktop Portal will now adequately be able to set window transiency.
There was a problem hiding this comment.
Reading the source code of GTK, it seems that the parent window must be visible to make the transit-for actually work:
https://github.com/GNOME/gtk/blob/3f6b5ccf4ab49c7e867ae4b9f11397598bd4eba8/gtk/gtkfilechoosernativeportal.c#L513-L531
|
Is it possible to call |
|
Nope, not possible since |
|
So from my perspective it seems the only solution is to resort to DBus...ugh |
|
Seems like libportal supports the File Chooser portal, would it be possible for electron to depend on libportal on Linux? https://github.com/flatpak/libportal/blob/main/libportal/filechooser.h |
|
I guess not since Electron presumably still supports Ubuntu 18.04 |
|
Chromium does have an implementation of file chooser portal which can be copied: but I wonder if we should just change the code to use |
|
I'll take a look at this next week. Thanks for the tip.
Do you mean just Is there any reason Electron can't just use the file dialogs that Chromium provides? Never quite understood why Electron has their own implementations. |
Chromium has code to choose native dialog for different desktops, we should not use
Chromium's dialog code was lacking features we need, but it was years ago and things have changed a lot. |
|
It would be interesting to re-evaluate this. Reducing the amount of duplicate effort on this front would be great. |
|
@tristan957 you're right and we have a bit - #30663 is one example i can definitely try to look into this - we do need our own implementations at least for the dialog apis still since we expose more than Chromium, but there's absolutely some cleanup to be done imo |
|
@codebytere what functionality does Electron need on top of what Chromium offers? |
|
I'm closing this PR since this approach can not fix the issue. |
Support for the XDG Desktop Portal first occurred in #19159, authored
by me. Unfortunately there was a regression on the way that the file
dialog was now presented. Normally, Chromium sets the "transient-for"
property on the dialog using gtk::SetGtkTransientFor(). Unfortunately
the work in the previous PR couldn't use this function because a
GtkFileChooserNative is not a GtkWidget, which is required by the
function, so the effects of window transiency were lost on platforms
where GTK was >= 3.20.
BUT! The lack of parent GTK window for gtk_file_chooser_native_new() can
be corrected by creating a fake GtkWindow. Given a GdkScreen and a type
of GTK_WINDOW_TOPLEVEL, we are able to create GtkWindow that has an xid
or Wayland handle of the Electron window. From here, we can pass the
fake parent to the function. Instead of Chromium setting the transiency
of the file chooser, the XDG Desktop Portal will now adequately be able
to set window transiency.
Fixes #32857
Description of Change
Checklist
npm testpassesRelease Notes
Notes: On Linux platforms with a GTK >= 3.20 using the XDG Desktop Portal, file dialogs will no longer appear behind their parent windows nor will they be movable as was the case prior to Electron v14.0.0.