feat(click): report (and optionally --follow) a tab opened by a click (#24-A)
A click on a target=_blank link / window.open opened a new tab, but the active
tab stayed put, so the post-click snapshot showed the OLD page — looking exactly
like the click failed. On the relay the new tab is discovered only via getTargets
(the relay doesn't push target events to the daemon), so it went unsurfaced.
handle_click now snapshots tracked targets before the click and, after, runs a
lightweight BrowserManager::adopt_newly_opened (one getTargets, attaches only the
new target — far cheaper than a full resync) to detect a freshly-opened tab. It's
reported as openedTab {tabId,url,title} in the response (and a '→ opened new tab
[tN] <url>' hint in text mode). Default keeps focus on the current tab (so
multi-tab flows aren't hijacked, per #7/#8.1); 'click <sel> --follow' switches to
the new tab. Verified live: clicking a _blank link prints
'→ opened new tab [t13] https://example.org/'.
Completes the #24 friction items (B/C/D shipped in 770708b).
This commit is contained in:
+28
-6
@@ -424,6 +424,9 @@ fn parse_command_inner(args: &[String], flags: &Flags) -> Result<Value, ParseErr
|
||||
// === Core Actions ===
|
||||
"click" => {
|
||||
let new_tab = rest.contains(&"--new-tab");
|
||||
// `--follow`: if the click opens a new tab, switch the active tab to
|
||||
// it (default reports the opened tab but stays put) (issue #24-A).
|
||||
let follow = rest.contains(&"--follow");
|
||||
// Coordinate click as a first-class form (issue #8.4): when the only
|
||||
// handle is a pixel position, no element/selector is needed.
|
||||
// click <x> <y> e.g. click 449 320
|
||||
@@ -432,23 +435,27 @@ fn parse_command_inner(args: &[String], flags: &Flags) -> Result<Value, ParseErr
|
||||
let coord_args: Vec<&str> = rest
|
||||
.iter()
|
||||
.copied()
|
||||
.filter(|a| *a != "--new-tab" && *a != "--coords")
|
||||
.filter(|a| !a.starts_with("--"))
|
||||
.collect();
|
||||
if let Some((x, y)) = parse_coords(&coord_args) {
|
||||
return Ok(json!({ "id": id, "action": "click", "x": x, "y": y }));
|
||||
}
|
||||
let sel = rest
|
||||
.iter()
|
||||
.find(|arg| **arg != "--new-tab")
|
||||
.find(|arg| !arg.starts_with("--"))
|
||||
.ok_or_else(|| ParseError::MissingArguments {
|
||||
context: "click".to_string(),
|
||||
usage: "click <selector> | click <x> <y> | click --coords <x>,<y> [--new-tab]",
|
||||
usage:
|
||||
"click <selector> | click <x> <y> | click --coords <x>,<y> [--new-tab] [--follow]",
|
||||
})?;
|
||||
let mut cmd = json!({ "id": id, "action": "click", "selector": sel });
|
||||
if new_tab {
|
||||
Ok(json!({ "id": id, "action": "click", "selector": sel, "newTab": true }))
|
||||
} else {
|
||||
Ok(json!({ "id": id, "action": "click", "selector": sel }))
|
||||
cmd["newTab"] = json!(true);
|
||||
}
|
||||
if follow {
|
||||
cmd["follow"] = json!(true);
|
||||
}
|
||||
Ok(cmd)
|
||||
}
|
||||
"dblclick" => {
|
||||
let sel = rest.first().ok_or_else(|| ParseError::MissingArguments {
|
||||
@@ -3784,6 +3791,21 @@ mod tests {
|
||||
assert!(cmd.get("x").is_none());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_click_follow_flag() {
|
||||
// `--follow` sets the flag; the selector is still found even with the flag
|
||||
// before it (issue #24-A).
|
||||
let cmd = parse_command(&args("click @e5 --follow"), &default_flags()).unwrap();
|
||||
assert_eq!(cmd["selector"], "@e5");
|
||||
assert_eq!(cmd["follow"], true);
|
||||
let cmd2 = parse_command(&args("click --follow @e5"), &default_flags()).unwrap();
|
||||
assert_eq!(cmd2["selector"], "@e5");
|
||||
assert_eq!(cmd2["follow"], true);
|
||||
// Absent by default.
|
||||
let plain = parse_command(&args("click @e5"), &default_flags()).unwrap();
|
||||
assert!(plain.get("follow").is_none());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_tabs_alias_lists() {
|
||||
assert_eq!(
|
||||
|
||||
@@ -3116,6 +3116,15 @@ async fn handle_click(cmd: &Value, state: &mut DaemonState) -> Result<Value, Str
|
||||
|
||||
let button = cmd.get("button").and_then(|v| v.as_str()).unwrap_or("left");
|
||||
let click_count = cmd.get("clickCount").and_then(|v| v.as_i64()).unwrap_or(1) as i32;
|
||||
let follow = cmd.get("follow").and_then(|v| v.as_bool()).unwrap_or(false);
|
||||
|
||||
// Snapshot tracked targets so we can tell if this click opened a NEW tab
|
||||
// (target=_blank link / window.open). On the relay the new tab is discovered
|
||||
// passively and doesn't steal focus (#7/#8.1), so without surfacing it the
|
||||
// post-click snapshot shows the OLD page and looks like the click failed
|
||||
// (issue #24-A).
|
||||
let before: std::collections::HashSet<String> =
|
||||
mgr.pages_list().into_iter().map(|p| p.target_id).collect();
|
||||
|
||||
interaction::click(
|
||||
&mgr.client,
|
||||
@@ -3128,7 +3137,26 @@ async fn handle_click(cmd: &Value, state: &mut DaemonState) -> Result<Value, Str
|
||||
)
|
||||
.await?;
|
||||
|
||||
Ok(json!({ "clicked": selector }))
|
||||
// Give a just-opened tab a moment to register, then look for it.
|
||||
let mgr = state.browser.as_mut().ok_or("Browser not launched")?;
|
||||
tokio::time::sleep(std::time::Duration::from_millis(150)).await;
|
||||
let opened = mgr.adopt_newly_opened(&before).await;
|
||||
|
||||
let mut out = json!({ "clicked": selector });
|
||||
if let Some(page) = opened {
|
||||
let tab_id = super::browser::format_tab_id(page.tab_id);
|
||||
out["openedTab"] = json!({ "tabId": tab_id, "url": page.url, "title": page.title });
|
||||
// `--follow`: switch the active tab to the newly-opened one (default is
|
||||
// to report it but stay put, so multi-tab flows aren't hijacked).
|
||||
if follow {
|
||||
state.ref_map.clear();
|
||||
state.iframe_sessions.clear();
|
||||
state.active_frame_id = None;
|
||||
let _ = mgr.tab_switch_by_id(page.tab_id).await;
|
||||
out["followed"] = json!(true);
|
||||
}
|
||||
}
|
||||
Ok(out)
|
||||
}
|
||||
|
||||
async fn handle_dblclick(cmd: &Value, state: &mut DaemonState) -> Result<Value, String> {
|
||||
|
||||
@@ -1279,6 +1279,67 @@ impl BrowserManager {
|
||||
/// the active tab is preserved, and re-pinned if it was pruned. Powers a live
|
||||
/// `tab list` and adopt-by-targetId so a fresh session can reach a stranded,
|
||||
/// still-filled tab without reloading it (issue #21).
|
||||
/// Detect targets that appeared since the `before` set (e.g. a click that
|
||||
/// opened a new tab via a `target=_blank` link or `window.open`), attach +
|
||||
/// track each in the background, and return the first newly-opened page.
|
||||
///
|
||||
/// Lighter than [`resync_targets`] — one `getTargets` and work only on the
|
||||
/// new targets, no whole-tab url/title refresh — so it's cheap enough to run
|
||||
/// after every click. The new tab is added in the background (never steals
|
||||
/// the active tab, per #7/#8.1); the caller surfaces it so the agent knows a
|
||||
/// tab opened instead of seeing the old page (issue #24-A).
|
||||
pub async fn adopt_newly_opened(&mut self, before: &HashSet<String>) -> Option<PageInfo> {
|
||||
let result: GetTargetsResult = self
|
||||
.client
|
||||
.send_command_typed("Target.getTargets", &json!({}), None)
|
||||
.await
|
||||
.ok()?;
|
||||
let live: Vec<TargetInfo> = result
|
||||
.target_infos
|
||||
.into_iter()
|
||||
.filter(should_track_target)
|
||||
.collect();
|
||||
let mut opened: Option<PageInfo> = None;
|
||||
for target in &live {
|
||||
if before.contains(&target.target_id)
|
||||
|| self.pages.iter().any(|p| p.target_id == target.target_id)
|
||||
{
|
||||
continue;
|
||||
}
|
||||
let attach: AttachToTargetResult = match self
|
||||
.client
|
||||
.send_command_typed(
|
||||
"Target.attachToTarget",
|
||||
&AttachToTargetParams {
|
||||
target_id: target.target_id.clone(),
|
||||
flatten: true,
|
||||
},
|
||||
None,
|
||||
)
|
||||
.await
|
||||
{
|
||||
Ok(r) => r,
|
||||
Err(_) => continue,
|
||||
};
|
||||
let tab_id = self.assign_tab_id();
|
||||
let page = PageInfo {
|
||||
tab_id,
|
||||
label: None,
|
||||
target_id: target.target_id.clone(),
|
||||
session_id: attach.session_id.clone(),
|
||||
url: target.url.clone(),
|
||||
title: target.title.clone(),
|
||||
target_type: target.target_type.clone(),
|
||||
};
|
||||
self.add_background_page(page.clone());
|
||||
let _ = self.enable_domains(&attach.session_id).await;
|
||||
if opened.is_none() {
|
||||
opened = Some(page);
|
||||
}
|
||||
}
|
||||
opened
|
||||
}
|
||||
|
||||
pub async fn resync_targets(&mut self) -> Result<(), String> {
|
||||
self.client
|
||||
.send_command_typed::<_, Value>(
|
||||
|
||||
@@ -186,6 +186,29 @@ pub fn print_response_with_opts(resp: &Response, action: Option<&str>, opts: &Ou
|
||||
}
|
||||
|
||||
if let Some(data) = &resp.data {
|
||||
// A click that opened a new tab: surface it so the agent doesn't read the
|
||||
// unchanged old page as a failed click (issue #24-A).
|
||||
if let Some(opened) = data.get("openedTab") {
|
||||
let tid = opened.get("tabId").and_then(|v| v.as_str()).unwrap_or("?");
|
||||
let url = opened.get("url").and_then(|v| v.as_str()).unwrap_or("");
|
||||
let followed = data
|
||||
.get("followed")
|
||||
.and_then(|v| v.as_bool())
|
||||
.unwrap_or(false);
|
||||
let verb = if followed {
|
||||
"switched to new tab"
|
||||
} else {
|
||||
"opened new tab"
|
||||
};
|
||||
eprintln!(
|
||||
"{} {} [{}] {}",
|
||||
color::cyan("→"),
|
||||
verb,
|
||||
tid,
|
||||
color::dim(url)
|
||||
);
|
||||
}
|
||||
|
||||
// Dialog status response
|
||||
if action == Some("dialog") {
|
||||
if let Some(has_dialog) = data.get("hasDialog").and_then(|v| v.as_bool()) {
|
||||
|
||||
Reference in New Issue
Block a user