From cff8857a36fd5881863cc20700f02aabb528fc2c Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Mon, 26 May 2025 19:12:04 +0800 Subject: [PATCH 1/4] AddEventListener object listener Instead of taking a callback function, addEventListener can take an object that exposes a `handleEvent` function. When used this way, `this` is automatically bound. I don't think the current behavior is correct when `handleEvent` is defined as a property (getter), but I couldn't figure out how to make it work the way WPT expects, and it hopefully isn't a common usage pattern. Also added option support to removeEventListener. --- src/browser/dom/event_target.zig | 39 ++++++++++++++++++++++------ src/browser/events/event.zig | 19 +++++++++++++- src/browser/xhr/event_target.zig | 40 ++++++++++++++--------------- src/runtime/js.zig | 44 ++++++++++++++++++++++++-------- 4 files changed, 101 insertions(+), 41 deletions(-) diff --git a/src/browser/dom/event_target.zig b/src/browser/dom/event_target.zig index bd320385..79881daf 100644 --- a/src/browser/dom/event_target.zig +++ b/src/browser/dom/event_target.zig @@ -57,7 +57,7 @@ pub const EventTarget = struct { pub fn _addEventListener( self: *parser.EventTarget, typ: []const u8, - cbk: Env.Function, + listener: EventHandler.Listener, opts_: ?AddEventListenerOpts, page: *Page, ) !void { @@ -80,6 +80,8 @@ pub const EventTarget = struct { } } + const cbk = (try listener.callback(self)) orelse return; + // check if event target has already this listener const lst = try parser.eventTargetHasListener( self, @@ -90,8 +92,7 @@ pub const EventTarget = struct { if (lst != null) { return; } - - const eh = try EventHandler.init(page.arena, try cbk.withThis(self)); + const eh = try EventHandler.init(page.arena, cbk); try parser.eventTargetAddEventListener( self, @@ -101,19 +102,34 @@ pub const EventTarget = struct { ); } + const RemoveEventListenerOpts = union(enum) { + opts: Opts, + capture: bool, + + const Opts = struct { + capture: ?bool, + }; + }; + pub fn _removeEventListener( self: *parser.EventTarget, typ: []const u8, cbk: Env.Function, - capture: ?bool, - // TODO: hanle EventListenerOptions - // see #https://github.com/lightpanda-io/jsruntime-lib/issues/114 + opts_: ?RemoveEventListenerOpts, ) !void { + var capture = false; + if (opts_) |opts| { + capture = switch (opts) { + .capture => |c| c, + .opts => |o| o.capture orelse false, + }; + } + // check if event target has already this listener const lst = try parser.eventTargetHasListener( self, typ, - capture orelse false, + capture, cbk.id, ); if (lst == null) { @@ -125,7 +141,7 @@ pub const EventTarget = struct { self, typ, lst.?, - capture orelse false, + capture, ); } @@ -244,4 +260,11 @@ test "Browser.DOM.EventTarget" { .{ "phase", "3" }, .{ "cur.getAttribute('id')", "content" }, }, .{}); + + try runner.testCases(&.{ + .{ "const obj1 = {calls: 0, handleEvent: function() { this.calls += 1; } };", null }, + .{ "content.addEventListener('he', obj1);", null }, + .{ "content.dispatchEvent(new Event('he'));", null }, + .{ "obj1.calls", "1" }, + }, .{}); } diff --git a/src/browser/events/event.zig b/src/browser/events/event.zig index 59f1db0e..f4fddcfa 100644 --- a/src/browser/events/event.zig +++ b/src/browser/events/event.zig @@ -21,7 +21,6 @@ const Allocator = std.mem.Allocator; const log = @import("../../log.zig"); const parser = @import("../netsurf.zig"); -const Function = @import("../env.zig").Function; const generate = @import("../../runtime/generate.zig"); const DOMException = @import("../dom/exceptions.zig").DOMException; @@ -142,6 +141,24 @@ pub const EventHandler = struct { callback: Function, node: parser.EventNode, + const Env = @import("../env.zig").Env; + const Function = Env.Function; + + pub const Listener = union(enum) { + function: Function, + object: Env.JsObject, + + pub fn callback(self: Listener, target: *parser.EventTarget) !?Function { + return switch (self) { + .function => |func| try func.withThis(target), + .object => |obj| blk: { + const func = (try obj.getFunction("handleEvent")) orelse return null; + break :blk try func.withThis(try obj.persist()); + }, + }; + } + }; + pub fn init(allocator: Allocator, callback: Function) !*EventHandler { const eh = try allocator.create(EventHandler); eh.* = .{ diff --git a/src/browser/xhr/event_target.zig b/src/browser/xhr/event_target.zig index fe686973..57afd2bf 100644 --- a/src/browser/xhr/event_target.zig +++ b/src/browser/xhr/event_target.zig @@ -44,16 +44,20 @@ pub const XMLHttpRequestEventTarget = struct { self: *XMLHttpRequestEventTarget, alloc: std.mem.Allocator, typ: []const u8, - cbk: Function, - ) !void { + listener: EventHandler.Listener, + ) !?Function { const target = @as(*parser.EventTarget, @ptrCast(self)); - const eh = try EventHandler.init(alloc, try cbk.withThis(target)); + + const callback = (try listener.callback(target)) orelse return null; + const eh = try EventHandler.init(alloc, callback); try parser.eventTargetAddEventListener( target, typ, &eh.node, false, ); + + return callback; } fn unregister(self: *XMLHttpRequestEventTarget, typ: []const u8, cbk_id: usize) !void { const et = @as(*parser.EventTarget, @ptrCast(self)); @@ -86,34 +90,28 @@ pub const XMLHttpRequestEventTarget = struct { return self.onloadend_cbk; } - pub fn set_onloadstart(self: *XMLHttpRequestEventTarget, handler: Function, page: *Page) !void { + pub fn set_onloadstart(self: *XMLHttpRequestEventTarget, listener: EventHandler.Listener, page: *Page) !void { if (self.onloadstart_cbk) |cbk| try self.unregister("loadstart", cbk.id); - try self.register(page.arena, "loadstart", handler); - self.onloadstart_cbk = handler; + self.onloadstart_cbk = try self.register(page.arena, "loadstart", listener); } - pub fn set_onprogress(self: *XMLHttpRequestEventTarget, handler: Function, page: *Page) !void { + pub fn set_onprogress(self: *XMLHttpRequestEventTarget, listener: EventHandler.Listener, page: *Page) !void { if (self.onprogress_cbk) |cbk| try self.unregister("progress", cbk.id); - try self.register(page.arena, "progress", handler); - self.onprogress_cbk = handler; + self.onprogress_cbk = try self.register(page.arena, "progress", listener); } - pub fn set_onabort(self: *XMLHttpRequestEventTarget, handler: Function, page: *Page) !void { + pub fn set_onabort(self: *XMLHttpRequestEventTarget, listener: EventHandler.Listener, page: *Page) !void { if (self.onabort_cbk) |cbk| try self.unregister("abort", cbk.id); - try self.register(page.arena, "abort", handler); - self.onabort_cbk = handler; + self.onabort_cbk = try self.register(page.arena, "abort", listener); } - pub fn set_onload(self: *XMLHttpRequestEventTarget, handler: Function, page: *Page) !void { + pub fn set_onload(self: *XMLHttpRequestEventTarget, listener: EventHandler.Listener, page: *Page) !void { if (self.onload_cbk) |cbk| try self.unregister("load", cbk.id); - try self.register(page.arena, "load", handler); - self.onload_cbk = handler; + self.onload_cbk = try self.register(page.arena, "load", listener); } - pub fn set_ontimeout(self: *XMLHttpRequestEventTarget, handler: Function, page: *Page) !void { + pub fn set_ontimeout(self: *XMLHttpRequestEventTarget, listener: EventHandler.Listener, page: *Page) !void { if (self.ontimeout_cbk) |cbk| try self.unregister("timeout", cbk.id); - try self.register(page.arena, "timeout", handler); - self.ontimeout_cbk = handler; + self.ontimeout_cbk = try self.register(page.arena, "timeout", listener); } - pub fn set_onloadend(self: *XMLHttpRequestEventTarget, handler: Function, page: *Page) !void { + pub fn set_onloadend(self: *XMLHttpRequestEventTarget, listener: EventHandler.Listener, page: *Page) !void { if (self.onloadend_cbk) |cbk| try self.unregister("loadend", cbk.id); - try self.register(page.arena, "loadend", handler); - self.onloadend_cbk = handler; + self.onloadend_cbk = try self.register(page.arena, "loadend", listener); } }; diff --git a/src/runtime/js.zig b/src/runtime/js.zig index 56c5d8f8..755a2292 100644 --- a/src/runtime/js.zig +++ b/src/runtime/js.zig @@ -582,7 +582,7 @@ pub fn Env(comptime State: type, comptime WebApis: type) type { // Given an anytype, turns it into a v8.Object. The anytype could be: // 1 - A V8.object already - // 2 - Our this JsObject wrapper around a V8.Object + // 2 - Our JsObject wrapper around a V8.Object // 3 - A zig instance that has previously been given to V8 // (i.e., the value has to be known to the executor) fn valueToExistingObject(self: *const Scope, value: anytype) !v8.Object { @@ -951,15 +951,7 @@ pub fn Env(comptime State: type, comptime WebApis: type) type { if (!js_value.isFunction()) { return null; } - - const func = v8.Persistent(v8.Function).init(self.isolate, js_value.castTo(v8.Function)); - try self.trackCallback(func); - - return .{ - .func = func, - .scope = self, - .id = js_value.castTo(v8.Object).getIdentityHash(), - }; + return try self.createFunction(js_value); } const js_obj = js_value.castTo(v8.Object); @@ -996,6 +988,20 @@ pub fn Env(comptime State: type, comptime WebApis: type) type { return value; } + fn createFunction(self: *Scope, js_value: v8.Value) !Function { + // caller should have made sure this was a function + std.debug.assert(js_value.isFunction()); + + const func = v8.Persistent(v8.Function).init(self.isolate, js_value.castTo(v8.Function)); + try self.trackCallback(func); + + return .{ + .func = func, + .scope = self, + .id = js_value.castTo(v8.Object).getIdentityHash(), + }; + } + // Probing is part of trying to map a JS value to a Zig union. There's // a lot of ambiguity in this process, in part because some JS values // can almost always be coerced. For example, anything can be coerced @@ -1234,11 +1240,16 @@ pub fn Env(comptime State: type, comptime WebApis: type) type { }; pub fn withThis(self: *const Function, value: anytype) !Function { + const this_obj = if (@TypeOf(value) == JsObject) + value.js_obj + else + try self.scope.valueToExistingObject(value); + return .{ .id = self.id, + .this = this_obj, .func = self.func, .scope = self.scope, - .this = try self.scope.valueToExistingObject(value), }; } @@ -1375,6 +1386,17 @@ pub fn Env(comptime State: type, comptime WebApis: type) type { .js_obj = gop.value_ptr.castToObject(), }; } + + pub fn getFunction(self: JsObject, name: []const u8) !?Function { + const scope = self.scope; + const js_name = v8.String.initUtf8(scope.isolate, name); + + const js_value = try self.js_obj.getValue(scope.context, js_name.toName()); + if (!js_value.isFunction()) { + return null; + } + return try scope.createFunction(js_value); + } }; // This only exists so that we know whether a function wants the opaque From 99b7508c7a2570ed88968aa81a94ccf57371de1d Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Mon, 26 May 2025 19:19:39 +0800 Subject: [PATCH 2/4] support object listener on removeEventListener also --- src/browser/dom/event_target.zig | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/browser/dom/event_target.zig b/src/browser/dom/event_target.zig index 79881daf..fb21d424 100644 --- a/src/browser/dom/event_target.zig +++ b/src/browser/dom/event_target.zig @@ -114,7 +114,7 @@ pub const EventTarget = struct { pub fn _removeEventListener( self: *parser.EventTarget, typ: []const u8, - cbk: Env.Function, + listener: EventHandler.Listener, opts_: ?RemoveEventListenerOpts, ) !void { var capture = false; @@ -125,6 +125,8 @@ pub const EventTarget = struct { }; } + const cbk = (try listener.callback(self)) orelse return; + // check if event target has already this listener const lst = try parser.eventTargetHasListener( self, @@ -266,5 +268,9 @@ test "Browser.DOM.EventTarget" { .{ "content.addEventListener('he', obj1);", null }, .{ "content.dispatchEvent(new Event('he'));", null }, .{ "obj1.calls", "1" }, + + .{ "content.removeEventListener('he', obj1);", null }, + .{ "content.dispatchEvent(new Event('he'));", null }, + .{ "obj1.calls", "1" }, }, .{}); } From f0017c3e9263dc3d57828eecbfee9ad6542ff7ad Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Mon, 26 May 2025 21:58:27 +0800 Subject: [PATCH 3/4] No-op eventHandler's passive option This is a hint to the brower that the listener won't call preventDefault. In theory, we should enforce this. But in practice, ignoring it should be ok. --- src/browser/dom/event_target.zig | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/browser/dom/event_target.zig b/src/browser/dom/event_target.zig index fb21d424..e34e3690 100644 --- a/src/browser/dom/event_target.zig +++ b/src/browser/dom/event_target.zig @@ -48,8 +48,12 @@ pub const EventTarget = struct { const Opts = struct { capture: ?bool, + // We ignore this property. It seems to be largely used to help the + // browser make certain performance tweaks (i.e. the browser knows + // that the listener won't call preventDefault() and thus can safely + // run the default as needed). + passive: ?bool, once: ?bool, // currently does nothing - passive: ?bool, // currently does nothing signal: ?bool, // currently does nothing }; }; @@ -74,7 +78,6 @@ pub const EventTarget = struct { // better to be explicit and error. if (o.once orelse false) return error.NotImplemented; if (o.signal orelse false) return error.NotImplemented; - if (o.passive orelse false) return error.NotImplemented; capture = o.capture orelse false; }, } From 9ce3fc9f8e76589d59ee7c0a106a6b61cd1fa9b7 Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Tue, 27 May 2025 11:00:26 +0800 Subject: [PATCH 4/4] Refactor events Removes some duplication between xhr/event_target and dom/event_target. Implement 'once' option of addEventListener. --- src/browser/dom/event_target.zig | 58 +------------------ src/browser/dom/mutation_observer.zig | 10 ++-- src/browser/events/event.zig | 83 ++++++++++++++++++++++++++- src/browser/netsurf.zig | 4 +- src/browser/page.zig | 2 +- src/browser/xhr/event_target.zig | 17 +++--- 6 files changed, 100 insertions(+), 74 deletions(-) diff --git a/src/browser/dom/event_target.zig b/src/browser/dom/event_target.zig index e34e3690..acb6e934 100644 --- a/src/browser/dom/event_target.zig +++ b/src/browser/dom/event_target.zig @@ -41,68 +41,14 @@ pub const EventTarget = struct { // JS funcs // -------- - - const AddEventListenerOpts = union(enum) { - opts: Opts, - capture: bool, - - const Opts = struct { - capture: ?bool, - // We ignore this property. It seems to be largely used to help the - // browser make certain performance tweaks (i.e. the browser knows - // that the listener won't call preventDefault() and thus can safely - // run the default as needed). - passive: ?bool, - once: ?bool, // currently does nothing - signal: ?bool, // currently does nothing - }; - }; - pub fn _addEventListener( self: *parser.EventTarget, typ: []const u8, listener: EventHandler.Listener, - opts_: ?AddEventListenerOpts, + opts: ?EventHandler.Opts, page: *Page, ) !void { - var capture = false; - if (opts_) |opts| { - switch (opts) { - .capture => |c| capture = c, - .opts => |o| { - // Done this way so that, for common cases that _only_ set - // capture, i.e. {captrue: true}, it works. - // But for any case that sets any of the other flags, we - // error. If we don't error, this function call would succeed - // but the behavior might be wrong. At this point, it's - // better to be explicit and error. - if (o.once orelse false) return error.NotImplemented; - if (o.signal orelse false) return error.NotImplemented; - capture = o.capture orelse false; - }, - } - } - - const cbk = (try listener.callback(self)) orelse return; - - // check if event target has already this listener - const lst = try parser.eventTargetHasListener( - self, - typ, - capture, - cbk.id, - ); - if (lst != null) { - return; - } - const eh = try EventHandler.init(page.arena, cbk); - - try parser.eventTargetAddEventListener( - self, - typ, - &eh.node, - capture, - ); + _ = try EventHandler.register(page.arena, self, typ, listener, opts); } const RemoveEventListenerOpts = union(enum) { diff --git a/src/browser/dom/mutation_observer.zig b/src/browser/dom/mutation_observer.zig index c4deff88..c651c851 100644 --- a/src/browser/dom/mutation_observer.zig +++ b/src/browser/dom/mutation_observer.zig @@ -63,13 +63,13 @@ pub const MutationObserver = struct { // register node's events if (options.childList or options.subtree) { - try parser.eventTargetAddEventListener( + _ = try parser.eventTargetAddEventListener( parser.toEventTarget(parser.Node, node), "DOMNodeInserted", &observer.event_node, false, ); - try parser.eventTargetAddEventListener( + _ = try parser.eventTargetAddEventListener( parser.toEventTarget(parser.Node, node), "DOMNodeRemoved", &observer.event_node, @@ -77,7 +77,7 @@ pub const MutationObserver = struct { ); } if (options.attr()) { - try parser.eventTargetAddEventListener( + _ = try parser.eventTargetAddEventListener( parser.toEventTarget(parser.Node, node), "DOMAttrModified", &observer.event_node, @@ -85,7 +85,7 @@ pub const MutationObserver = struct { ); } if (options.cdata()) { - try parser.eventTargetAddEventListener( + _ = try parser.eventTargetAddEventListener( parser.toEventTarget(parser.Node, node), "DOMCharacterDataModified", &observer.event_node, @@ -93,7 +93,7 @@ pub const MutationObserver = struct { ); } if (options.subtree) { - try parser.eventTargetAddEventListener( + _ = try parser.eventTargetAddEventListener( parser.toEventTarget(parser.Node, node), "DOMSubtreeModified", &observer.event_node, diff --git a/src/browser/events/event.zig b/src/browser/events/event.zig index f4fddcfa..1dc7c422 100644 --- a/src/browser/events/event.zig +++ b/src/browser/events/event.zig @@ -138,8 +138,11 @@ pub const Event = struct { }; pub const EventHandler = struct { + once: bool, + capture: bool, callback: Function, node: parser.EventNode, + listener: *parser.EventListener, const Env = @import("../env.zig").Env; const Function = Env.Function; @@ -159,15 +162,73 @@ pub const EventHandler = struct { } }; - pub fn init(allocator: Allocator, callback: Function) !*EventHandler { + pub const Opts = union(enum) { + flags: Flags, + capture: bool, + + const Flags = struct { + once: ?bool, + capture: ?bool, + // We ignore this property. It seems to be largely used to help the + // browser make certain performance tweaks (i.e. the browser knows + // that the listener won't call preventDefault() and thus can safely + // run the default as needed). + passive: ?bool, + signal: ?bool, // currently does nothing + }; + }; + + pub fn register( + allocator: Allocator, + target: *parser.EventTarget, + typ: []const u8, + listener: Listener, + opts_: ?Opts, + ) !?*EventHandler { + var once = false; + var capture = false; + if (opts_) |opts| { + switch (opts) { + .capture => |c| capture = c, + .flags => |f| { + // Done this way so that, for common cases that _only_ set + // capture, i.e. {captrue: true}, it works. + // But for any case that sets any of the other flags, we + // error. If we don't error, this function call would succeed + // but the behavior might be wrong. At this point, it's + // better to be explicit and error. + if (f.signal orelse false) return error.NotImplemented; + once = f.once orelse false; + capture = f.capture orelse false; + }, + } + } + + const callback = (try listener.callback(target)) orelse return null; + + // check if event target has already this listener + if (try parser.eventTargetHasListener(target, typ, capture, callback.id) != null) { + return null; + } + const eh = try allocator.create(EventHandler); eh.* = .{ + .once = once, + .capture = capture, .callback = callback, .node = .{ .id = callback.id, .func = handle, }, + .listener = undefined, }; + + eh.listener = try parser.eventTargetAddEventListener( + target, + typ, + &eh.node, + capture, + ); return eh; } @@ -182,6 +243,17 @@ pub const EventHandler = struct { self.callback.tryCall(void, .{ievent}, &result) catch { log.debug(.event, "handle callback error", .{ .err = result.exception, .stack = result.stack }); }; + + if (self.once) { + const target = (parser.eventTarget(event) catch return).?; + const typ = parser.eventType(event) catch return; + parser.eventTargetRemoveEventListener( + target, + typ, + self.listener, + self.capture, + ) catch {}; + } } }; @@ -282,4 +354,13 @@ test "Browser.Event" { .{ "document.dispatchEvent(new Event('count'))", "true" }, .{ "nb", "0" }, }, .{}); + + try runner.testCases(&.{ + .{ "nb = 0; function cbk(event) { nb ++; }", null }, + .{ "document.addEventListener('count', cbk, {once: true})", null }, + .{ "document.dispatchEvent(new Event('count'))", "true" }, + .{ "document.dispatchEvent(new Event('count'))", "true" }, + .{ "document.dispatchEvent(new Event('count'))", "true" }, + .{ "nb", "1" }, + }, .{}); } diff --git a/src/browser/netsurf.zig b/src/browser/netsurf.zig index 756d8fb6..d83a6a85 100644 --- a/src/browser/netsurf.zig +++ b/src/browser/netsurf.zig @@ -615,7 +615,7 @@ pub fn eventTargetAddEventListener( typ: []const u8, node: *EventNode, capture: bool, -) !void { +) !*EventListener { const event_handler = struct { fn handle(event_: ?*Event, ptr_: ?*anyopaque) callconv(.C) void { const ptr = ptr_ orelse return; @@ -634,6 +634,8 @@ pub fn eventTargetAddEventListener( const s = try strFromData(typ); const err = eventTargetVtable(et).add_event_listener.?(et, s, listener, capture); try DOMErr(err); + + return listener.?; } pub fn eventTargetHasListener( diff --git a/src/browser/page.zig b/src/browser/page.zig index e5ac0d38..39a5279c 100644 --- a/src/browser/page.zig +++ b/src/browser/page.zig @@ -273,7 +273,7 @@ pub const Page = struct { const doc = parser.documentHTMLToDocument(html_doc); const document_element = (try parser.documentGetDocumentElement(doc)) orelse return error.DocumentElementError; - try parser.eventTargetAddEventListener( + _ = try parser.eventTargetAddEventListener( parser.toEventTarget(parser.Element, document_element), "click", &self.window_clicked_event_node, diff --git a/src/browser/xhr/event_target.zig b/src/browser/xhr/event_target.zig index 57afd2bf..8f4b9e28 100644 --- a/src/browser/xhr/event_target.zig +++ b/src/browser/xhr/event_target.zig @@ -48,17 +48,14 @@ pub const XMLHttpRequestEventTarget = struct { ) !?Function { const target = @as(*parser.EventTarget, @ptrCast(self)); - const callback = (try listener.callback(target)) orelse return null; - const eh = try EventHandler.init(alloc, callback); - try parser.eventTargetAddEventListener( - target, - typ, - &eh.node, - false, - ); - - return callback; + // The only time this can return null if the listener is already + // registered. But before calling `register`, all of our functions + // remove any existing listener, so it should be impossible to get null + // from this function call. + const eh = (try EventHandler.register(alloc, target, typ, listener, null)) orelse unreachable; + return eh.callback; } + fn unregister(self: *XMLHttpRequestEventTarget, typ: []const u8, cbk_id: usize) !void { const et = @as(*parser.EventTarget, @ptrCast(self)); // check if event target has already this listener