diff --git a/crates/ff-rdp-cli/src/cli/args.rs b/crates/ff-rdp-cli/src/cli/args.rs index fb026f4..f6ddf6d 100644 --- a/crates/ff-rdp-cli/src/cli/args.rs +++ b/crates/ff-rdp-cli/src/cli/args.rs @@ -701,6 +701,15 @@ top-level attempt and the scan (e.g. --frame sourcepoint). If the selector matches nowhere, the error names how many frames were tried and their URLs instead of a bare timeout. +When a selector matches more than one element, the default (flag-less) +behaviour clicks DOM-order index 0 — which may be hidden. Use --visible to +click the first non-hidden match instead, or --index N to pick a specific +match (0-based). Mutually exclusive with each other. On success, the output +gains {\"match_count\": N, \"chosen_index\": N}; on failure (no visible match / +index out of range), the error names the match count. If the plain selector +times out because it resolved to a hidden element, the error itself suggests +--visible/--index with the observed match count. + Output: {\"results\": {\"clicked\": true, \"tag\": \"...\", \"text\": \"...\", \"frame_url\": null}, \"total\": 1, \"meta\": {\"frame_url\": null, ...}} `frame_url` is always present (never omitted) — null when the click landed on the top-level document, the frame's URL string when it landed inside a frame. @@ -725,6 +734,12 @@ The value is set via the native HTMLInputElement/HTMLTextAreaElement/HTMLSelectE prototype setter so React/Vue/Svelte value trackers are invalidated, and `input` and `change` events are dispatched after the assignment. +When a selector matches more than one element (e.g. two `input[name=keywords]` +on the same page, one hidden), the default (flag-less) behaviour types into +DOM-order index 0. Use --visible to target the first non-hidden match instead, +or --index N for a specific match (0-based). Mutually exclusive with each +other. On success, the output gains {\"match_count\": N, \"chosen_index\": N}. + Output: {\"results\": {\"typed\": true, \"tag\": \"INPUT\", \"value\": \"...\"}, \"total\": 1, \"meta\": {...}}")] Type(TypeArgs), /// Wait for a condition to become true (polls every 100ms). @@ -1031,6 +1046,12 @@ Output (--all): full resolved-style object per match (dumps every property)" #[command( long_about = "Inspect CSS styles for an element matching a CSS selector. +When a selector matches more than one element, use --visible to inspect the +first non-hidden match instead of the default DOM-order index 0, or --index N +for a specific match (0-based). Mutually exclusive with each other; resolved +before styles are read, so the reported selector/rules are for the chosen +element only. + Output (computed): {\"results\": [{\"selector\": \"...\", \"computed\": {\"color\": \"...\", ...}}], \"total\": N, \"meta\": {...}} Output (--applied): {\"results\": [{\"selector\": \"...\", \"rules\": [{\"selector\": \"...\", \"properties\": [...]}]}], \"total\": N, \"meta\": {...}} Output (--layout): {\"results\": [{\"selector\": \"...\", \"box\": {\"margin\": {...}, \"border\": {...}, \"padding\": {...}, \"content\": {...}}}], \"total\": N, \"meta\": {...}}" @@ -1605,6 +1626,15 @@ pub struct ClickArgs { /// top-level attempt and the frame scan (e.g. --frame sourcepoint) #[arg(long, value_name = "URL_SUBSTRING")] pub frame: Option, + /// When the selector matches more than one element, click the first + /// *visible* one instead of blindly taking DOM-order index 0. Mutually + /// exclusive with --index. + #[arg(long, conflicts_with = "index")] + pub visible: bool, + /// When the selector matches more than one element, click the Nth match + /// (0-based), regardless of visibility. Mutually exclusive with --visible. + #[arg(long, value_name = "N", conflicts_with = "visible")] + pub index: Option, } #[derive(clap::Args)] @@ -1639,6 +1669,15 @@ pub struct TypeArgs { /// After typing, wait for network and DOM to idle #[arg(long)] pub settle: bool, + /// When the selector matches more than one element, type into the first + /// *visible* one instead of blindly taking DOM-order index 0. Mutually + /// exclusive with --index. + #[arg(long, conflicts_with = "index")] + pub visible: bool, + /// When the selector matches more than one element, type into the Nth + /// match (0-based), regardless of visibility. Mutually exclusive with --visible. + #[arg(long, value_name = "N", conflicts_with = "visible")] + pub index: Option, } #[derive(clap::Args)] @@ -1967,6 +2006,15 @@ pub struct StylesArgs { /// Comma-separated list of CSS property names to include (computed mode only) #[arg(long, value_delimiter = ',', conflicts_with_all = ["applied", "layout"])] pub properties: Option>, + /// When the selector matches more than one element, inspect the first + /// *visible* one instead of blindly taking DOM-order index 0. Mutually + /// exclusive with --index. + #[arg(long, conflicts_with = "index")] + pub visible: bool, + /// When the selector matches more than one element, inspect the Nth match + /// (0-based), regardless of visibility. Mutually exclusive with --visible. + #[arg(long, value_name = "N", conflicts_with = "visible")] + pub index: Option, } #[derive(clap::Args)] diff --git a/crates/ff-rdp-cli/src/commands/click.rs b/crates/ff-rdp-cli/src/commands/click.rs index a3c9b14..2a0db45 100644 --- a/crates/ff-rdp-cli/src/commands/click.rs +++ b/crates/ff-rdp-cli/src/commands/click.rs @@ -20,8 +20,9 @@ use crate::output_pipeline::OutputPipeline; use super::connect_tab::{ConnectedTab, connect_and_get_target}; use super::js_helpers::{ - DispatchMode, JSON_SENTINEL, WaitForPredicate, autowait_element, build_click_js, - escape_selector, resolve_result, settle_page, wait_for_predicates, + DispatchMode, JSON_SENTINEL, MatchPolicy, WaitForPredicate, autowait_element, build_click_js, + escape_selector, resolve_disambiguated_target, resolve_result, settle_page, + wait_for_predicates, }; use super::network_events::build_network_entries; @@ -43,6 +44,11 @@ pub struct ClickOptions<'a> { /// substring, skipping both the top-level attempt and the frame scan. /// `None` runs the default top-level-first, scan-on-not-found behaviour. pub frame: Option<&'a str>, + /// iter-140 Theme C: `--visible` / `--index N` — disambiguate a selector + /// that matches more than one element before doing anything else. `None` + /// (the default, flag-less path) is completely unchanged: DOM-order index + /// 0, same timing, same JS. + pub match_policy: Option, } impl Default for ClickOptions<'_> { @@ -55,6 +61,7 @@ impl Default for ClickOptions<'_> { wait_for_timeout_ms: None, settle: false, frame: None, + match_policy: None, } } } @@ -97,6 +104,28 @@ pub fn run_core( let wait_timeout_ms = opts.wait_timeout_ms.unwrap_or(cli.timeout); let console_actor = ctx.target.console_actor.clone(); + // iter-140 Theme C: `--visible`/`--index` resolve an ambiguous selector to + // a single, genuinely-unique element selector up front, so every step + // below (auto-wait, the click itself) acts on exactly the element the + // flag named instead of blindly taking DOM-order index 0. Flag-less calls + // skip this entirely — see `ClickOptions::match_policy`'s doc comment. + let resolved_selector; + let mut disambiguation: Option<(usize, usize)> = None; // (match_count, chosen_index) + let selector: &str = if let Some(policy) = opts.match_policy { + let target = resolve_disambiguated_target( + &mut ctx, + &console_actor, + selector, + policy, + wait_timeout_ms, + )?; + disambiguation = Some((target.match_count, target.chosen_index)); + resolved_selector = target.selector; + &resolved_selector + } else { + selector + }; + // A1: Auto-wait for element readiness (unless --no-wait). // // iter-129: `autowait_element` only ever polled the top-level document. @@ -195,6 +224,14 @@ pub fn run_core( if let Some(sm) = settle_method { result["settle_method"] = json!(sm.as_meta_str()); } + // iter-140 Theme B/C: when --visible/--index disambiguated an ambiguous + // selector, report how many elements matched and which was chosen — the + // same transparency the plan asks for on the failure path, surfaced here + // on success so `--visible`/`--index` calls aren't silent about it. + if let Some((match_count, chosen_index)) = disambiguation { + result["match_count"] = json!(match_count); + result["chosen_index"] = json!(chosen_index); + } Ok(result) } @@ -213,14 +250,16 @@ pub fn run( let settle_method = result .as_object_mut() .and_then(|o| o.remove("settle_method")); - // iter-129: `frame_url` moves to `meta` the same way — always present - // (null or a URL string), never omitted. - let frame_url = result.as_object_mut().and_then(|o| o.remove("frame_url")); + // iter-140 Theme E: `--help` documents `frame_url` as present in BOTH + // `results` AND `meta` (never omitted from either) — the code used to + // `.remove()` it from `results` here, so `--jq '.results.frame_url'` + // threw on every call. Copy instead of removing so it stays in both. + let frame_url = result.get("frame_url").cloned().unwrap_or(Value::Null); let mut meta = json!({"selector": selector}); if let Some(sm) = settle_method { meta["settle_method"] = sm; } - meta["frame_url"] = frame_url.unwrap_or(Value::Null); + meta["frame_url"] = frame_url; crate::connection_meta::merge_into_if_verbose( &mut meta, &cli.host, @@ -394,6 +433,14 @@ fn build_click_js_for_mode(escaped_selector: &str, mode: DispatchMode) -> String /// does not throw. Returns a descriptive `AppError::User` — never the bare /// upstream timeout — when `frame_filter` matches no frame, or when every /// candidate frame's eval still throws the not-found error. +// iter-140 Theme D: on a many-frame page (theguardian.com: 97 frames, most +// of them consent-string-laden ad iframe URLs) joining every URL raw +// produced a 65 KB error message. Cap both the number of URLs listed and +// each URL's length (reusing iter-128's `middle_ellipsis`, already wired +// into the same shape of problem for `perf`/`network` — see iter-139). +const MAX_LISTED_FRAME_URLS: usize = 10; +const FRAME_URL_MAX_LEN: usize = 80; + fn click_in_scanned_frame( ctx: &mut ConnectedTab, selector: &str, @@ -410,21 +457,37 @@ fn click_in_scanned_frame( }) .collect(); - let all_urls = || -> String { - targets + let bounded_urls = |targets: &[&TargetEvent]| -> String { + let total = targets.len(); + let listed: Vec = targets .iter() - .map(|t| t.url.as_deref().unwrap_or("")) - .collect::>() - .join(", ") + .take(MAX_LISTED_FRAME_URLS) + .map(|t| { + crate::output::middle_ellipsis( + t.url.as_deref().unwrap_or(""), + FRAME_URL_MAX_LEN, + ) + }) + .collect(); + if total > MAX_LISTED_FRAME_URLS { + format!( + "{} (+{} more)", + listed.join(", "), + total - MAX_LISTED_FRAME_URLS + ) + } else { + listed.join(", ") + } }; if let Some(filter) = frame_filter && candidates.is_empty() { + let all: Vec<&TargetEvent> = targets.iter().collect(); return Err(AppError::User(format!( "click --frame '{filter}' matched no frame ({} frame(s) available: {})", targets.len(), - all_urls() + bounded_urls(&all) ))); } @@ -451,11 +514,17 @@ fn click_in_scanned_frame( // Nothing matched anywhere — the informative, frame-aware diagnostic // that replaces the old bare "element not found" / 10s timeout. - let n = targets.len(); + // + // iter-140 Theme D: this must count `candidates` — the frames actually + // tried — not `targets.len()`. With `--frame guim` on a 97-frame page, + // `--frame` narrows the scan to a handful of candidates; reporting + // "matched in 0 of 97 frames" claimed every frame was tried when only the + // filtered subset was. + let tried = candidates.len(); + let total = targets.len(); Err(AppError::User(format!( - "click: selector {selector:?} matched in 0 of {n} frames (top + {} subframes: {})", - n.saturating_sub(1), - all_urls() + "click: selector {selector:?} matched in 0 of {tried} frame(s) tried (of {total} total): {}", + bounded_urls(&candidates) ))) } diff --git a/crates/ff-rdp-cli/src/commands/dom.rs b/crates/ff-rdp-cli/src/commands/dom.rs index 1345bad..02227b9 100644 --- a/crates/ff-rdp-cli/src/commands/dom.rs +++ b/crates/ff-rdp-cli/src/commands/dom.rs @@ -9,7 +9,9 @@ use crate::output_controls::{OutputControls, SortDir}; use crate::output_pipeline::OutputPipeline; use super::connect_tab::connect_and_get_target; -use super::js_helpers::{JSON_SENTINEL, escape_selector, eval_or_bail, resolve_result}; +use super::js_helpers::{ + JSON_SENTINEL, UNIQUE_SELECTOR_JS_FN, escape_selector, eval_or_bail, resolve_result, +}; #[derive(Debug, Clone, Copy)] pub enum OutputMode { @@ -37,7 +39,15 @@ pub enum OutputMode { /// The ref ID is injected by the Rust caller as a counter (`__REF_START__`). /// Actionable attributes only: id, name, type, href, aria-*, data-state, role, /// placeholder, value (for inputs). +/// +/// `__UNIQUE_SELECTOR_FN__` is replaced with [`js_helpers::UNIQUE_SELECTOR_JS_FN`] +/// (iter-140 Theme A) — each node's `__resolver` is that function's output for +/// the matched element, a genuine CSS selector, not a `querySelectorAll(sel)[i]` +/// JS expression. The old expression form round-tripped into +/// `document.querySelector('...')` call sites (`click`/`type`/`styles`/etc.) +/// as a double-nested, invalid selector string — see the plan's Theme A bug #1. const ARIA_TREE_JS_TEMPLATE: &str = r"(function() { + __UNIQUE_SELECTOR_FN__ var ACTIONABLE_ATTRS = ['id','name','type','href','placeholder','value', 'aria-label','aria-expanded','aria-hidden','aria-haspopup','aria-selected', 'aria-checked','aria-disabled','aria-controls','aria-describedby', @@ -106,8 +116,10 @@ const ARIA_TREE_JS_TEMPLATE: &str = r"(function() { node.hasShadowRoot = true; node.shadowMode = sr.mode || 'open'; } - // Resolver expression: re-selects this element by its querySelectorAll index. - node.__resolver = 'document.querySelectorAll(\'__SELECTOR__\')[' + i + ']'; + // Resolver: a genuinely-unique CSS selector for this element (iter-140 + // Theme A), safe to feed straight back into `document.querySelector` / + // `DomWalkerActor::query_selector` from any later `--ref e` call. + node.__resolver = __ffrdpUniqueSelector(el); results.push(node); } if (results.length === 1) return '__FF_RDP_JSON__' + JSON.stringify(results[0]); @@ -442,6 +454,7 @@ fn build_js_with_ref_start(selector: &str, mode: OutputMode, ref_start: u64) -> ARIA_TREE_JS_TEMPLATE .replace("__SELECTOR__", &escaped) .replace("__REF_START__", &ref_start.to_string()) + .replace("__UNIQUE_SELECTOR_FN__", UNIQUE_SELECTOR_JS_FN) } OutputMode::OuterHtml => format!( r"(function() {{ diff --git a/crates/ff-rdp-cli/src/commands/index.rs b/crates/ff-rdp-cli/src/commands/index.rs index 3d9bb04..ca6ef8d 100644 --- a/crates/ff-rdp-cli/src/commands/index.rs +++ b/crates/ff-rdp-cli/src/commands/index.rs @@ -372,11 +372,19 @@ fn crawl_page( .filter(|s| !s.is_empty()); // Extract forms via our JS template. - let forms_json = eval_js_string(cli, form_extraction_js_template()).unwrap_or_default(); + let forms_json = eval_js_string( + cli, + &form_extraction_js_template(crate::commands::js_helpers::UNIQUE_SELECTOR_JS_FN), + ) + .unwrap_or_default(); let forms = parse_forms_from_json(&forms_json); // Extract landmarks via our JS template. - let landmarks_json = eval_js_string(cli, landmark_extraction_js_template()).unwrap_or_default(); + let landmarks_json = eval_js_string( + cli, + &landmark_extraction_js_template(crate::commands::js_helpers::UNIQUE_SELECTOR_JS_FN), + ) + .unwrap_or_default(); let landmarks = parse_landmarks_from_json(&landmarks_json); // Extract outgoing links. diff --git a/crates/ff-rdp-cli/src/commands/js_helpers.rs b/crates/ff-rdp-cli/src/commands/js_helpers.rs index 2009bde..82b7f43 100644 --- a/crates/ff-rdp-cli/src/commands/js_helpers.rs +++ b/crates/ff-rdp-cli/src/commands/js_helpers.rs @@ -6,7 +6,8 @@ use ff_rdp_core::{ }; use serde_json::Value; -use super::connect_tab::ConnectedTab; +use super::connect_tab::{ConnectedTab, connect_and_get_target}; +use crate::cli::args::Cli; use crate::error::AppError; /// Evaluate JavaScript on a tab and bail with an error if the result is an exception. @@ -87,6 +88,53 @@ pub(crate) fn escape_selector(selector: &str) -> String { inner.replace('\'', "\\'") } +// --------------------------------------------------------------------------- +// Unique-selector generation (iter-140 Theme A/F) +// --------------------------------------------------------------------------- + +/// Source of a JS function `__ffrdpUniqueSelector(el)` that computes a +/// genuinely-unique CSS selector for a live DOM element: an `#id` shortcut +/// when available, otherwise a `tag:nth-child(N)` structural path walked up +/// to (but not including) `document.documentElement`. +/// +/// This is the single source of truth for "turn a DOM node into a selector +/// safe to hand back to `document.querySelector` / `DomWalkerActor::query_selector` +/// unchanged" — used by: +/// - `dom.rs`'s ARIA-tree ref registration (`--ref e` resolvers), so a ref +/// round-trips into a real CSS selector instead of a bare JS expression +/// (iter-140 Theme A bug #1). +/// - `resolve_disambiguated_target` below, so `--visible`/`--index` on +/// `click`/`type`/`styles` resolve to the exact chosen element. +/// - `page_map`'s landmark/form-submit extraction, so generated page-maps +/// hand back selectors that resolve to exactly one element (iter-140 Theme F). +/// +/// Callers embed this once per IIFE and then call `__ffrdpUniqueSelector(el)`. +/// The function assumes `el` lives in the top-level document (no shadow-DOM +/// traversal) — consistent with every call site's existing scope. +pub(crate) const UNIQUE_SELECTOR_JS_FN: &str = r" + function __ffrdpUniqueSelector(el) { + if (!el || el.nodeType !== 1) return null; + if (el === document.documentElement) return 'html'; + var path = []; + var node = el; + while (node && node.nodeType === 1 && node !== document.documentElement) { + var part; + if (node.id) { + part = '#' + CSS.escape(node.id); + path.unshift(part); + break; + } + var sib = node; + var nth = 1; + while ((sib = sib.previousElementSibling)) { nth++; } + part = node.nodeName.toLowerCase() + ':nth-child(' + nth + ')'; + path.unshift(part); + node = node.parentElement; + } + return path.join(' > '); + } +"; + const POLL_INTERVAL_MS: u64 = 100; // --------------------------------------------------------------------------- @@ -182,22 +230,36 @@ pub(crate) fn autowait_element( // Phase 1: wait for element to exist + be visible + have non-zero rect. loop { if started.elapsed() >= timeout { - return Err(AppError::Timeout(format!( - "selector '{selector}' not ready (not found / hidden / unstable) after {timeout_ms}ms" - ))); + // iter-140 Theme B: run one extra (cheap — only at the moment of + // failure, never per-poll) diagnostic eval so the error names how + // many elements matched and distinguishes "hidden" from + // "not found" instead of the old undifferentiated + // "not found / hidden / unstable" for every cause. + let diag = diagnose_selector_failure(ctx, console_actor, selector, &escaped); + return Err(AppError::Timeout(format!("{diag} after {timeout_ms}ms"))); } let eval = WebConsoleActor::evaluate_js_async(ctx.transport_mut(), console_actor, &readiness_js) .map_err(AppError::from)?; - if let Some(ref exc) = eval.exception { - let msg = exc - .message - .as_deref() - .unwrap_or("element readiness check failed"); + if eval.exception.is_some() { + // iter-140 Theme B (on-the-wire correction): `display:none` / + // `visibility:hidden` on the DOM-order-0 match throws + // immediately here — this branch returns on the *first* eval, + // before the timeout loop above ever gets a chance to run + // `diagnose_selector_failure`. A selector matching one hidden + // element and one visible one (both legitimate real-page shapes: + // an a11y-hidden duplicate, an inactive tab panel) used to report + // only the bare JS message with no match count — exactly the + // "distinguishes hidden from not-found" gap Theme B exists to + // close, just reached from a different code path than the + // timeout branch. Route through the same diagnostic so both + // paths report match count / chosen index identically. + let diag = diagnose_selector_failure(ctx, console_actor, selector, &escaped); + let elapsed_ms = started.elapsed().as_millis(); return Err(AppError::Timeout(format!( - "selector '{selector}' not ready after {timeout_ms}ms: {msg}" + "{diag} (after {elapsed_ms}ms, timeout {timeout_ms}ms)" ))); } @@ -220,9 +282,8 @@ pub(crate) fn autowait_element( ))); } if started.elapsed() >= timeout { - return Err(AppError::Timeout(format!( - "selector '{selector}' not ready (not found / hidden / unstable) after {timeout_ms}ms" - ))); + let diag = diagnose_selector_failure(ctx, console_actor, selector, &escaped); + return Err(AppError::Timeout(format!("{diag} after {timeout_ms}ms"))); } let eval = @@ -249,6 +310,276 @@ pub(crate) fn autowait_element( Ok(Value::Null) // caller will proceed with the action } +/// Diagnose *why* a selector never became ready, for a richer timeout error +/// than the old undifferentiated "not found / hidden / unstable" (iter-140 +/// Theme B: gov.uk's `input[name=keywords]` matches two elements — `type` +/// silently took the hidden one and reported nothing about the other match). +/// +/// Runs a single extra JS eval — cheap, since it only happens once, at the +/// moment `autowait_element` gives up — that reports the match count and +/// whether the DOM-order-0 match (the one autowait actually polled) is +/// hidden. Distinguishes: +/// - 0 matches → not found +/// - 1 match, hidden → a single permanently-hidden element (not an ambiguity +/// problem — just genuinely hidden) +/// - 2+ matches, chosen (index 0) hidden → the exact repro from the plan: +/// names the count and points at `--visible`/`--index` to recover +/// - 2+ matches, chosen (index 0) visible but unstable → same match-count +/// context, without wrongly implying the element can't be found at all +/// +/// Best-effort: if the diagnostic eval itself throws or the transport drops, +/// falls back to the original undifferentiated message rather than masking +/// the real timeout with a second error. +fn diagnose_selector_failure( + ctx: &mut ConnectedTab, + console_actor: &ActorId, + selector: &str, + escaped_selector: &str, +) -> String { + let js = format!( + r"(function() {{ + var matches = document.querySelectorAll('{escaped_selector}'); + var n = matches.length; + if (n === 0) return JSON.stringify({{matchCount: 0}}); + var el = matches[0]; + var r = el.getBoundingClientRect(); + var cs = window.getComputedStyle(el); + var hidden = cs.display === 'none' || cs.visibility === 'hidden' || (r.width === 0 && r.height === 0); + return JSON.stringify({{matchCount: n, hidden: hidden}}); +}})()" + ); + + let diag = WebConsoleActor::evaluate_js_async(ctx.transport_mut(), console_actor, &js) + .ok() + .filter(|r| r.exception.is_none()) + .and_then(|r| match r.result { + Grip::Value(v) => v + .as_str() + .and_then(|s| serde_json::from_str::(s).ok()), + _ => None, + }); + + let Some(diag) = diag else { + return format!("selector '{selector}' not ready (not found / hidden / unstable)"); + }; + + let match_count = diag.get("matchCount").and_then(Value::as_u64).unwrap_or(0); + if match_count == 0 { + return format!("selector '{selector}' not ready — 0 elements matched (not found)"); + } + let hidden = diag.get("hidden").and_then(Value::as_bool).unwrap_or(false); + if match_count == 1 { + return if hidden { + format!("selector '{selector}' not ready — the 1 matching element is hidden") + } else { + format!( + "selector '{selector}' not ready — matched 1 element (layout did not stabilise)" + ) + }; + } + let last_index = match_count - 1; + if hidden { + format!( + "selector '{selector}' not ready — matched {match_count} elements, chose index 0 \ + which is hidden; pass --visible or --index 0..{last_index} to target a different match" + ) + } else { + format!( + "selector '{selector}' not ready — matched {match_count} elements, chose index 0 \ + (layout did not stabilise); pass --index 0..{last_index} to target a different match" + ) + } +} + +// --------------------------------------------------------------------------- +// Match-policy disambiguation (iter-140 Theme B/C) +// --------------------------------------------------------------------------- + +/// How to choose among multiple elements matched by a CSS selector, when the +/// caller explicitly asked to disambiguate via `--visible` / `--index N`. +/// +/// The flag-less default path (`autowait_element` above) is entirely +/// unaffected by this enum — it keeps taking DOM-order index 0 with unchanged +/// timing, per [`crate::commands::click::ClickOptions::match_policy`]'s doc +/// comment. This only applies when a flag is passed. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum MatchPolicy { + /// `--visible`: the first match that is not hidden (display:none, + /// visibility:hidden, or a zero-size rect). Reports "no visible match" + /// rather than silently falling back to a hidden one. + Visible, + /// `--index N`: the Nth match (0-based), regardless of visibility. + Index(usize), +} + +impl MatchPolicy { + /// Build a `MatchPolicy` from the two mutually-exclusive CLI flags. + /// + /// clap's `conflicts_with` already prevents both being set on the command + /// line; this is a defensive second check for callers that construct the + /// combination programmatically (script runner steps, tests). + pub(crate) fn from_flags( + visible: bool, + index: Option, + ) -> Result, AppError> { + match (visible, index) { + (true, Some(_)) => Err(AppError::User( + "--visible and --index are mutually exclusive".to_string(), + )), + (true, None) => Ok(Some(Self::Visible)), + (false, Some(n)) => Ok(Some(Self::Index(n))), + (false, None) => Ok(None), + } + } +} + +/// The result of resolving an ambiguous selector to one specific element. +pub(crate) struct ResolvedTarget { + /// A genuinely-unique CSS selector for the chosen element (an `#id` + /// shortcut or a `tag:nth-child(N)` structural path — see + /// [`UNIQUE_SELECTOR_JS_FN`]), safe to feed into `document.querySelector` + /// or `DomWalkerActor::query_selector` unchanged. + pub(crate) selector: String, + /// How many elements the original selector matched. + pub(crate) match_count: usize, + /// Which 0-based index was chosen among those matches. + pub(crate) chosen_index: usize, +} + +/// Build the JS that evaluates `escaped_selector`, applies `policy` to pick +/// one match, and returns that match's genuinely-unique selector alongside +/// the match count — or `{ok: false, matchCount}` when `policy` can't be +/// satisfied (no visible match / index out of range). +fn build_disambiguation_js(escaped_selector: &str, policy: MatchPolicy) -> String { + let choose = match policy { + MatchPolicy::Index(n) => format!("var chosenIndex = ({n} < matches.length) ? {n} : -1;"), + MatchPolicy::Visible => r" + var chosenIndex = -1; + for (var i = 0; i < matches.length; i++) { + var r = matches[i].getBoundingClientRect(); + var cs = window.getComputedStyle(matches[i]); + var visible = r.width > 0 && r.height > 0 && cs.display !== 'none' && cs.visibility !== 'hidden'; + if (visible) { chosenIndex = i; break; } + }" + .to_string(), + }; + + format!( + r"(function() {{ + {UNIQUE_SELECTOR_JS_FN} + var matches = document.querySelectorAll('{escaped_selector}'); + var matchCount = matches.length; + {choose} + if (chosenIndex === -1) {{ + return '{JSON_SENTINEL}' + JSON.stringify({{ok: false, matchCount: matchCount}}); + }} + var chosen = matches[chosenIndex]; + return '{JSON_SENTINEL}' + JSON.stringify({{ + ok: true, + matchCount: matchCount, + chosenIndex: chosenIndex, + selector: __ffrdpUniqueSelector(chosen) + }}); +}})()" + ) +} + +/// Resolve a possibly-ambiguous selector to the unique selector of a single +/// chosen element, per `policy` (iter-140 Theme B/C — `--visible`/`--index` +/// on `click`/`type`/`styles`). +/// +/// Polls until `timeout_ms` elapses so a `--visible` match that appears after +/// the initial call (e.g. a hydrating SPA) is still caught, matching +/// `autowait_element`'s existing patience on the flag-less path. +pub(crate) fn resolve_disambiguated_target( + ctx: &mut ConnectedTab, + console_actor: &ActorId, + selector: &str, + policy: MatchPolicy, + timeout_ms: u64, +) -> Result { + use std::time::{Duration, Instant}; + + let escaped = escape_selector(selector); + let js = build_disambiguation_js(&escaped, policy); + let timeout = Duration::from_millis(timeout_ms); + let poll = Duration::from_millis(POLL_INTERVAL_MS); + let started = Instant::now(); + + let last_match_count: u64 = loop { + let eval = WebConsoleActor::evaluate_js_async(ctx.transport_mut(), console_actor, &js) + .map_err(AppError::from)?; + if let Some(exc) = &eval.exception { + let msg = exc + .message + .as_deref() + .unwrap_or("selector evaluation failed"); + return Err(AppError::User(format!( + "selector '{selector}' is invalid: {msg}" + ))); + } + let value = resolve_result(ctx, &eval.result)?; + let match_count = value.get("matchCount").and_then(Value::as_u64).unwrap_or(0); + if value.get("ok").and_then(Value::as_bool) == Some(true) { + let resolved = value + .get("selector") + .and_then(Value::as_str) + .map(str::to_owned) + .ok_or_else(|| { + AppError::Internal(anyhow::anyhow!("disambiguation JS missing 'selector'")) + })?; + let chosen_index = value + .get("chosenIndex") + .and_then(Value::as_u64) + .unwrap_or(0); + return Ok(ResolvedTarget { + selector: resolved, + match_count: usize::try_from(match_count).unwrap_or(usize::MAX), + chosen_index: usize::try_from(chosen_index).unwrap_or(usize::MAX), + }); + } + if started.elapsed() >= timeout { + break match_count; + } + std::thread::sleep(poll); + }; + + Err(AppError::Timeout(match policy { + MatchPolicy::Index(n) => format!( + "selector '{selector}' matched {last_match_count} element(s) after {timeout_ms}ms — \ + index {n} is out of range (0..{})", + last_match_count.saturating_sub(1) + ), + MatchPolicy::Visible if last_match_count == 0 => { + format!("selector '{selector}' matched 0 elements (not found) after {timeout_ms}ms") + } + MatchPolicy::Visible => format!( + "selector '{selector}' matched {last_match_count} element(s) after {timeout_ms}ms but \ + none are visible — pass --index 0..{} to target a hidden one", + last_match_count.saturating_sub(1) + ), + })) +} + +/// Connect, resolve `selector` per `policy`, and return the resolved unique +/// selector's `String` alone. For one-shot commands (`styles`/`cascade`/ +/// `computed`) that don't otherwise need a held-open [`ConnectedTab`] before +/// dispatching to their own `run` function — those open their own connection +/// internally, so this makes (and drops) a short-lived one just for the +/// resolution step. +pub(crate) fn resolve_disambiguated_selector_standalone( + cli: &Cli, + selector: &str, + policy: MatchPolicy, + timeout_ms: u64, +) -> Result { + let mut ctx = connect_and_get_target(cli)?; + let console_actor = ctx.target.console_actor.clone(); + let target = + resolve_disambiguated_target(&mut ctx, &console_actor, selector, policy, timeout_ms)?; + Ok(target.selector) +} + // --------------------------------------------------------------------------- // Pointer-event dispatch // --------------------------------------------------------------------------- @@ -664,6 +995,42 @@ mod tests { assert_eq!(escape_selector("input[name=email]"), "input[name=email]"); } + #[test] + fn match_policy_from_flags_visible_only() { + assert_eq!( + MatchPolicy::from_flags(true, None).unwrap(), + Some(MatchPolicy::Visible) + ); + } + + #[test] + fn match_policy_from_flags_index_only() { + assert_eq!( + MatchPolicy::from_flags(false, Some(2)).unwrap(), + Some(MatchPolicy::Index(2)) + ); + } + + #[test] + fn match_policy_from_flags_neither_is_none() { + assert_eq!(MatchPolicy::from_flags(false, None).unwrap(), None); + } + + #[test] + fn match_policy_from_flags_both_is_error() { + // Defensive check for programmatic callers (script runner steps, + // tests) that construct the combination without going through + // clap's `conflicts_with`, which already prevents this on the CLI. + let err = MatchPolicy::from_flags(true, Some(1)).unwrap_err(); + let AppError::User(msg) = &err else { + panic!("expected AppError::User, got: {err:?}"); + }; + assert!( + msg.contains("mutually exclusive"), + "expected a mutually-exclusive User error, got: {msg:?}" + ); + } + #[test] fn is_truthy_true_values() { assert!(is_truthy(&Grip::Value(json!(true)))); diff --git a/crates/ff-rdp-cli/src/commands/type_text.rs b/crates/ff-rdp-cli/src/commands/type_text.rs index c79da89..1d25394 100644 --- a/crates/ff-rdp-cli/src/commands/type_text.rs +++ b/crates/ff-rdp-cli/src/commands/type_text.rs @@ -8,8 +8,8 @@ use crate::output_pipeline::OutputPipeline; use super::connect_tab::connect_and_get_target; use super::js_helpers::{ - JSON_SENTINEL, WaitForPredicate, autowait_element, escape_selector, eval_or_bail, - resolve_result, settle_page, wait_for_predicates, + JSON_SENTINEL, MatchPolicy, WaitForPredicate, autowait_element, escape_selector, eval_or_bail, + resolve_disambiguated_target, resolve_result, settle_page, wait_for_predicates, }; /// Options controlling auto-wait and post-action behaviour for `type`. @@ -25,6 +25,10 @@ pub struct TypeOptions<'a> { pub wait_for_timeout_ms: Option, /// Whether to wait for page settle after typing (--settle). pub settle: bool, + /// iter-140 Theme C: `--visible` / `--index N` — disambiguate a selector + /// that matches more than one element before doing anything else. `None` + /// (the default, flag-less path) is unchanged. + pub match_policy: Option, } /// Type text into a DOM element and return the result value without printing. @@ -42,6 +46,26 @@ pub fn run_core( let wait_timeout_ms = opts.wait_timeout_ms.unwrap_or(cli.timeout); + // iter-140 Theme C: resolve `--visible`/`--index` to a single, + // genuinely-unique element selector up front — see the matching comment + // in click.rs's run_core for why this must happen before auto-wait. + let resolved_selector; + let mut disambiguation: Option<(usize, usize)> = None; // (match_count, chosen_index) + let selector: &str = if let Some(policy) = opts.match_policy { + let target = resolve_disambiguated_target( + &mut ctx, + &console_actor, + selector, + policy, + wait_timeout_ms, + )?; + disambiguation = Some((target.match_count, target.chosen_index)); + resolved_selector = target.selector; + &resolved_selector + } else { + selector + }; + // A2: Auto-wait for the element to be focusable (also calls .focus()). if !opts.no_wait { autowait_element(&mut ctx, &console_actor, selector, wait_timeout_ms, true)?; @@ -103,6 +127,11 @@ pub fn run_core( if let Some(sm) = settle_method { result["settle_method"] = json!(sm.as_meta_str()); } + // iter-140 Theme B/C: report disambiguation transparency on success too. + if let Some((match_count, chosen_index)) = disambiguation { + result["match_count"] = json!(match_count); + result["chosen_index"] = json!(chosen_index); + } Ok(result) } diff --git a/crates/ff-rdp-cli/src/daemon/server.rs b/crates/ff-rdp-cli/src/daemon/server.rs index cbd96d7..b61146e 100644 --- a/crates/ff-rdp-cli/src/daemon/server.rs +++ b/crates/ff-rdp-cli/src/daemon/server.rs @@ -105,8 +105,26 @@ struct RefStore { /// Monotonically-increasing counter. Starts at 1; each `register-refs` /// call receives the current value and advances it by the number of refs /// registered so that successive calls produce globally-unique handles. + /// Reset to 1 on [`Self::clear`] so post-navigation refs restart at `e1`. next: u64, - /// `"e"` → JS resolution expression (e.g. `"document.querySelectorAll('button')[2]"`). + /// The highest value `next` has ever reached, across every generation — + /// unlike `next`, this is **never** reset by [`Self::clear`]. + /// + /// Used by `resolve-ref` to distinguish "this id belonged to a page that + /// has since navigated away" (expired) from "this id was never allocated + /// in this daemon session at all" (typo / wrong session). Before this + /// field existed, the heuristic compared against `next`, which resets to + /// 1 on every `clear()` — so immediately after a real navigation, *every* + /// previously-valid id (e.g. `e2`) looked like `n > next` and was + /// misreported as "not registered" instead of "expired" (iter-140 Theme A + /// AC3). + high_water: u64, + /// `"e"` → CSS selector that resolves to exactly this element (a + /// genuinely-unique `#id` or `tag:nth-child(N)` structural path — see + /// `js_helpers::UNIQUE_SELECTOR_JS_FN`). iter-140 Theme A: this used to be + /// a JS expression like `"document.querySelectorAll('button')[2]"`, which + /// broke every consumer that fed it into `document.querySelector(...)` as + /// if it were plain selector text. refs: HashMap, } @@ -114,6 +132,7 @@ impl RefStore { fn new() -> Self { Self { next: 1, + high_water: 1, refs: HashMap::new(), } } @@ -123,6 +142,7 @@ impl RefStore { fn alloc(&mut self, count: u64) -> u64 { let start = self.next; self.next = start.saturating_add(count); + self.high_water = self.high_water.max(self.next); start } @@ -167,6 +187,7 @@ impl RefStore { self.refs.clear(); // Reset counter so IDs restart at e1 after each navigation. self.next = 1; + // `high_water` is deliberately NOT reset — see its doc comment. } } @@ -1191,7 +1212,9 @@ fn dispatch_firefox_message( forward_to_rpc_client(state, msg); } else { // Detect navigation events and clear the ref store. - if is_navigation_event(msg) { + if is_navigation_event(state, msg) { + let msg_type = msg.get("type").and_then(Value::as_str).unwrap_or_default(); + tracing::debug!(msg_type, "daemon: navigation event — clearing ref store"); state.nav_generation.fetch_add(1, Ordering::Relaxed); lock_or_recover!(state.ref_store).clear(); // Record a navigation boundary in the network buffer. @@ -1200,7 +1223,6 @@ fn dispatch_firefox_message( // We record boundaries for `tabNavigated` only so the // boundary URL reflects the committed document, not the // in-flight request. - let msg_type = msg.get("type").and_then(Value::as_str).unwrap_or_default(); if msg_type == "tabNavigated" { let nav_url = msg .get("url") @@ -1276,18 +1298,76 @@ fn is_console_push_event(msg: &Value) -> bool { /// document. Earliest reliable signal. /// - `tabNavigated` on the tab actor once the new document has been /// committed. -/// - `frameUpdate` for nested-frame navigations. +/// - `frameUpdate`, but **only** when it reports the top-level frame at a URL +/// that differs from the last committed one (see +/// [`frame_update_is_real_navigation`]) — same-document top-level +/// navigations (fragment changes, `history.pushState`) that +/// `tabNavigated`/`willNavigate` don't reliably cover. /// -/// All three indicate the DOM has been replaced and any `e` refs allocated -/// against the old page are stale. We over-invalidate rather than under- -/// invalidate: an extra clear is harmless (the next allocation simply gets a -/// fresh generation), whereas a missed signal could let stale refs resolve to -/// the wrong element. -fn is_navigation_event(msg: &Value) -> bool { - matches!( - msg.get("type").and_then(Value::as_str), - Some("tabNavigated" | "willNavigate" | "frameUpdate") - ) +/// `tabNavigated`/`willNavigate` indicate the top document has been replaced +/// and any `e` refs allocated against the old page are stale. +/// +/// `frameUpdate` is **not** navigation-specific: per Firefox's +/// `WindowGlobalTargetActor._notifyDocShellsUpdate`, the top-level target +/// emits `frameUpdate` any time *any* docShell in the page changes — created, +/// destroyed, or navigated — including a same-origin or cross-origin `" + ); + } + routes.insert( + "/".to_owned(), + FixtureRoute::html(format!( + "t140 many frames

