Skip to content

servoshell: Port Authentication dialog code to use egui instead of tinyfiledialogs#35507

Merged
mrobinson merged 1 commit intoservo:mainfrom
chickenleaf:authentication-dialog
Feb 19, 2025
Merged

servoshell: Port Authentication dialog code to use egui instead of tinyfiledialogs#35507
mrobinson merged 1 commit intoservo:mainfrom
chickenleaf:authentication-dialog

Conversation

@chickenleaf
Copy link
Contributor

@chickenleaf chickenleaf commented Feb 17, 2025

  • Part of migration from tinyfiledialogs to egui
  • Issues:
  1. Enter key when dialog is open, interacts with the webview that is behind the dialog (and not in focus) and ends up queuing another dialog behind.
  2. Authentication dialog does not show up without this work around:
pub(crate) fn new_toplevel_webview(self: &Rc<Self>, url: Url) {
        let webview = self.servo().new_webview(url);
        webview.set_delegate(self.clone());
        if self.inner_mut().focused_webview_id.is_none() {
            self.inner_mut().focused_webview_id = Some(webview.id());
        }
        self.add(webview);
    }
  1. Cannot access another tab when a dialog is open

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #___ (GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because ___

@chickenleaf
Copy link
Contributor Author

@mrobinson @mukilan

Copy link
Member

@mrobinson mrobinson left a comment

Choose a reason for hiding this comment

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

Looks good with the changes that @mukilan suggested. This will need a small change in order to remove the workaround.

Comment on lines +112 to +115
if self.inner_mut().focused_webview_id.is_none() {
self.inner_mut().focused_webview_id = Some(webview.id());
}
Copy link
Member

Choose a reason for hiding this comment

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

I looked into this a bit and I think it will be hard to fix this issue before we do more changes in the compositor (having a compositor per WebView and setting up the association synchronously when creating WebViews). In the meantime maybe we can look into just setting focused_webview_id as the first WebView we create when creating AppState.

Copy link
Member

@mrobinson mrobinson Feb 19, 2025

Choose a reason for hiding this comment

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

I've slightly modified this workaround to be a bit less invasive.

Copy link
Contributor Author

@chickenleaf chickenleaf Feb 19, 2025

Choose a reason for hiding this comment

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

I just pushed this dialog with corrections without the changes you have done, is that alright?
i added the patch @mukilan gave to fix the background mouse interactions and also the Escape and Enter keyboard interactions.

@mrobinson mrobinson force-pushed the authentication-dialog branch from bbb4251 to 284a7ed Compare February 19, 2025 11:15
@mrobinson mrobinson enabled auto-merge February 19, 2025 11:16
auto-merge was automatically disabled February 19, 2025 11:26

Head branch was pushed to by a user without write access

@chickenleaf chickenleaf force-pushed the authentication-dialog branch from 284a7ed to cd118f1 Compare February 19, 2025 11:26
@mrobinson
Copy link
Member

@chickenleaf I had pushed my changes to your branch and it seems that your latest push has overriden them and also added a new commit fixing the mouse interaction issue. I think we should go ahead and stick with 284a7ed for this change and then do the mouse interaction fixes in a new PR. I will repush to this branch and then at that point you just need to open a new PR with your changes.

@mrobinson mrobinson force-pushed the authentication-dialog branch from cd118f1 to 284a7ed Compare February 19, 2025 11:54
@mrobinson
Copy link
Member

Okay. I have pushed the changes again. Nothing more is needed in this PR from you @chickenleaf. Thanks!

@mrobinson mrobinson enabled auto-merge February 19, 2025 11:54
@mrobinson mrobinson added this pull request to the merge queue Feb 19, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 19, 2025
Comment on lines +393 to +394
let authentication_dialog = Dialog::new_authentication_dialog(authentication_request);
self.add_dialog(webview, authentication_dialog);
Copy link
Member

Choose a reason for hiding this comment

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

We need to make sure that when in headless mode we do not create the dialog. I'll make this change.

Copy link
Member

Choose a reason for hiding this comment

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

The reason here is that if we are in headless mode, the dialog will be added to the list of dialogs, but never resolved, since there is no UI for the user to resolve it. This headless mode is used for WPT test which is why landing this hit test failures.

…yfiledialogs

Signed-off-by: L Ashwin B <lashwinib@gmail.com>
@mrobinson mrobinson force-pushed the authentication-dialog branch from 284a7ed to d884688 Compare February 19, 2025 13:11
@mrobinson mrobinson enabled auto-merge February 19, 2025 13:12
@mrobinson mrobinson added this pull request to the merge queue Feb 19, 2025
Merged via the queue into servo:main with commit b0561c7 Feb 19, 2025
22 checks passed
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.

3 participants