diff --git a/src/browser/Page.zig b/src/browser/Page.zig index f2cab39b..1b5e5cfc 100644 --- a/src/browser/Page.zig +++ b/src/browser/Page.zig @@ -995,21 +995,32 @@ pub fn domChanged(self: *Page) void { }; } -fn getElementIdMap(page: *Page, node: *Node) *std.StringHashMapUnmanaged(*Element) { +const ElementIdMaps = struct { lookup: *std.StringHashMapUnmanaged(*Element), removed_ids: *std.StringHashMapUnmanaged(void) }; + +fn getElementIdMap(page: *Page, node: *Node) ElementIdMaps { // Walk up the tree checking for ShadowRoot and tracking the root var current = node; while (true) { if (current.is(ShadowRoot)) |shadow_root| { - return &shadow_root._elements_by_id; + return .{ + .lookup = &shadow_root._elements_by_id, + .removed_ids = &shadow_root._removed_ids, + }; } const parent = current._parent orelse { if (current._type == .document) { - return ¤t._type.document._elements_by_id; + return .{ + .lookup = ¤t._type.document._elements_by_id, + .removed_ids = ¤t._type.document._removed_ids, + }; } // Detached nodes should not have IDs registered std.debug.assert(false); - return &page.document._elements_by_id; + return .{ + .lookup = &page.document._elements_by_id, + .removed_ids = &page.document._removed_ids, + }; }; current = parent; @@ -1017,22 +1028,35 @@ fn getElementIdMap(page: *Page, node: *Node) *std.StringHashMapUnmanaged(*Elemen } pub fn addElementId(self: *Page, parent: *Node, element: *Element, id: []const u8) !void { - var id_map = self.getElementIdMap(parent); - const gop = try id_map.getOrPut(self.arena, id); + var id_maps = self.getElementIdMap(parent); + const gop = try id_maps.lookup.getOrPut(self.arena, id); if (!gop.found_existing) { gop.value_ptr.* = element; + return; + } + + const existing = gop.value_ptr.*.asNode(); + switch (element.asNode().compareDocumentPosition(existing)) { + 0x04 => gop.value_ptr.* = element, + else => {}, } } pub fn removeElementId(self: *Page, element: *Element, id: []const u8) void { - var id_map = self.getElementIdMap(element.asNode()); - _ = id_map.remove(id); + const node = element.asNode(); + self.removeElementIdWithMaps(self.getElementIdMap(node), id); +} + +pub fn removeElementIdWithMaps(self: *Page, id_maps: ElementIdMaps, id: []const u8) void { + if (id_maps.lookup.remove(id)) { + id_maps.removed_ids.put(self.arena, id, {}) catch {}; + } } pub fn getElementByIdFromNode(self: *Page, node: *Node, id: []const u8) ?*Element { if (node.isConnected() or node.isInShadowTree()) { - const id_map = self.getElementIdMap(node); - return id_map.get(id); + const lookup = self.getElementIdMap(node).lookup; + return lookup.get(id); } var tw = @import("webapi/TreeWalker.zig").Full.Elements.init(node, .{}); while (tw.next()) |el| { @@ -2097,7 +2121,7 @@ pub fn removeNode(self: *Page, parent: *Node, child: *Node, opts: RemoveNodeOpts // grab this before we null the parent const was_connected = child.isConnected(); // Capture the ID map before disconnecting, so we can remove IDs from the correct document - const id_map = if (was_connected) self.getElementIdMap(child) else null; + const id_maps = if (was_connected) self.getElementIdMap(child) else null; child._parent = null; child._child_link = .{}; @@ -2148,7 +2172,7 @@ pub fn removeNode(self: *Page, parent: *Node, child: *Node, opts: RemoveNodeOpts var tw = @import("webapi/TreeWalker.zig").Full.Elements.init(child, .{}); while (tw.next()) |el| { if (el.getAttributeSafe("id")) |id| { - _ = id_map.?.remove(id); + self.removeElementIdWithMaps(id_maps.?, id); } Element.Html.Custom.invokeDisconnectedCallbackOnElement(el, self); diff --git a/src/browser/js/ExecutionWorld.zig b/src/browser/js/ExecutionWorld.zig index b4a45f78..0dccd76e 100644 --- a/src/browser/js/ExecutionWorld.zig +++ b/src/browser/js/ExecutionWorld.zig @@ -184,9 +184,10 @@ pub fn unknownPropertyCallback(c_name: ?*const v8.C_Name, raw_info: ?*const v8.C if (maybe_property) |prop| { if (!ignored.has(prop)) { - const document = context.page.document; + const page = context.page; + const document = page.document; - if (document.getElementById(prop)) |el| { + if (document.getElementById(prop, page)) |el| { const js_value = context.zigValueToJs(el, .{}) catch { return v8.Intercepted.No; }; diff --git a/src/browser/tests/element/duplicate_ids.html b/src/browser/tests/element/duplicate_ids.html new file mode 100644 index 00000000..52ef7da6 --- /dev/null +++ b/src/browser/tests/element/duplicate_ids.html @@ -0,0 +1,19 @@ + + + +
first
+
second
+ + diff --git a/src/browser/webapi/Document.zig b/src/browser/webapi/Document.zig index 8b6c9e76..bbf56655 100644 --- a/src/browser/webapi/Document.zig +++ b/src/browser/webapi/Document.zig @@ -48,6 +48,8 @@ _location: ?*Location = null, _ready_state: ReadyState = .loading, _current_script: ?*Element.Html.Script = null, _elements_by_id: std.StringHashMapUnmanaged(*Element) = .empty, +// Track IDs that were removed from the map - they might have duplicates in the tree +_removed_ids: std.StringHashMapUnmanaged(void) = .empty, _active_element: ?*Element = null, _style_sheets: ?*StyleSheetList = null, _write_insertion_point: ?*Node = null, @@ -163,11 +165,32 @@ pub fn createAttributeNS(_: *const Document, namespace: []const u8, name: []cons }); } -pub fn getElementById(self: *const Document, id: []const u8) ?*Element { +pub fn getElementById(self: *Document, id: []const u8, page: *Page) ?*Element { if (id.len == 0) { return null; } - return self._elements_by_id.get(id); + + if (self._elements_by_id.get(id)) |element| { + return element; + } + + //ID was removed but might have duplicates + if (self._removed_ids.remove(id)) { + var tw = @import("TreeWalker.zig").Full.Elements.init(self.asNode(), .{}); + while (tw.next()) |el| { + const element_id = el.getAttributeSafe("id") orelse continue; + if (std.mem.eql(u8, element_id, id)) { + // we ignore this error to keep getElementById easy to call + // if it really failed, then we're out of memory and nothing's + // going to work like it should anyways. + const owned_id = page.dupeString(id) catch return null; + self._elements_by_id.put(page.arena, owned_id, el) catch return null; + return el; + } + } + } + + return null; } const GetElementsByTagNameResult = union(enum) { @@ -688,15 +711,15 @@ pub const JsApi = struct { pub const createTreeWalker = bridge.function(Document.createTreeWalker, .{}); pub const createNodeIterator = bridge.function(Document.createNodeIterator, .{}); pub const getElementById = bridge.function(_getElementById, .{}); - fn _getElementById(self: *const Document, value_: ?js.Value) !?*Element { + fn _getElementById(self: *Document, value_: ?js.Value, page: *Page) !?*Element { const value = value_ orelse return null; if (value.isNull()) { - return self.getElementById("null"); + return self.getElementById("null", page); } if (value.isUndefined()) { - return self.getElementById("undefined"); + return self.getElementById("undefined", page); } - return self.getElementById(try value.toZig([]const u8)); + return self.getElementById(try value.toZig([]const u8), page); } pub const querySelector = bridge.function(Document.querySelector, .{ .dom_exception = true }); pub const querySelectorAll = bridge.function(Document.querySelectorAll, .{ .dom_exception = true }); diff --git a/src/browser/webapi/DocumentFragment.zig b/src/browser/webapi/DocumentFragment.zig index be2b841a..65dd0af1 100644 --- a/src/browser/webapi/DocumentFragment.zig +++ b/src/browser/webapi/DocumentFragment.zig @@ -71,8 +71,10 @@ pub fn className(_: *const DocumentFragment) []const u8 { return "[object DocumentFragment]"; } -pub fn getElementById(self: *DocumentFragment, id_: ?[]const u8) ?*Element { - const id = id_ orelse return null; +pub fn getElementById(self: *DocumentFragment, id: []const u8) ?*Element { + if (id.len == 0) { + return null; + } var tw = @import("TreeWalker.zig").Full.Elements.init(self.asNode(), .{}); while (tw.next()) |el| { @@ -239,7 +241,18 @@ pub const JsApi = struct { pub const constructor = bridge.constructor(DocumentFragment.init, .{}); - pub const getElementById = bridge.function(DocumentFragment.getElementById, .{}); + pub const getElementById = bridge.function(_getElementById, .{}); + fn _getElementById(self: *DocumentFragment, value_: ?js.Value) !?*Element { + const value = value_ orelse return null; + if (value.isNull()) { + return self.getElementById("null"); + } + if (value.isUndefined()) { + return self.getElementById("undefined"); + } + return self.getElementById(try value.toZig([]const u8)); + } + pub const querySelector = bridge.function(DocumentFragment.querySelector, .{ .dom_exception = true }); pub const querySelectorAll = bridge.function(DocumentFragment.querySelectorAll, .{ .dom_exception = true }); pub const children = bridge.accessor(DocumentFragment.getChildren, null, .{}); diff --git a/src/browser/webapi/ShadowRoot.zig b/src/browser/webapi/ShadowRoot.zig index 9fb11c07..022eec71 100644 --- a/src/browser/webapi/ShadowRoot.zig +++ b/src/browser/webapi/ShadowRoot.zig @@ -39,6 +39,7 @@ _proto: *DocumentFragment, _mode: Mode, _host: *Element, _elements_by_id: std.StringHashMapUnmanaged(*Element) = .{}, +_removed_ids: std.StringHashMapUnmanaged(void) = .{}, pub fn init(host: *Element, mode: Mode, page: *Page) !*ShadowRoot { return page._factory.documentFragment(ShadowRoot{ @@ -72,9 +73,34 @@ pub fn getHost(self: *const ShadowRoot) *Element { return self._host; } -pub fn getElementById(self: *ShadowRoot, id_: ?[]const u8) ?*Element { - const id = id_ orelse return null; - return self._elements_by_id.get(id); +pub fn getElementById(self: *ShadowRoot, id: []const u8, page: *Page) ?*Element { + if (id.len == 0) { + return null; + } + + // Fast path: ID is in the map + if (self._elements_by_id.get(id)) |element| { + return element; + } + + // Slow path: ID was removed but might have duplicates + if (self._removed_ids.remove(id)) { + // Do a tree walk to find another element with this ID + var tw = @import("TreeWalker.zig").Full.Elements.init(self.asNode(), .{}); + while (tw.next()) |el| { + const element_id = el.getAttributeSafe("id") orelse continue; + if (std.mem.eql(u8, element_id, id)) { + // we ignore this error to keep getElementById easy to call + // if it really failed, then we're out of memory and nothing's + // going to work like it should anyways. + const owned_id = page.dupeString(id) catch return null; + self._elements_by_id.put(page.arena, owned_id, el) catch return null; + return el; + } + } + } + + return null; } pub const JsApi = struct { @@ -88,7 +114,17 @@ pub const JsApi = struct { pub const mode = bridge.accessor(ShadowRoot.getMode, null, .{}); pub const host = bridge.accessor(ShadowRoot.getHost, null, .{}); - pub const getElementById = bridge.function(ShadowRoot.getElementById, .{}); + pub const getElementById = bridge.function(_getElementById, .{}); + fn _getElementById(self: *ShadowRoot, value_: ?js.Value, page: *Page) !?*Element { + const value = value_ orelse return null; + if (value.isNull()) { + return self.getElementById("null", page); + } + if (value.isUndefined()) { + return self.getElementById("undefined", page); + } + return self.getElementById(try value.toZig([]const u8), page); + } }; const testing = @import("../../testing.zig"); diff --git a/src/browser/webapi/collections/node_live.zig b/src/browser/webapi/collections/node_live.zig index 566ea281..902e9234 100644 --- a/src/browser/webapi/collections/node_live.zig +++ b/src/browser/webapi/collections/node_live.zig @@ -171,7 +171,7 @@ pub fn NodeLive(comptime mode: Mode) type { } pub fn getByName(self: *Self, name: []const u8, page: *Page) ?*Element { - if (page.document.getElementById(name)) |element| { + if (page.document.getElementById(name, page)) |element| { const node = element.asNode(); if (self._tw.contains(node) and self.matches(node)) { return element; diff --git a/src/browser/webapi/element/html/Button.zig b/src/browser/webapi/element/html/Button.zig index c13cdcf4..6b093640 100644 --- a/src/browser/webapi/element/html/Button.zig +++ b/src/browser/webapi/element/html/Button.zig @@ -91,7 +91,7 @@ pub fn getForm(self: *Button, page: *Page) ?*Form { // If form attribute exists, ONLY use that (even if it references nothing) if (element.getAttributeSafe("form")) |form_id| { - if (page.document.getElementById(form_id)) |form_element| { + if (page.document.getElementById(form_id, page)) |form_element| { return form_element.is(Form); } // form attribute present but invalid - no form owner diff --git a/src/browser/webapi/element/html/Input.zig b/src/browser/webapi/element/html/Input.zig index 7bcd27a3..5c04b6a4 100644 --- a/src/browser/webapi/element/html/Input.zig +++ b/src/browser/webapi/element/html/Input.zig @@ -264,7 +264,7 @@ pub fn getForm(self: *Input, page: *Page) ?*Form { // If form attribute exists, ONLY use that (even if it references nothing) if (element.getAttributeSafe("form")) |form_id| { - if (page.document.getElementById(form_id)) |form_element| { + if (page.document.getElementById(form_id, page)) |form_element| { return form_element.is(Form); } // form attribute present but invalid - no form owner diff --git a/src/browser/webapi/element/html/Select.zig b/src/browser/webapi/element/html/Select.zig index ddac4b6d..293d53dc 100644 --- a/src/browser/webapi/element/html/Select.zig +++ b/src/browser/webapi/element/html/Select.zig @@ -219,7 +219,7 @@ pub fn getForm(self: *Select, page: *Page) ?*Form { // If form attribute exists, ONLY use that (even if it references nothing) if (element.getAttributeSafe("form")) |form_id| { - if (page.document.getElementById(form_id)) |form_element| { + if (page.document.getElementById(form_id, page)) |form_element| { return form_element.is(Form); } // form attribute present but invalid - no form owner diff --git a/src/browser/webapi/element/html/TextArea.zig b/src/browser/webapi/element/html/TextArea.zig index 26ba3438..c87b8798 100644 --- a/src/browser/webapi/element/html/TextArea.zig +++ b/src/browser/webapi/element/html/TextArea.zig @@ -90,7 +90,7 @@ pub fn getForm(self: *TextArea, page: *Page) ?*Form { // If form attribute exists, ONLY use that (even if it references nothing) if (element.getAttributeSafe("form")) |form_id| { - if (page.document.getElementById(form_id)) |form_element| { + if (page.document.getElementById(form_id, page)) |form_element| { return form_element.is(Form); } // form attribute present but invalid - no form owner