top

{iframes}" + )), + ); + for i in 0..n { + routes.insert( + format!("/leaf{i}.html"), + FixtureRoute::html(format!("

leaf {i}

")), + ); + } + routes +} + +/// Leaf-iframe count shared by [`live_140_frame_error_bounded`] and +/// [`live_140_frame_filter_count_accurate`]'s fixtures. +const MANY_IFRAMES_N: usize = 14; + +/// AC: `live_140_frame_error_bounded` — frame-scan error on a many-frame page +/// is bounded in size. Before iter-140, `click_in_scanned_frame`'s `all_urls` +/// joined every frame URL raw and untruncated — 65 KB on theguardian.com's 97 +/// frames. +#[test] +#[ignore = "requires a live Firefox instance — set FF_RDP_LIVE_TESTS=1"] +fn live_140_frame_error_bounded() { + if !live_tests_enabled() { + eprintln!("live_140_frame_error_bounded: set FF_RDP_LIVE_TESTS=1"); + return; + } + let Some(ff) = firefox_with_daemon("live_140_frame_error_bounded") else { + return; + }; + let port = ff.port(); + + let Some(server) = FixtureServer::start(many_iframes_routes(MANY_IFRAMES_N)) else { + eprintln!("live_140_frame_error_bounded: could not bind fixture HTTP — skipping"); + stop_daemon(port); + return; + }; + navigate(port, &server.base_url()); + + // --no-wait skips auto-wait so the missing-selector top-level attempt + // throws immediately and `do_click` runs its own frame scan directly + // (see click.rs's `do_click`/`click_in_scanned_frame`). + let out = run(port, &["click", "--no-wait", "button.nowhere-to-be-found"]); + assert!( + !out.status.success(), + "a selector matching nowhere must fail" + ); + let text = combined(&out); + + assert!( + text.len() < 3000, + "error must be bounded (well under the old 65 KB), got {} bytes: {text}", + text.len() + ); + assert!( + text.contains("more"), + "error must indicate more frames exist beyond the listed cap: {text}" + ); + assert!( + text.contains(&format!("of {} total", MANY_IFRAMES_N + 1)), + "error must report the true total frame count ({}): {text}", + MANY_IFRAMES_N + 1 + ); + + stop_daemon(port); +} + +/// AC: `live_140_frame_filter_count_accurate` — `--frame` reports the +/// filtered candidate count, not the total. Before iter-140, +/// `click_in_scanned_frame`'s zero-match message used `targets.len()` +/// (every frame on the page) even when `--frame` narrowed the scan to a +/// handful of candidates. +#[test] +#[ignore = "requires a live Firefox instance — set FF_RDP_LIVE_TESTS=1"] +fn live_140_frame_filter_count_accurate() { + if !live_tests_enabled() { + eprintln!("live_140_frame_filter_count_accurate: set FF_RDP_LIVE_TESTS=1"); + return; + } + let Some(ff) = firefox_with_daemon("live_140_frame_filter_count_accurate") else { + return; + }; + let port = ff.port(); + + // leaf1, leaf10, leaf11, leaf12, leaf13 all contain the substring "leaf1" + // — 5 of the 14 leaf frames (15 targets total including top). + let Some(server) = FixtureServer::start(many_iframes_routes(MANY_IFRAMES_N)) else { + eprintln!("live_140_frame_filter_count_accurate: could not bind fixture HTTP — skipping"); + stop_daemon(port); + return; + }; + navigate(port, &server.base_url()); + + let out = run( + port, + &[ + "click", + "--no-wait", + "--frame", + "leaf1", + "button.nowhere-to-be-found", + ], + ); + assert!( + !out.status.success(), + "a selector matching nowhere must fail" + ); + let text = combined(&out); + + assert!( + text.contains("5 frame(s) tried") || text.contains("of 5"), + "must report exactly the 5 filtered candidates (leaf1/leaf10/leaf11/leaf12/leaf13), \ + not the full frame count: {text}" + ); + assert!( + text.contains(&format!("of {} total", MANY_IFRAMES_N + 1)), + "must still report the true total ({}) alongside the filtered count: {text}", + MANY_IFRAMES_N + 1 + ); + assert!( + !text.contains(&format!("matched in 0 of {} frame", MANY_IFRAMES_N + 1)), + "must NOT claim every frame was tried when --frame narrowed the scan: {text}" + ); + + stop_daemon(port); +} + +// --------------------------------------------------------------------------- +// Theme F — generated page-map selectors are unique +// --------------------------------------------------------------------------- + +/// AC: `live_140_page_map_selectors_unique` — generated page-map selectors +/// resolve to exactly one element. Before iter-140, a landmark child element +/// with no `id` fell back to a bare tag name (`"selector": "button"`), which +/// matches every button on the page. +#[test] +#[ignore = "requires a live Firefox instance — set FF_RDP_LIVE_TESTS=1"] +fn live_140_page_map_selectors_unique() { + if !live_tests_enabled() { + eprintln!("live_140_page_map_selectors_unique: set FF_RDP_LIVE_TESTS=1"); + return; + } + let Some(ff) = firefox_with_daemon("live_140_page_map_selectors_unique") else { + return; + }; + let port = ff.port(); + + let mut routes = HashMap::new(); + routes.insert( + "/".to_owned(), + FixtureRoute::html( + "t140 page-map\ + \ +
\ +
\ + \ +
", + ), + ); + let Some(server) = FixtureServer::start(routes) else { + eprintln!("live_140_page_map_selectors_unique: could not bind fixture HTTP — skipping"); + stop_daemon(port); + return; + }; + let base = server.base_url(); + navigate(port, &base); + + let out_path = std::env::temp_dir().join(format!("t140-page-map-{port}.json")); + // `index` prints its internal `navigate` call's envelope to stdout ahead + // of its own result line (a pre-existing, separately-tracked output- + // hygiene issue — dogfooding-session-63 finding #21 — unrelated to this + // iteration's Themes A-F), so stdout is not single-object JSON here. + // Read the written page-map file directly instead, matching + // `live_62_page_map_index`'s existing pattern for this same command. + let index_out = run( + port, + &[ + "index", + &base, + "--max-pages", + "1", + "--out", + out_path.to_str().expect("utf-8 temp path"), + ], + ); + assert!( + index_out.status.success(), + "ff-rdp index failed: {}", + combined(&index_out) + ); + + let map_text = std::fs::read_to_string(&out_path) + .unwrap_or_else(|e| panic!("reading page-map {}: {e}", out_path.display())); + let map: Value = serde_json::from_str(&map_text).expect("page-map must be valid JSON"); + let _ = std::fs::remove_file(&out_path); + + // Collect every generated selector: landmark regions, landmark child + // elements, form selectors, field selectors, submit selectors. + let mut selectors: Vec = Vec::new(); + let page = &map["pages"]["index"]; + if let Some(landmarks) = page["landmarks"].as_array() { + for lm in landmarks { + if let Some(s) = lm["region"].as_str() { + selectors.push(s.to_owned()); + } + if let Some(elements) = lm["elements"].as_array() { + for el in elements { + if let Some(s) = el["selector"].as_str() { + selectors.push(s.to_owned()); + } + } + } + } + } + if let Some(forms) = page["forms"].as_array() { + for form in forms { + if let Some(s) = form["selector"].as_str() { + selectors.push(s.to_owned()); + } + if let Some(fields) = form["fields"].as_array() { + for f in fields { + if let Some(s) = f["selector"].as_str() { + selectors.push(s.to_owned()); + } + } + } + if let Some(s) = form["submit"]["selector"].as_str() { + selectors.push(s.to_owned()); + } + } + } + assert!( + selectors.len() >= 6, + "expected several generated selectors (landmarks/form/fields/submit), got {}: {map}", + selectors.len() + ); + // The page has THREE plain