Merge pull request #1729 from lightpanda-io/target_navigation

Add target-aware(ish) navigation

src/browser/Page.zig

@@ -69,6 +69,7 @@ const ArenaPool = App.ArenaPool;
 const timestamp = @import("../datetime.zig").timestamp;
 const milliTimestamp = @import("../datetime.zig").milliTimestamp;
 
+const IFrame = Element.Html.IFrame;
 const WebApiURL = @import("webapi/URL.zig");
 const GlobalEventHandlersLookup = @import("webapi/global_event_handlers.zig").Lookup;
 
@@ -223,7 +224,7 @@ _arena_pool_leak_track: (if (IS_DEBUG) std.AutoHashMapUnmanaged(usize, struct {
 parent: ?*Page,
 window: *Window,
 document: *Document,
-iframe: ?*Element.Html.IFrame = null,
+iframe: ?*IFrame = null,
 frames: std.ArrayList(*Page) = .{},
 frames_sorted: bool = true,
 
@@ -572,62 +573,76 @@ pub fn navigate(self: *Page, request_url: [:0]const u8, opts: NavigateOpts) !voi
     };
 }
 
-// We cannot navigate immediately as navigating will delete the DOM tree,
+// Navigation can happen in many places, such as executing a <script> tag or
-// which holds this event's node.
+// a JavaScript callback, a CDP command, etc...It's rarely safe to do immediately
-// As such we schedule the function to be called as soon as possible.
+// as the caller almost certainly does'nt expect the page to go away during the
-pub fn scheduleNavigation(self: *Page, request_url: []const u8, opts: NavigateOpts, priority: NavigationPriority) !void {
+// call. So, we schedule the navigation for the next tick.
-    if (self.canScheduleNavigation(priority) == false) {
+pub fn scheduleNavigation(self: *Page, request_url: []const u8, opts: NavigateOpts, nt: Navigation) !void {
+    if (self.canScheduleNavigation(std.meta.activeTag(nt)) == false) {
         return;
     }
     const arena = try self.arena_pool.acquire();
     errdefer self.arena_pool.release(arena);
-    return self.scheduleNavigationWithArena(arena, request_url, opts, priority);
+    return self.scheduleNavigationWithArena(arena, request_url, opts, nt);
 }
 
-fn scheduleNavigationWithArena(self: *Page, arena: Allocator, request_url: []const u8, opts: NavigateOpts, priority: NavigationPriority) !void {
+// Don't name the first parameter "self", because the target of this navigation
+// might change inside the function. So the code should be explicit about the
+// page that it's acting on.
+fn scheduleNavigationWithArena(originator: *Page, arena: Allocator, request_url: []const u8, opts: NavigateOpts, nt: Navigation) !void {
     const resolved_url, const is_about_blank = blk: {
         if (std.mem.eql(u8, request_url, "about:blank")) {
-            break :blk .{ "about:blank", true }; // navigate will handle this special case
+            // navigate will handle this special case
+            break :blk .{ "about:blank", true };
         }
         const u = try URL.resolve(
             arena,
-            self.base(),
+            originator.base(),
             request_url,
             .{ .always_dupe = true, .encode = true },
         );
         break :blk .{ u, false };
     };
 
-    const session = self._session;
+    const target = switch (nt) {
-    if (!opts.force and URL.eqlDocument(self.url, resolved_url)) {
+        .script => |p| p orelse originator,
-        self.url = try self.arena.dupeZ(u8, resolved_url);
+        .iframe => |iframe| iframe._window.?._page, // only an frame with existing content (i.e. a window) can be navigated
-        self.window._location = try Location.init(self.url, self);
+        .anchor, .form => |node| blk: {
-        self.document._location = self.window._location;
+            const doc = node.ownerDocument(originator) orelse break :blk originator;
-        if (self.parent == null) {
+            break :blk doc._page orelse originator;
-            try session.navigation.updateEntries(self.url, opts.kind, self, true);
+        },
+    };
+
+    const session = target._session;
+    if (!opts.force and URL.eqlDocument(target.url, resolved_url)) {
+        target.url = try target.arena.dupeZ(u8, resolved_url);
+        target.window._location = try Location.init(target.url, target);
+        target.document._location = target.window._location;
+        if (target.parent == null) {
+            try session.navigation.updateEntries(target.url, opts.kind, target, true);
         }
         // doin't defer this, the caller, the caller is responsible for freeing
         // it on error
-        self.arena_pool.release(arena);
+        target.arena_pool.release(arena);
         return;
     }
 
     log.info(.browser, "schedule navigation", .{
         .url = resolved_url,
         .reason = opts.reason,
-        .type = self._type,
+        .type = target._type,
     });
 
     // This is a micro-optimization. Terminate any inflight request as early
     // as we can. This will be more propery shutdown when we process the
     // scheduled navigation.
-    if (self.parent == null) {
+    if (target.parent == null) {
         session.browser.http_client.abort();
     } else {
         // This doesn't terminate any inflight requests for nested frames, but
         // again, this is just an optimization. We'll correctly shut down all
         // nested inflight requests when we process the navigation.
-        session.browser.http_client.abortFrame(self._frame_id);
+        session.browser.http_client.abortFrame(target._frame_id);
     }
 
     const qn = try arena.create(QueuedNavigation);
@@ -635,12 +650,12 @@ fn scheduleNavigationWithArena(self: *Page, arena: Allocator, request_url: []con
         .opts = opts,
         .arena = arena,
         .url = resolved_url,
-        .priority = priority,
         .is_about_blank = is_about_blank,
+        .navigation_type = std.meta.activeTag(nt),
     };
 
-    self._queued_navigation = qn;
+    target._queued_navigation = qn;
-    return session.scheduleNavigation(self);
+    return session.scheduleNavigation(target);
 }
 
 // A script can have multiple competing navigation events, say it starts off
@@ -648,30 +663,31 @@ fn scheduleNavigationWithArena(self: *Page, arena: Allocator, request_url: []con
 // You might think that we just stop at the first one, but that doesn't seem
 // to be what browsers do, and it isn't particularly well supported by v8 (i.e.
 // halting execution mid-script).
-// From what I can tell, there are 3 "levels" of priority, in order:
+// From what I can tell, there are 4 "levels" of priority, in order:
 // 1 - form submission
 // 2 - JavaScript apis (e.g. top.location)
 // 3 - anchor clicks
+// 4 - iframe.src =
 // Within, each category, it's last-one-wins.
-fn canScheduleNavigation(self: *Page, priority: NavigationPriority) bool {
+fn canScheduleNavigation(self: *Page, new_target_type: NavigationType) bool {
     if (self.parent) |parent| {
         if (parent.isGoingAway()) {
             return false;
         }
     }
 
-    const existing = self._queued_navigation orelse return true;
+    const existing_target_type = (self._queued_navigation orelse return true).navigation_type;
 
-    if (existing.priority == priority) {
+    if (existing_target_type == new_target_type) {
         // same reason, than this latest one wins
         return true;
     }
 
-    return switch (existing.priority) {
+    return switch (existing_target_type) {
         .iframe => true, // everything is higher priority than iframe.src = "x"
-        .anchor => priority != .iframe, // an anchor is only higher priority than an iframe
+        .anchor => new_target_type != .iframe, // an anchor is only higher priority than an iframe
         .form => false, // nothing is higher priority than a form
-        .script => priority == .form, // a form is higher priority than a script
+        .script => new_target_type == .form, // a form is higher priority than a script
     };
 }
 
@@ -703,7 +719,7 @@ pub fn scriptsCompletedLoading(self: *Page) void {
     self.pendingLoadCompleted();
 }
 
-pub fn iframeCompletedLoading(self: *Page, iframe: *Element.Html.IFrame) void {
+pub fn iframeCompletedLoading(self: *Page, iframe: *IFrame) void {
     blk: {
         var ls: JS.Local.Scope = undefined;
         self.js.localScope(&ls);
@@ -1010,7 +1026,7 @@ pub fn scriptAddedCallback(self: *Page, comptime from_parser: bool, script: *Ele
     };
 }
 
-pub fn iframeAddedCallback(self: *Page, iframe: *Element.Html.IFrame) !void {
+pub fn iframeAddedCallback(self: *Page, iframe: *IFrame) !void {
     if (self.isGoingAway()) {
         // if we're planning on navigating to another page, don't load this iframe
         return;
@@ -1024,16 +1040,16 @@ pub fn iframeAddedCallback(self: *Page, iframe: *Element.Html.IFrame) !void {
         return;
     }
 
-    if (iframe._content_window) |cw| {
+    if (iframe._window != null) {
         // This frame is being re-navigated. We need to do this through a
         // scheduleNavigation phase. We can't navigate immediately here, for
         // the same reason that a "root" page can't immediately navigate:
         // we could be in the middle of a JS callback or something else that
         // doesn't exit the page to just suddenly go away.
-        return cw._page.scheduleNavigation(src, .{
+        return self.scheduleNavigation(src, .{
             .reason = .script,
             .kind = .{ .push = null },
-        }, .iframe);
+        }, .{ .iframe = iframe });
     }
 
     iframe._executed = true;
@@ -1047,8 +1063,8 @@ pub fn iframeAddedCallback(self: *Page, iframe: *Element.Html.IFrame) !void {
 
     self._pending_loads += 1;
     page_frame.iframe = iframe;
-    iframe._content_window = page_frame.window;
+    iframe._window = page_frame.window;
-    errdefer iframe._content_window = null;
+    errdefer iframe._window = null;
 
     // on first load, dispatch frame_created evnet
     self._session.notification.dispatch(.page_frame_created, &.{
@@ -1072,7 +1088,7 @@ pub fn iframeAddedCallback(self: *Page, iframe: *Element.Html.IFrame) !void {
     page_frame.navigate(url, .{ .reason = .initialFrameNavigation }) catch |err| {
         log.warn(.page, "iframe navigate failure", .{ .url = url, .err = err });
         self._pending_loads -= 1;
-        iframe._content_window = null;
+        iframe._window = null;
         page_frame.deinit(true);
         return error.IFrameLoadError;
     };
@@ -1992,7 +2008,7 @@ pub fn createElementNS(self: *Page, namespace: Element.Namespace, name: []const
                         .{ ._proto = undefined },
                     ),
                     asUint("iframe") => return self.createHtmlElementT(
-                        Element.Html.IFrame,
+                        IFrame,
                         namespace,
                         attribute_iterator,
                         .{ ._proto = undefined },
@@ -2903,7 +2919,7 @@ fn nodeIsReady(self: *Page, comptime from_parser: bool, node: *Node) !void {
             log.err(.page, "page.nodeIsReady", .{ .err = err, .element = "script", .type = self._type, .url = self.url });
             return err;
         };
-    } else if (node.is(Element.Html.IFrame)) |iframe| {
+    } else if (node.is(IFrame)) |iframe| {
         if ((comptime from_parser == false) and iframe._src.len == 0) {
             // iframe was added via JavaScript, but without a src
             return;
@@ -3049,19 +3065,26 @@ pub const NavigatedOpts = struct {
     method: Http.Method = .GET,
 };
 
-const NavigationPriority = enum {
+const NavigationType = enum {
     form,
     script,
     anchor,
     iframe,
 };
 
+const Navigation = union(NavigationType) {
+    form: *Node,
+    script: ?*Page,
+    anchor: *Node,
+    iframe: *IFrame,
+};
+
 pub const QueuedNavigation = struct {
     arena: Allocator,
     url: [:0]const u8,
     opts: NavigateOpts,
-    priority: NavigationPriority,
     is_about_blank: bool,
+    navigation_type: NavigationType,
 };
 
 pub fn triggerMouseClick(self: *Page, x: f64, y: f64) !void {
@@ -3114,11 +3137,17 @@ pub fn handleClick(self: *Page, target: *Node) !void {
                 return;
             }
 
+            // TODO: We need to support targets properly, but this is the most
+            // common case: a click on an anchor navigates the page/frame that
+            // anchor is in.
+
+            // ownerDocument only returns null when `target` is a document, which
+            // it is NOT in this case. Even for a detched node, it'll return self.document
             try element.focus(self);
             try self.scheduleNavigation(href, .{
                 .reason = .script,
                 .kind = .{ .push = null },
-            }, .anchor);
+            }, .{ .anchor = target });
         },
         .input => |input| {
             try element.focus(self);
@@ -3253,7 +3282,7 @@ pub fn submitForm(self: *Page, submitter_: ?*Element, form_: ?*Element.Html.Form
     } else {
         action = try URL.concatQueryString(arena, action, buf.written());
     }
-    return self.scheduleNavigationWithArena(arena, action, opts, .form);
+    return self.scheduleNavigationWithArena(arena, action, opts, .{ .form = form_element.asNode() });
 }
 
 // insertText is a shortcut to insert text into the active element.

src/browser/Session.zig

@@ -416,7 +416,7 @@ fn processQueuedNavigation(self: *Session) !void {
         const page = navigations.items[i];
         if (page._queued_navigation) |qn| {
             if (qn.is_about_blank) {
-                log.warn(.page, "recursive about blank", .{});
+                log.warn(.page, "recursive about    blank", .{});
                 _ = navigations.swapRemove(i);
                 continue;
             }
@@ -425,50 +425,37 @@ fn processQueuedNavigation(self: *Session) !void {
     }
 }
 
-fn processFrameNavigation(self: *Session, current_page: *Page, qn: *QueuedNavigation) !void {
+fn processFrameNavigation(self: *Session, page: *Page, qn: *QueuedNavigation) !void {
-    lp.assert(current_page.parent != null, "root queued navigation", .{});
+    lp.assert(page.parent != null, "root queued navigation", .{});
 
     const browser = self.browser;
-    const iframe = current_page.iframe.?;
+    const iframe = page.iframe.?;
-    const parent = current_page.parent.?;
+    const parent = page.parent.?;
 
-    current_page._queued_navigation = null;
+    page._queued_navigation = null;
     defer browser.arena_pool.release(qn.arena);
 
-    errdefer iframe._content_window = null;
+    errdefer iframe._window = null;
 
-    if (current_page._parent_notified) {
+    if (page._parent_notified) {
         // we already notified the parent that we had loaded
         parent._pending_loads += 1;
     }
 
-    const frame_id = current_page._frame_id;
+    const frame_id = page._frame_id;
-    defer current_page.deinit(true);
+    page.deinit(true);
+    page.* = undefined;
 
-    const new_page = try parent.arena.create(Page);
+    try Page.init(page, frame_id, self, parent);
-    try Page.init(new_page, frame_id, self, parent);
+    errdefer page.deinit(true);
-    errdefer new_page.deinit(true);
 
-    new_page.iframe = iframe;
+    page.iframe = iframe;
-    iframe._content_window = new_page.window;
+    iframe._window = page.window;
 
-    new_page.navigate(qn.url, qn.opts) catch |err| {
+    page.navigate(qn.url, qn.opts) catch |err| {
         log.err(.browser, "queued frame navigation error", .{ .err = err });
         return err;
     };
-
-    for (parent.frames.items, 0..) |p, i| {
-        // Page.frames may or may not be sorted (depending on the
-        // Page.frames_sorted flag). Putting this new page at the same
-        // position as the one it's replacing is the simplest, safest and
-        // probably most efficient option.
-        if (p == current_page) {
-            parent.frames.items[i] = new_page;
-            break;
-        }
-    } else {
-        lp.assert(false, "Existing frame not found", .{ .len = parent.frames.items.len });
-    }
 }
 
 fn processRootQueuedNavigation(self: *Session) !void {

src/browser/js/Context.zig

@@ -153,7 +153,7 @@ pub fn fromIsolate(isolate: js.Isolate) *Context {
 }
 
 pub fn deinit(self: *Context) void {
-    if (comptime IS_DEBUG) {
+    if (comptime IS_DEBUG and @import("builtin").is_test == false) {
         var it = self.unknown_properties.iterator();
         while (it.next()) |kv| {
             log.debug(.unknown_prop, "unknown property", .{

src/browser/tests/frames/frames.html

@@ -13,6 +13,7 @@
 <script id="basic">
    // reload it
   $('#f2').src = 'support/sub2.html';
+  testing.expectEqual(true, true);
 
   testing.eventually(() => {
     testing.expectEqual(undefined, window[10]);
@@ -103,29 +104,29 @@
   }
 </script>
 
-<!--
-Need correct _target support
 <script id=link_click>
-  {
+  testing.async(async (restore) => {
-    let f6 = document.createElement('iframe');
+    await new Promise((resolve) => {
-    f6.id = 'f6';
+      let count = 0;
-    f6.addEventListener('load', () => {
+      let f6 = document.createElement('iframe');
-      f6.contentDocument.querySelector('#link').click();
+      f6.id = 'f6';
-    }, {once: true});
+      f6.addEventListener('load', () => {
-    f6.src = "support/with_link.html";
+        if (++count == 2) {
-    document.documentElement.appendChild(f6);
+          resolve();
-
+          return;
-    testing.eventually(() => {
+        }
-      console.warn(f6.contentDocument);
+        f6.contentDocument.querySelector('#link').click();
-      testing.expectEqual("<html><head></head><body></body></html>", f6.contentDocument.documentElement.outerHTML);
+      });
+      f6.src = "support/with_link.html";
+      document.documentElement.appendChild(f6);
     });
-  }
+    restore();
-</script>
+    testing.expectEqual("<html><head></head><body>It was clicked!\n</body></html>", f6.contentDocument.documentElement.outerHTML);
- -->
-
-<script id=count>
-  testing.eventually(() => {
-    testing.expectEqual(5, window.length);
   });
 </script>
 
+<script id=count>
+  testing.eventually(() => {
+    testing.expectEqual(6, window.length);
+  });
+</script>

src/browser/tests/frames/support/with_link.html

@@ -1,2 +1,2 @@
 <!DOCTYPE html>
-<a href="after_link.html" id=link>a link</a>
+<a href="support/after_link.html" id=link>a link</a>

src/browser/webapi/HTMLDocument.zig

@@ -180,8 +180,8 @@ pub fn getLocation(self: *const HTMLDocument) ?*@import("Location.zig") {
     return self._proto._location;
 }
 
-pub fn setLocation(_: *const HTMLDocument, url: [:0]const u8, page: *Page) !void {
+pub fn setLocation(self: *HTMLDocument, url: [:0]const u8, page: *Page) !void {
-    return page.scheduleNavigation(url, .{ .reason = .script, .kind = .{ .push = null } }, .script);
+    return page.scheduleNavigation(url, .{ .reason = .script, .kind = .{ .push = null } }, .{ .script = self._proto._page });
 }
 
 pub fn getAll(self: *HTMLDocument, page: *Page) !*collections.HTMLAllCollection {

src/browser/webapi/Location.zig

@@ -83,19 +83,19 @@ pub fn setHash(_: *const Location, hash: []const u8, page: *Page) !void {
     return page.scheduleNavigation(normalized_hash, .{
         .reason = .script,
         .kind = .{ .replace = null },
-    }, .script);
+    }, .{ .script = page });
 }
 
 pub fn assign(_: *const Location, url: [:0]const u8, page: *Page) !void {
-    return page.scheduleNavigation(url, .{ .reason = .script, .kind = .{ .push = null } }, .script);
+    return page.scheduleNavigation(url, .{ .reason = .script, .kind = .{ .push = null } }, .{ .script = page });
 }
 
 pub fn replace(_: *const Location, url: [:0]const u8, page: *Page) !void {
-    return page.scheduleNavigation(url, .{ .reason = .script, .kind = .{ .replace = null } }, .script);
+    return page.scheduleNavigation(url, .{ .reason = .script, .kind = .{ .replace = null } }, .{ .script = page });
 }
 
 pub fn reload(_: *const Location, page: *Page) !void {
-    return page.scheduleNavigation(page.url, .{ .reason = .script, .kind = .reload }, .script);
+    return page.scheduleNavigation(page.url, .{ .reason = .script, .kind = .reload }, .{ .script = page });
 }
 
 pub fn toString(self: *const Location, page: *const Page) ![:0]const u8 {

src/browser/webapi/Window.zig

@@ -160,8 +160,8 @@ pub fn getSelection(self: *const Window) *Selection {
     return &self._document._selection;
 }
 
-pub fn setLocation(_: *const Window, url: [:0]const u8, page: *Page) !void {
+pub fn setLocation(self: *Window, url: [:0]const u8, page: *Page) !void {
-    return page.scheduleNavigation(url, .{ .reason = .script, .kind = .{ .push = null } }, .script);
+    return page.scheduleNavigation(url, .{ .reason = .script, .kind = .{ .push = null } }, .{ .script = self._page });
 }
 
 pub fn getHistory(_: *Window, page: *Page) *History {

src/browser/webapi/element/html/IFrame.zig

@@ -30,7 +30,7 @@ const IFrame = @This();
 _proto: *HtmlElement,
 _src: []const u8 = "",
 _executed: bool = false,
-_content_window: ?*Window = null,
+_window: ?*Window = null,
 
 pub fn asElement(self: *IFrame) *Element {
     return self._proto._proto;
@@ -40,11 +40,11 @@ pub fn asNode(self: *IFrame) *Node {
 }
 
 pub fn getContentWindow(self: *const IFrame) ?*Window {
-    return self._content_window;
+    return self._window;
 }
 
 pub fn getContentDocument(self: *const IFrame) ?*Document {
-    const window = self._content_window orelse return null;
+    const window = self._window orelse return null;
     return window._document;
 }
 

src/browser/webapi/navigation/Navigation.zig

@@ -308,7 +308,7 @@ pub fn navigateInner(
 
                 _ = try self.pushEntry(url, .{ .source = .navigation, .value = state }, page, true);
             } else {
-                try page.scheduleNavigation(url, .{ .reason = .navigation, .kind = kind }, .script);
+                try page.scheduleNavigation(url, .{ .reason = .navigation, .kind = kind }, .{ .script = page });
             }
         },
         .replace => |state| {
@@ -321,7 +321,7 @@ pub fn navigateInner(
 
                 _ = try self.replaceEntry(url, .{ .source = .navigation, .value = state }, page, true);
             } else {
-                try page.scheduleNavigation(url, .{ .reason = .navigation, .kind = kind }, .script);
+                try page.scheduleNavigation(url, .{ .reason = .navigation, .kind = kind }, .{ .script = page });
             }
         },
         .traverse => |index| {
@@ -334,11 +334,11 @@ pub fn navigateInner(
                 // todo: Fire navigate event
                 finished.resolve("navigation traverse", {});
             } else {
-                try page.scheduleNavigation(url, .{ .reason = .navigation, .kind = kind }, .script);
+                try page.scheduleNavigation(url, .{ .reason = .navigation, .kind = kind }, .{ .script = page });
             }
         },
         .reload => {
-            try page.scheduleNavigation(url, .{ .reason = .navigation, .kind = kind }, .script);
+            try page.scheduleNavigation(url, .{ .reason = .navigation, .kind = kind }, .{ .script = page });
         },
     }