Browse Source

Ladybird: Protect some application commands against non-Tab key windows

Currently, the only NSWindow type in the AppKit chrome is the Tab. Once
we have other window types (e.g. Inspector), commands which assume they
are used on a Tab will either crash or behave weirdly.

This changes the createNewTab: command to accept the tab from which the
new tab is created, rather than assuming that tab is the key window. So
if some JS on a page calls window.open() while a non-Tab window is key,
the new tab will be opened within the same tab group.

This also changes closeCurrentTab: to work on any key window. Regardless
of whether the key window is a Tab or some other window, pressing cmd+W
should just close that window.
Timothy Flynn 1 year ago
parent
commit
70830b711e

+ 1 - 1
Ladybird/AppKit/Application/ApplicationDelegate.h

@@ -24,8 +24,8 @@
                 withCookieJar:(Browser::CookieJar)cookie_jar
       webdriverContentIPCPath:(StringView)webdriver_content_ipc_path;
 
-- (nonnull TabController*)createNewTab:(Optional<URL> const&)url;
 - (nonnull TabController*)createNewTab:(Optional<URL> const&)url
+                               fromTab:(nullable Tab*)tab
                            activateTab:(Web::HTML::ActivateTab)activate_tab;
 
 - (void)removeTab:(nonnull TabController*)controller;

+ 15 - 15
Ladybird/AppKit/Application/ApplicationDelegate.mm

@@ -83,25 +83,18 @@
 #pragma mark - Public methods
 
 - (TabController*)createNewTab:(Optional<URL> const&)url
-{
-    return [self createNewTab:url activateTab:Web::HTML::ActivateTab::Yes];
-}
-
-- (TabController*)createNewTab:(Optional<URL> const&)url
+                       fromTab:(Tab*)tab
                    activateTab:(Web::HTML::ActivateTab)activate_tab
 {
-    // This handle must be acquired before creating the new tab.
-    auto* current_tab = (Tab*)[NSApp keyWindow];
-
     auto* controller = [[TabController alloc] init:url.value_or(m_new_tab_page_url)];
     [controller showWindow:nil];
 
-    if (current_tab) {
-        [[current_tab tabGroup] addWindow:controller.window];
+    if (tab) {
+        [[tab tabGroup] addWindow:controller.window];
 
         // FIXME: Can we create the tabbed window above without it becoming active in the first place?
         if (activate_tab == Web::HTML::ActivateTab::No) {
-            [current_tab orderFront:nil];
+            [tab orderFront:nil];
         }
     }
 
@@ -133,13 +126,18 @@
 
 - (void)closeCurrentTab:(id)sender
 {
-    auto* current_tab = (Tab*)[NSApp keyWindow];
-    [current_tab close];
+    auto* current_window = [NSApp keyWindow];
+    [current_window close];
 }
 
 - (void)openLocation:(id)sender
 {
-    auto* current_tab = (Tab*)[NSApp keyWindow];
+    auto* current_tab = [NSApp keyWindow];
+
+    if (![current_tab isKindOfClass:[Tab class]]) {
+        return;
+    }
+
     auto* controller = (TabController*)[current_tab windowController];
     [controller focusLocationToolbarItem];
 }
@@ -353,7 +351,9 @@
 
 - (void)applicationDidFinishLaunching:(NSNotification*)notification
 {
-    [self createNewTab:m_initial_url];
+    [self createNewTab:m_initial_url
+               fromTab:nil
+           activateTab:Web::HTML::ActivateTab::Yes];
 }
 
 - (void)applicationWillTerminate:(NSNotification*)notification

+ 16 - 6
Ladybird/AppKit/UI/LadybirdWebView.mm

@@ -178,9 +178,12 @@ struct HideCursor {
         [self setNeedsDisplay:YES];
     };
 
-    m_web_view_bridge->on_new_tab = [](auto activate_tab) {
+    m_web_view_bridge->on_new_tab = [self](auto activate_tab) {
         auto* delegate = (ApplicationDelegate*)[NSApp delegate];
-        auto* controller = [delegate createNewTab:"about:blank"sv activateTab:activate_tab];
+
+        auto* controller = [delegate createNewTab:"about:blank"sv
+                                          fromTab:[self tab]
+                                      activateTab:activate_tab];
 
         auto* tab = (Tab*)[controller window];
         auto* web_view = [tab web_view];
@@ -353,17 +356,24 @@ struct HideCursor {
         auto* delegate = (ApplicationDelegate*)[NSApp delegate];
 
         if (modifiers == Mod_Super) {
-            [delegate createNewTab:url activateTab:Web::HTML::ActivateTab::No];
+            [delegate createNewTab:url
+                           fromTab:[self tab]
+                       activateTab:Web::HTML::ActivateTab::No];
         } else if (target == "_blank"sv) {
-            [delegate createNewTab:url activateTab:Web::HTML::ActivateTab::Yes];
+            [delegate createNewTab:url
+                           fromTab:[self tab]
+                       activateTab:Web::HTML::ActivateTab::Yes];
         } else {
             [[self tabController] load:url];
         }
     };
 
-    m_web_view_bridge->on_link_middle_click = [](auto url, auto, unsigned) {
+    m_web_view_bridge->on_link_middle_click = [self](auto url, auto, unsigned) {
         auto* delegate = (ApplicationDelegate*)[NSApp delegate];
-        [delegate createNewTab:url activateTab:Web::HTML::ActivateTab::No];
+
+        [delegate createNewTab:url
+                       fromTab:[self tab]
+                   activateTab:Web::HTML::ActivateTab::No];
     };
 
     m_web_view_bridge->on_context_menu_request = [self](auto position) {

+ 4 - 1
Ladybird/AppKit/UI/TabController.mm

@@ -166,7 +166,10 @@ enum class IsHistoryNavigation {
 - (void)createNewTab:(id)sender
 {
     auto* delegate = (ApplicationDelegate*)[NSApp delegate];
-    [delegate createNewTab:OptionalNone {}];
+
+    [delegate createNewTab:OptionalNone {}
+                   fromTab:[self tab]
+               activateTab:Web::HTML::ActivateTab::Yes];
 }
 
 - (void)updateNavigationButtonStates