Merge pull request #1332 from lightpanda-io/getElementById

getElementById duplicate-id handling

src/browser/Page.zig

@@ -997,21 +997,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 &current._type.document._elements_by_id;
+                return .{
+                    .lookup = &current._type.document._elements_by_id,
+                    .removed_ids = &current._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;
@@ -1019,22 +1030,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);
+    var id_maps = self.getElementIdMap(parent);
-    const gop = try id_map.getOrPut(self.arena, id);
+    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());
+    const node = element.asNode();
-    _ = id_map.remove(id);
+    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);
+        const lookup = self.getElementIdMap(node).lookup;
-        return id_map.get(id);
+        return lookup.get(id);
     }
     var tw = @import("webapi/TreeWalker.zig").Full.Elements.init(node, .{});
     while (tw.next()) |el| {
@@ -2119,7 +2143,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 = .{};
@@ -2170,7 +2194,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);

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;
                 };

src/browser/tests/element/duplicate_ids.html

@@ -0,0 +1,19 @@
+<!DOCTYPE html>
+<script src="../testing.js"></script>
+
+<div id="test">first</div>
+<div id="test">second</div>
+
+<script id=duplicateIds>
+  const first = document.getElementById('test');
+  testing.expectEqual('first', first.textContent);
+
+  first.remove();
+
+  const second = document.getElementById('test');
+  testing.expectEqual('second', second.textContent);
+
+  // second.remove();
+
+  // testing.expectEqual(null, document.getElementById('test'));
+</script>

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,
@@ -173,11 +175,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) {
@@ -723,15 +746,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 });

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 {
+pub fn getElementById(self: *DocumentFragment, id: []const u8) ?*Element {
-    const id = id_ orelse return null;
+    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, .{});

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 {
+pub fn getElementById(self: *ShadowRoot, id: []const u8, page: *Page) ?*Element {
-    const id = id_ orelse return null;
+    if (id.len == 0) {
-    return self._elements_by_id.get(id);
+        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");

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;

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

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

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

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