fix: make hide_bid_if_invisible behave like the other property flags

browsergym/core/src/browsergym/utils/obs.py

@@ -41,7 +41,7 @@ def flatten_dom_to_str(
     filter_with_bid_only: bool = False,
     filter_som_only: bool = False,
     coord_decimals: int = 0,
-    hide_bid_if_invisible: int = False,
+    hide_bid_if_invisible: bool = False,
     hide_all_bids: bool = False,
 ) -> str:
     """Formats a DOM snapshot into a string text"""
@@ -131,18 +131,23 @@ def dfs(node_idx: int, parent_node_skipped: bool) -> str:
                     filter_with_bid_only=filter_with_bid_only,
                     filter_som_only=filter_som_only,
                     coord_decimals=coord_decimals,
+                    hide_bid_if_invisible=hide_bid_if_invisible,
                 )
 
                 # insert extra attributes before regular attributes
                 attributes = extra_attributes_to_print + attributes
 
+                # a bid with no recorded visibility is kept, the way _process_bid keeps it
+                bid_visibility = (extra_properties or {}).get(bid, {}).get("visibility")
+
                 # insert bid as first attribute
                 if not (
                     hide_all_bids
                     or bid is None
                     or (
                         hide_bid_if_invisible
-                        and extra_properties.get(bid, {}).get("visibility", 0) < 0.5
+                        and bid_visibility is not None
+                        and bid_visibility < 0.5
                     )
                 ):
                     attributes.insert(0, f'bid="{bid}"')
@@ -205,6 +210,7 @@ def _process_bid(
     filter_with_bid_only: bool = False,
     filter_som_only: bool = False,
     coord_decimals: int = 0,
+    hide_bid_if_invisible: bool = False,
 ):
     """
     Process extra attributes and attribute-based filters, for the element with the given bid.
@@ -225,6 +231,7 @@ def _process_bid(
                 filter_visible_only,
                 filter_with_bid_only,
                 filter_som_only,
+                hide_bid_if_invisible,
             )
         ):
             raise ValueError("extra_properties argument required")
@@ -371,6 +378,7 @@ def dfs(node_idx: int, depth: int, parent_node_filtered: bool, parent_node_name:
                     filter_with_bid_only=filter_with_bid_only,
                     filter_som_only=filter_som_only,
                     coord_decimals=coord_decimals,
+                    hide_bid_if_invisible=hide_bid_if_invisible,
                 )
 
                 # if either is True, skip the node
@@ -386,12 +394,16 @@ def dfs(node_idx: int, depth: int, parent_node_filtered: bool, parent_node_name:
                 else:
                     node_str = f"{node_role} {repr(node_name.strip())}"
 
+                # a bid with no recorded visibility is kept, the way _process_bid keeps it
+                bid_visibility = (extra_properties or {}).get(bid, {}).get("visibility")
+
                 if not (
                     hide_all_bids
                     or bid is None
                     or (
                         hide_bid_if_invisible
-                        and extra_properties.get(bid, {}).get("visibility", 0) < 0.5
+                        and bid_visibility is not None
+                        and bid_visibility < 0.5
                     )
                 ):
                     node_str = f"[{bid}] " + node_str

tests/core/test_obs_bid_visibility.py

@@ -0,0 +1,75 @@
+"""Tests for how the observation formatters treat the hide_bid_if_invisible flag.
+
+These run on hand-built snapshots rather than a live page, so they need no browser.
+"""
+
+import pytest
+
+from browsergym.core.constants import BROWSERGYM_ID_ATTRIBUTE as BID_ATTR
+from browsergym.utils.obs import flatten_axtree_to_str, flatten_dom_to_str
+
+
+def _axtree(bid: str) -> dict:
+    return {
+        "nodes": [
+            {
+                "nodeId": "1",
+                "role": {"value": "RootWebArea"},
+                "name": {"value": "a page"},
+                "childIds": ["2"],
+            },
+            {
+                "nodeId": "2",
+                "role": {"value": "button"},
+                "name": {"value": "Submit"},
+                "childIds": [],
+                "browsergym_id": bid,
+            },
+        ]
+    }
+
+
+def _dom_snapshot(bid: str) -> dict:
+    strings = ["#document", "html", "body", "button", BID_ATTR, bid]
+    return {
+        "strings": strings,
+        "documents": [
+            {
+                "nodes": {
+                    "nodeType": [9, 1, 1, 1],
+                    "nodeName": [0, 1, 2, 3],
+                    "nodeValue": [-1, -1, -1, -1],
+                    "parentIndex": [-1, 0, 1, 2],
+                    "attributes": [[], [], [], [4, 5]],
+                    "contentDocumentIndex": {"index": [], "value": []},
+                },
+            }
+        ],
+    }
+
+
+@pytest.mark.parametrize(
+    ("flatten", "build"),
+    [(flatten_axtree_to_str, _axtree), (flatten_dom_to_str, _dom_snapshot)],
+    ids=["axtree", "dom"],
+)
+def test_hide_bid_if_invisible_without_extra_properties_is_an_error(flatten, build):
+    """The flag cannot be honoured without visibility data, so say so, as the other flags do."""
+    with pytest.raises(ValueError, match="extra_properties argument required"):
+        flatten(build("42"), hide_bid_if_invisible=True)
+
+
+@pytest.mark.parametrize(
+    ("flatten", "build"),
+    [(flatten_axtree_to_str, _axtree), (flatten_dom_to_str, _dom_snapshot)],
+    ids=["axtree", "dom"],
+)
+def test_hide_bid_if_invisible_hides_only_a_recorded_invisible_bid(flatten, build):
+    """A bid with no recorded visibility stays, matching how _process_bid treats one."""
+    visible = {"42": {"visibility": 1.0, "bbox": None, "clickable": False, "set_of_marks": False}}
+    invisible = {"42": {"visibility": 0.0, "bbox": None, "clickable": False, "set_of_marks": False}}
+
+    assert "42" in flatten(build("42"), extra_properties=visible, hide_bid_if_invisible=True)
+    assert "42" not in flatten(build("42"), extra_properties=invisible, hide_bid_if_invisible=True)
+    # no entry at all: nothing says it is invisible, so the bid is kept
+    assert "42" in flatten(build("42"), extra_properties={}, hide_bid_if_invisible=True)