diff --git a/changes/307.fixed.md b/changes/307.fixed.md new file mode 100644 index 00000000..6a57a89e --- /dev/null +++ b/changes/307.fixed.md @@ -0,0 +1 @@ +**A file link that opens a new tab no longer looks like a blank page.** When Sentient clicked a download link that opens in a new tab, it reported a blank page instead of the page it was on. Now it stays on that page and reports the saved file. diff --git a/docs/API.md b/docs/API.md index 432d1068..5ee4cffd 100644 --- a/docs/API.md +++ b/docs/API.md @@ -2339,6 +2339,9 @@ Every new tool declares a `Risk`; approvals behave as in section 1. - `browser_click(ref)` write; effective risk `send` (via `risk_fn`) when the element's text/aria/value looks like buy, pay, place order, checkout, send, post, publish, delete, remove, confirm, subscribe, transfer, or it submits a form with payment fields → `{ok, clicked, url, message, new_tab?, dialogs?}`. A link opening a new tab makes it active. + A new tab that only carried a download (a file link with `target=_blank`: a blank tab the download starts from, + which may close again) or is Edge's downloads hub is not made active, and `url` stays the clicked page (#307); a + blank new tab gets 3 s to show a page before it counts as one. `confirm()` dialogs are accepted only for clicks approved as `send`; alerts are accepted; `dialogs` lists them. Its approval `risk_label` is "Purchase" for buy, pay, order, checkout and the like. A subscribe, follow, sign up or "notify me" reads "Signs up" (normal approval rules) only when the element or the text around it (its card or diff --git a/sentient/browser/service.py b/sentient/browser/service.py index c658cf2b..ffa9577b 100644 --- a/sentient/browser/service.py +++ b/sentient/browser/service.py @@ -65,6 +65,8 @@ STORAGE_FLUSH_S = 1.5 PAGE_CLOSE_TIMEOUT_S = 5.0 DOWNLOAD_APPEAR_GRACE_S = 0.3 +# #307: how long a blank tab a click opened gets to turn into a page, a download or nothing +DOWNLOAD_TAB_WAIT_S = 3.0 DOWNLOAD_MAX_DURATION_S = 120.0 DOWNLOAD_CANCEL_TIMEOUT_S = 5.0 DOWNLOAD_TASK_RETENTION_S = 300.0 @@ -234,6 +236,9 @@ def __init__(self, app): self._attached = False self._download_pages: set[Any] = set() self._internal_pages: set[Any] = set() + # a new tab -> the tab that was active when it opened (its opener when known), to go back to when the new + # tab closes or turns out to be Edge's downloads hub (#307) + self._came_from: dict[Any, Any] = {} self._profile = DEFAULT_PROFILE # the running profile, or the next one to start self._for_user = False # the open window was shown for the user (Open to sign in): don't switch under them self._engine: str | None = None @@ -724,6 +729,7 @@ async def _close_context(self) -> None: self._focused = None self._download_pages.clear() self._internal_pages.clear() + self._came_from.clear() if self._browser_cdp is not None: with contextlib.suppress(Exception): await self._browser_cdp.detach() @@ -822,6 +828,7 @@ def _on_context_closed(self, context: Any) -> None: self._snap = None self._focused = None self._download_pages.clear() + self._came_from.clear() self._browser_cdp = None self._clear_attached_downloads() if not self._closing: # the user closed the visible window (or their attached browser) @@ -854,23 +861,27 @@ def _is_downloads_hub(url: Any) -> bool: def _mark_internal_page(self, page: Any) -> None: self._internal_pages.add(page) + back = self._came_from.pop(page, None) if self._active is page: context = self._context pages = [ candidate for candidate in (self._usable_pages(context) if context is not None else []) if not self._attached or candidate in self._download_pages ] - self._active = pages[-1] if pages else None + self._active = back if back in pages else (pages[-1] if pages else None) def _on_page(self, page: Any) -> None: self._wire_page(page) if self._is_downloads_hub(page.url): self._mark_internal_page(page) elif not self._attached: # a link opened a new tab: keep working in it + if self._active is not None and self._active is not page: + self._came_from.setdefault(page, self._active) self._active = page self._spawn(self._publish_status()) def _on_popup(self, opener: Any, popup: Any) -> None: + self._came_from[popup] = opener if self._is_downloads_hub(popup.url): self._mark_internal_page(popup) return @@ -882,6 +893,7 @@ def _on_popup(self, opener: Any, popup: Any) -> None: def _on_page_closed(self, page: Any) -> None: self._download_pages.discard(page) self._internal_pages.discard(page) + back = self._came_from.pop(page, None) if self._snap and self._snap.get("page") is page: self._snap = None ctx = self._context @@ -890,7 +902,8 @@ def _on_page_closed(self, page: Any) -> None: p for p in (self._usable_pages(ctx) if ctx else []) if p is not page and (not self._attached or p in self._download_pages) ] - self._active = remaining[-1] if remaining else None + # back to the tab it came from (a file link's blank tab closes once the download starts, #307) + self._active = back if back in remaining else (remaining[-1] if remaining else None) if ctx is not None and not self._closing: self._spawn(self._publish_status()) @@ -1621,7 +1634,8 @@ async def click(self, ctx: Any, ref: str) -> dict: refusal = self._guard("click", info["ref"], live_risk) if refusal: return {"error": refusal} - pages_before = len(self._usable_pages(self._context)) + tabs_before = list(self._usable_pages(self._context)) + pages_before = len(tabs_before) self._accept_confirm = live_risk >= Risk.send or self.app.config.tools.approvals.mode == "off" try: await loc.click(timeout=10_000) @@ -1641,6 +1655,23 @@ async def click(self, ctx: Any, ref: str) -> dict: if self._context is None: raise BrowserError("The browser window was closed. Call the tool again and it will reopen.") active = self._active or page + # an older tab took over because the tab the click opened is gone again + fell_back = active in tabs_before and not any( + p not in tabs_before for p in self._usable_pages(self._context) + ) + if ( + active is not page + and not page.is_closed() + and (fell_back or await self._download_only_tab(active, downloads_before)) + ): + # #307: the tab the click opened only carried a download (or is Edge's downloads hub): the page that + # was clicked is still the one to work in, and the one to report. A real page that opened meanwhile + # in yet another new tab is kept. + now = self._active + if now is None or now is active or now.is_closed() or now in tabs_before or now in self._internal_pages: + active = self._active = page + else: + active = now await self._settle(active) await self._enforce_domains(active) out = { @@ -1649,7 +1680,7 @@ async def click(self, ctx: Any, ref: str) -> dict: "url": active.url, "message": "Clicked. Call browser_snapshot to see the page now.", } - if len(self._usable_pages(self._context)) > pages_before: + if active is not page and len(self._usable_pages(self._context)) > pages_before: out["new_tab"] = True out["message"] = "Clicked; it opened a new tab, which is now active. Call browser_snapshot to see it." await self._after_action(ctx, active) @@ -1658,6 +1689,22 @@ async def click(self, ctx: Any, ref: str) -> dict: ) return self._take_dialogs(out) + async def _download_only_tab(self, tab: Any, downloads_before: set[asyncio.Task[str]]) -> bool: + """True when ``tab``, made active by a click, holds no page: Edge's downloads hub, or a blank tab that a + download started from (a link to a file with ``target=_blank``) or that closed again. A blank tab gets + ``DOWNLOAD_TAB_WAIT_S`` to show a page instead.""" + deadline = time.monotonic() + DOWNLOAD_TAB_WAIT_S + while True: + if tab.is_closed() or tab in self._internal_pages or self._is_downloads_hub(tab.url): + return True + if str(tab.url or "") not in ("", "about:blank"): + return False + if self._download_tasks - downloads_before: + return True + if time.monotonic() >= deadline: + return False + await asyncio.sleep(0.05) + async def type(self, ctx: Any, ref: str, text: str, submit: bool = False) -> dict: async with self._lock: downloads_before = self._download_tasks.copy() diff --git a/tests/browser/conftest.py b/tests/browser/conftest.py index 3fec6862..a3b70808 100644 --- a/tests/browser/conftest.py +++ b/tests/browser/conftest.py @@ -48,6 +48,9 @@ "search.html": "Results

Results page

Search results here.

", "help.html": "Help

Help center

", "download.html": 'DownloadsDownload report', + # #307: a link that opens its file in a new tab + "download-tab.html": ('DownloadsGet it in a new tab ' + 'Open help in new tab'), "report.txt": "Sentient browser download test.\n", "signup.html": """Newsletter

Weekly notes

Free, no spam.

diff --git a/tests/browser/test_live_browser.py b/tests/browser/test_live_browser.py index fa0da27b..dc0885d1 100644 --- a/tests/browser/test_live_browser.py +++ b/tests/browser/test_live_browser.py @@ -187,3 +187,38 @@ async def closed_event() -> dict: break await asyncio.sleep(0.05) assert not browser.running + + +async def test_download_from_a_new_tab_link_keeps_the_page(browser, site): + """#307: a file link with target=_blank opens a blank tab the download starts from. The click reports the page + that was clicked, not the blank tab, and the file is saved and reported.""" + ctx = make_ctx(browser.app) + snap = await bt.browser_open.call(ctx, {"url": f"{site}/download-tab.html"}) + result = await bt.browser_click.call(ctx, {"ref": ref_of(snap["text"], r'link "Get it in a new tab"')}) + assert result["url"] == f"{site}/download-tab.html" + assert "new_tab" not in result + download_paths = result.get("downloads", []) + if not download_paths: + pending = [task for task in browser._download_tasks if not task.done()] + await asyncio.wait_for(asyncio.gather(*pending), timeout=10) + download_paths = (await bt.browser_snapshot.call(ctx, {}))["downloads"] + assert download_paths == ["downloads/direct-report.txt"] + assert (paths.files_dir() / download_paths[0]).read_text(encoding="utf-8") == "Direct browser-open download test.\n" + later = await bt.browser_snapshot.call(ctx, {}) + assert later["url"] == f"{site}/download-tab.html" and later["title"] == "Downloads" + + +async def test_download_tab_link_with_another_tab_open_keeps_the_clicked_page(browser, site): + """#307: when the blank download tab closes, the clicked tab stays active, not the last other tab.""" + ctx = make_ctx(browser.app) + text = (await bt.browser_open.call(ctx, {"url": f"{site}/download-tab.html"}))["text"] + assert (await bt.browser_click.call(ctx, {"ref": ref_of(text, r'link "Open help in new tab"')})).get("new_tab") + await bt.browser_switch_tab.call(ctx, {"index": 0}) + text = (await bt.browser_snapshot.call(ctx, {}))["text"] + result = await bt.browser_click.call(ctx, {"ref": ref_of(text, r'link "Get it in a new tab"')}) + assert result["url"] == f"{site}/download-tab.html" and "new_tab" not in result + later = await bt.browser_snapshot.call(ctx, {}) + assert later["url"] == f"{site}/download-tab.html" + pending = [task for task in browser._download_tasks if not task.done()] + if pending: + await asyncio.wait_for(asyncio.gather(*pending), timeout=10) diff --git a/tests/browser/test_profiles.py b/tests/browser/test_profiles.py index fd538d17..8b45e04e 100644 --- a/tests/browser/test_profiles.py +++ b/tests/browser/test_profiles.py @@ -2002,3 +2002,73 @@ async def new_cdp_session(self, page): assert rel == "downloads/scoped.txt" assert queried_pages == [dl_page] + + +async def test_a_click_that_only_opened_a_download_tab_stays_on_the_page(monkeypatch): + """#307: the blank tab a file link opens, or Edge's downloads hub, is not a page to keep working in.""" + import sentient.browser.service as service + + monkeypatch.setattr(service, "DOWNLOAD_TAB_WAIT_S", 0.3) + svc = BrowserService(make_app()) + + class Page: + def __init__(self, url, closed=False): + self.url, self.closed = url, closed + + def is_closed(self): + return self.closed + + before: set = set() + assert await svc._download_only_tab(Page("", closed=True), before) # the blank tab closed once the file came + assert await svc._download_only_tab(Page("edge://downloads-hub/"), before) + assert not await svc._download_only_tab(Page("https://example.com/help"), before) # a real new page + assert not await svc._download_only_tab(Page("about:blank"), before) # stays blank, no download: kept + + async def save(): + return "downloads/x.txt" + + task = asyncio.create_task(save()) + svc._download_tasks.add(task) + assert await svc._download_only_tab(Page("about:blank"), before) # a download started from it + await task + + +def test_a_closed_or_hub_tab_goes_back_to_the_tab_it_came_from(): + """#307: with several tabs open, a blank download tab that closes, or a new tab that turns out to be Edge's + downloads hub, hands back to the tab that was active when it opened, not to the last tab.""" + svc = BrowserService(make_app()) + + class Page: + def __init__(self, url): + self.url = url + + @property + def main_frame(self): + return self + + def is_closed(self): + return False + + def on(self, *_): + return None + + clicked, other = Page("https://example.com/reports"), Page("https://example.com/help") + context = type("Context", (), {"pages": [clicked, other]})() + svc._context = context + svc._closing = True # no status publishing in this unit test + svc._active = clicked + + blank = Page("") + context.pages.append(blank) + svc._on_page(blank) + assert svc._active is blank + context.pages.remove(blank) + svc._on_page_closed(blank) + assert svc._active is clicked + + hub = Page("") + context.pages.append(hub) + svc._on_page(hub) + hub.url = "edge://downloads-hub/" + svc._on_navigated(hub, hub) + assert svc._active is clicked