diff --git a/CHANGELOG.md b/CHANGELOG.md index b6c7e2d..1d46a50 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,34 @@ together). ### Added +- **A row that needs two columns to name it could not be named.** The row + target identifies by content instead of position, which is the whole + reason it exists — but it took one anchor, and one column is often not + unique. A table with two people called John and two called Doe has no + single anchor that finds John Doe's row. `containing "Doe"` failed with + `matches 2 rows`, correctly and unhelpfully, and the only thing left to + write was `tr:nth-child(2)`: the positional selector this target was built + to remove, reintroduced by hand. + + ```yaml + - Click the "Edit" in the item containing "John" and "Doe" + - assert: the "Email" column of the row containing "John" and "Doe" shows john@example.com + ``` + + Every anchor must be in the SAME row, so the conjunction narrows rather + than widens. Anchors that sit in different rows match nothing rather than + picking one of them, and a conjunction that is still ambiguous gives the + same `matches N rows` error a single anchor does — the diagnostic is not + weakened by having more ways to be specific. + + The quotes delimit and `and` is a separator, the same rule the multi-option + `Select` above uses, for the same reason: an anchor is arbitrary page text. + + This is not the selector growth the charter declines. That clause is about + adopting another framework's idioms; this is flowproof's own + identity-not-position principle reaching a table where one column does not + identify anything. + - **Selecting four options selected one, and said nothing.** `Select` commits through the control's value setter, which is correct and is also a *replacement*. So the obvious spelling — four `Select` steps at one diff --git a/crates/flowproof-adapters/src/web.rs b/crates/flowproof-adapters/src/web.rs index e9e4d71..47c71fa 100644 --- a/crates/flowproof-adapters/src/web.rs +++ b/crates/flowproof-adapters/src/web.rs @@ -72,7 +72,7 @@ const CELL_HINTS: &str = r#"function(){ return JSON.stringify({ field: fieldOf(c), id: id }); }"#; -const CELL_RESOLVER: &str = r#"function(COL, ANCHOR, COLFIELD, ROWID){ +const CELL_RESOLVER: &str = r#"function(COL, ANCHOR, COLFIELD, ROWID, ALSO){ document.querySelectorAll('[data-flowproof-cell]').forEach(function(e){ e.removeAttribute('data-flowproof-cell'); }); @@ -134,7 +134,17 @@ const CELL_RESOLVER: &str = r#"function(COL, ANCHOR, COLFIELD, ROWID){ }); if (!rows.length) continue; var idRow = ROWID ? rows.find(function(r){ return idOf(r) === ROWID; }) : null; - var anchorRows = rows.filter(function(r){ return txt(r).indexOf(ANCHOR) !== -1; }); + // EVERY anchor must be in the SAME row. One column is often not + // unique - two people called John, two called Doe - and requiring the + // conjunction is how a row is named without falling back to position. + var anchorRows = rows.filter(function(r){ + var t = txt(r); + if (t.indexOf(ANCHOR) === -1) return false; + for (var a = 0; a < (ALSO || []).length; a++) { + if (t.indexOf(ALSO[a]) === -1) return false; + } + return true; + }); var chosen = null; if (idRow){ if (txt(idRow).indexOf(ANCHOR) !== -1) chosen = idRow; @@ -168,7 +178,7 @@ const ITEM_CONTAINERS: &str = "li, [role=listitem], [role=row], [role=option], [ /// discarded, so the INNERMOST wins. Exactly one must remain. Returns a /// status: `ok`, `no_match`, `anchor_without_container`, `ambiguous:`, /// or `bad_container`. -const SCOPE_RESOLVER: &str = r#"function(CONTAINER, ANCHOR, CONTAINERID, ITEMS){ +const SCOPE_RESOLVER: &str = r#"function(CONTAINER, ANCHOR, CONTAINERID, ITEMS, ALSO){ document.querySelectorAll('[data-flowproof-scope]').forEach(function(e){ e.removeAttribute('data-flowproof-scope'); }); @@ -186,7 +196,15 @@ const SCOPE_RESOLVER: &str = r#"function(CONTAINER, ANCHOR, CONTAINERID, ITEMS){ var candidates; try { candidates = Array.prototype.slice.call(document.querySelectorAll(selector)); } catch (e) { return 'bad_container'; } - var matching = candidates.filter(function(c){ return txt(c).indexOf(ANCHOR) !== -1; }); + // Every anchor in the SAME container - see the cell resolver. + var matching = candidates.filter(function(c){ + var t = txt(c); + if (t.indexOf(ANCHOR) === -1) return false; + for (var a = 0; a < (ALSO || []).length; a++) { + if (t.indexOf(ALSO[a]) === -1) return false; + } + return true; + }); // Innermost wins: drop any survivor that contains another survivor. var inner = matching.filter(function(c){ return !matching.some(function(o){ return o !== c && c !== o && c.contains(o); }); @@ -739,11 +757,12 @@ impl WebAppDriver { .unwrap_or_else(|| "null".into()) }; format!( - "({CELL_RESOLVER})({col},{anchor},{field},{rowid})", + "({CELL_RESOLVER})({col},{anchor},{field},{rowid},{also})", col = serde_json::Value::from(cell.column.as_str()), anchor = serde_json::Value::from(cell.anchor.as_str()), field = opt(&cell.column_field), rowid = opt(&cell.row_id), + also = serde_json::Value::from(cell.also.clone()), ) } @@ -808,11 +827,12 @@ impl WebAppDriver { .unwrap_or_else(|| "null".into()) }; format!( - "({SCOPE_RESOLVER})({container},{anchor},{id},{items})", + "({SCOPE_RESOLVER})({container},{anchor},{id},{items},{also})", container = serde_json::Value::from(scope.container.as_str()), anchor = serde_json::Value::from(scope.anchor.as_str()), id = opt(&scope.container_id), items = serde_json::Value::from(ITEM_CONTAINERS), + also = serde_json::Value::from(scope.also.clone()), ) } diff --git a/crates/flowproof-agent/src/recorder.rs b/crates/flowproof-agent/src/recorder.rs index 387ff07..e9ead12 100644 --- a/crates/flowproof-agent/src/recorder.rs +++ b/crates/flowproof-agent/src/recorder.rs @@ -214,11 +214,20 @@ fn selectors_for(app: &str, target: &Target, label: Option<&str>) -> Vec { + Target::Cell { + column, + anchor, + also, + } => { let mut payload = serde_json::Map::new(); payload.insert("kind".into(), "cell".into()); payload.insert("column_text".into(), column.as_str().into()); payload.insert("row_anchor".into(), anchor.as_str().into()); + // A new key beside the primary anchor, so a reader that only + // knows one anchor still reads the payload it expects. + if !also.is_empty() { + payload.insert("row_anchor_also".into(), serde_json::json!(also)); + } vec![Selector { tier: SelectorTier::Structural, provenance: flowproof_trace::format::Adapter::Web, @@ -241,11 +250,15 @@ fn selectors_for(app: &str, target: &Target, label: Option<&str>) -> Vec { let mut payload = serde_json::Map::new(); payload.insert("kind".into(), "scoped".into()); payload.insert("container".into(), container.as_str().into()); + if !also.is_empty() { + payload.insert("anchor_also".into(), serde_json::json!(also)); + } payload.insert("container_anchor".into(), anchor.as_str().into()); match inner.as_ref() { Target::Css(css) => payload.insert("inner_css".into(), css.as_str().into()), @@ -903,10 +916,15 @@ fn target_selector(target: &Target) -> Option { ..UiaSelector::default() }) } - Target::Cell { column, anchor } => Some(UiaSelector { + Target::Cell { + column, + anchor, + also, + } => Some(UiaSelector { cell: Some(flowproof_driver::CellQuery { column: column.clone(), anchor: anchor.clone(), + also: also.clone(), ..Default::default() }), ..UiaSelector::default() @@ -914,10 +932,12 @@ fn target_selector(target: &Target) -> Option { Target::Scoped { container, anchor, + also, inner, } => Some(UiaSelector { scope: Some(flowproof_driver::ScopeQuery { container: container.clone(), + also: also.clone(), anchor: anchor.clone(), inner_css: match inner.as_ref() { Target::Css(css) => Some(css.clone()), @@ -3679,6 +3699,7 @@ steps: Target::Scoped { container: "item".into(), anchor: "Invoice 4711".into(), + also: Vec::new(), inner: Box::new(inner), } } @@ -3765,6 +3786,7 @@ steps: target: Target::Cell { column: "Actions".into(), anchor: "Grace Hopper".into(), + also: Vec::new(), }, label: "Actions".into(), dialog: None, diff --git a/crates/flowproof-agent/src/rules.rs b/crates/flowproof-agent/src/rules.rs index fb8c9a3..0a4c546 100644 --- a/crates/flowproof-agent/src/rules.rs +++ b/crates/flowproof-agent/src/rules.rs @@ -54,6 +54,9 @@ pub enum Target { Cell { column: String, anchor: String, + /// Additional anchors that must ALSO be in the same row - the way + /// a row is named when one column is not unique. + also: Vec, }, /// An element addressed INSIDE a container identified by an anchor: /// `the "Amount" in the item containing "Invoice 4711"`. The container @@ -63,6 +66,8 @@ pub enum Target { Scoped { container: String, anchor: String, + /// Additional anchors that must ALSO be in the same container. + also: Vec, inner: Box, }, /// An element addressed inside a SAME-ORIGIN iframe: @@ -1054,17 +1059,19 @@ fn parse_scroll_edge(s: &str) -> Option { enum Scope { Cell { anchor: String, + /// Additional anchors that must ALSO be in the same row. + also: Vec, }, Container { container: String, anchor: String, + /// Additional anchors that must ALSO be in the same container. + also: Vec, }, /// `in|inside the iframe ""` - a whole separate document, not a /// narrower root in this one. It is a Scope because it occupies the /// same target-tail slot and obeys the same one-per-target rule. - Frame { - frame: String, - }, + Frame { frame: String }, } /// The opening of a capture reference, from the crate that resolves them. @@ -1109,13 +1116,37 @@ const CONTAINER_FORM: &str = "a container must be the word `item` or a quoted \ /// Parse `""` and whatever follows it, off the text after /// `containing`. -fn quoted_anchor(after: &str, noun: &str) -> Result<(String, String), String> { +/// `""` or `"" and "" and …` — the anchors that must ALL be found +/// in the same row or container. +/// +/// One column is often not unique (two people called John, two called Doe), +/// and the only way to say the true thing today is `tr:nth-child(2)` — the +/// positional selector this whole target exists to remove. Conjunction is +/// the same identity-not-position idea reaching a table that needs two +/// columns to name a row. +/// +/// `and` between quoted anchors is a separator, never part of an anchor: +/// the quotes delimit, exactly as they do in a multi-option `Select`. +fn quoted_anchors(after: &str, noun: &str) -> Result<(String, Vec, String), String> { let quoted = after .trim_start() .strip_prefix('"') .ok_or_else(|| format!("expected a quoted {noun} after 'containing'"))?; let (anchor, rest) = quoted_label(quoted).ok_or_else(|| format!("unterminated {noun}"))?; - Ok((anchor.to_string(), rest.trim().to_string())) + let mut also = Vec::new(); + let mut rest = rest.trim(); + while let Some(next) = strip_prefix_ci(rest, "and ") { + let Some(quoted) = next.trim_start().strip_prefix('"') else { + break; + }; + let (extra, tail) = quoted_label(quoted).ok_or_else(|| format!("unterminated {noun}"))?; + if extra.is_empty() { + return Err(format!("a {noun} may not be empty")); + } + also.push(extra.to_string()); + rest = tail.trim(); + } + Ok((anchor.to_string(), also, rest.to_string())) } /// `column of|in the row containing ""` - the cell form, which @@ -1123,7 +1154,10 @@ fn quoted_anchor(after: &str, noun: &str) -> Result<(String, String), String> { fn strip_cell_suffix(tail: &str) -> Option> { let after = strip_prefix_ci(tail, "column of the row containing ") .or_else(|| strip_prefix_ci(tail, "column in the row containing "))?; - Some(quoted_anchor(after, "row anchor").map(|(anchor, rest)| (Scope::Cell { anchor }, rest))) + Some( + quoted_anchors(after, "row anchor") + .map(|(anchor, also, rest)| (Scope::Cell { anchor, also }, rest)), + ) } /// Where a container phrase could START: `in the …` / `inside the …`, @@ -1182,10 +1216,18 @@ fn parse_container_phrase(after: &str) -> Option } match parse_container_word(container_text) { Ok(container) => { - return Some( - quoted_anchor(anchor_part, "container anchor") - .map(|(anchor, rest)| (Scope::Container { container, anchor }, rest)), - ); + return Some(quoted_anchors(anchor_part, "container anchor").map( + |(anchor, also, rest)| { + ( + Scope::Container { + container, + anchor, + also, + }, + rest, + ) + }, + )); } Err(reason) => { if first_error.is_none() { @@ -1467,13 +1509,19 @@ fn scoped_target( )); } Ok(match scope { - Scope::Cell { anchor } => Target::Cell { + Scope::Cell { anchor, also } => Target::Cell { column: label.to_string(), anchor, + also, }, - Scope::Container { container, anchor } => Target::Scoped { + Scope::Container { + container, + anchor, + also, + } => Target::Scoped { container, anchor, + also, inner: Box::new(target_from_label(label)), }, Scope::Frame { frame } => Target::Framed { @@ -3974,6 +4022,10 @@ mod tests { "web", r#"Remember the "Amount" in the item containing "Invoice 4711" as amount"#, ), + ( + "web", + r#"Press the "Edit" button in the item containing "John" and "Doe""#, + ), // Interpolation: several references in one step, and literal // text around them. Documented under "A typed value is // interpolated, not evaluated". @@ -4097,6 +4149,10 @@ mod tests { r#"the "Amount" in the "css:[data-test=transaction]" containing "Invoice 4711" shows 50"#, ), ("web", r#"the "Total" shows ${captured.rows} + 1"#), + ( + "web", + r#"the "Email" column of the row containing "John" and "Doe" shows john@example.com"#, + ), ("calc", "display shows 8"), ("notepad", "document contains hello"), ]; @@ -4243,6 +4299,7 @@ mod tests { target: Target::Cell { column: "Actions".into(), anchor: "Ada".into(), + also: Vec::new(), }, name: "href".into(), check: AttrCheck::Value { @@ -5079,6 +5136,66 @@ mod framed_target_tests { /// arbitrary app text: "Rock, Paper and Scissors" is ONE option on some /// page, and an unquoted list could not tell it from three - silently, /// by selecting the wrong set. + /// A row named by two columns, because one is not unique. The + /// alternative today is `tr:nth-child(2)` - the positional selector + /// this target exists to remove. + #[test] + fn a_row_can_be_named_by_more_than_one_anchor() { + assert_eq!( + assert_step(r#"the "Email" column of the row containing "John" and "Doe" shows x"#) + .expect("parses"), + vec![ResolvedAction::AssertText { + target: Target::Cell { + column: "Email".into(), + anchor: "John".into(), + also: vec!["Doe".into()], + }, + expected: "x".into(), + matcher: TextMatch::Contains, + timeout_ms: ASSERT_TIMEOUT_MS, + }] + ); + // The container form takes the same conjunction. + assert_eq!( + plain(r#"Press the "Edit" button in the item containing "John" and "Doe""#) + .expect("parses"), + vec![ResolvedAction::Press { + target: Target::Scoped { + container: "item".into(), + anchor: "John".into(), + also: vec!["Doe".into()], + inner: Box::new(Target::Text("Edit".into())), + }, + label: "Edit".into(), + dialog: None, + }] + ); + // Three is no different from two. + let three = + plain(r#"Click the "Pay" in the item containing "John" and "Doe" and "Overdue""#) + .expect("parses"); + assert!(matches!( + &three[0], + ResolvedAction::Press { target: Target::Scoped { also, .. }, .. } + if also.len() == 2 + )); + // And a single anchor is completely unchanged. + assert_eq!( + plain(r#"Press the "Pay" button in the item containing "Invoice 4711""#) + .expect("parses"), + vec![ResolvedAction::Press { + target: Target::Scoped { + container: "item".into(), + anchor: "Invoice 4711".into(), + also: Vec::new(), + inner: Box::new(Target::Text("Pay".into())), + }, + label: "Pay".into(), + dialog: None, + }] + ); + } + #[test] fn a_multi_select_list_is_delimited_by_quotes_and_nothing_else() { let three = plain( @@ -5388,6 +5505,7 @@ mod scoped_target_tests { Target::Scoped { container: "item".into(), anchor: anchor.into(), + also: Vec::new(), inner: Box::new(inner), } } @@ -5669,6 +5787,7 @@ mod scoped_target_tests { let cell = Target::Cell { column: "Actions".into(), anchor: "Grace Hopper".into(), + also: Vec::new(), }; assert_eq!( plain(r#"Click the "Actions" column of the row containing "Grace Hopper""#) diff --git a/crates/flowproof-driver/src/app.rs b/crates/flowproof-driver/src/app.rs index 86ab224..abebc28 100644 --- a/crates/flowproof-driver/src/app.rs +++ b/crates/flowproof-driver/src/app.rs @@ -52,6 +52,10 @@ pub struct UiaSelector { pub struct CellQuery { pub column: String, pub anchor: String, + /// Additional anchors that must ALSO be present in the same row. Empty + /// for the single-anchor form, which is every trace written before + /// conjunction existed. + pub also: Vec, pub column_field: Option, pub row_id: Option, } @@ -76,6 +80,8 @@ pub struct CellHints { pub struct ScopeQuery { pub container: String, pub anchor: String, + /// Additional anchors that must ALSO be present in the same container. + pub also: Vec, pub inner_css: Option, pub inner_id: Option, pub inner_text: Option, diff --git a/crates/flowproof-replay/src/lib.rs b/crates/flowproof-replay/src/lib.rs index ee3dfaf..9c65811 100644 --- a/crates/flowproof-replay/src/lib.rs +++ b/crates/flowproof-replay/src/lib.rs @@ -97,6 +97,20 @@ fn selector_to_uia(selector: &Selector) -> Option { .and_then(|v| v.as_str()) .map(str::to_string) }; + // A missing key is an EMPTY list, not an error: every trace written + // before conjunction existed simply has no extra anchors. + let get_list = |key: &str| -> Vec { + selector + .payload + .get(key) + .and_then(|v| v.as_array()) + .map(|a| { + a.iter() + .filter_map(|v| v.as_str().map(str::to_string)) + .collect() + }) + .unwrap_or_default() + }; let nth = selector .payload .get("nth") @@ -118,6 +132,9 @@ fn selector_to_uia(selector: &Selector) -> Option { cell: (get("kind").as_deref() == Some("cell")).then(|| flowproof_driver::CellQuery { column: get("column_text").unwrap_or_default(), anchor: get("row_anchor").unwrap_or_default(), + // Absent in every trace written before conjunction, which + // decodes to the single-anchor behaviour unchanged. + also: get_list("row_anchor_also"), column_field: get("column_field"), row_id: get("row_id"), }), @@ -144,6 +161,7 @@ fn selector_to_uia(selector: &Selector) -> Option { flowproof_driver::ScopeQuery { container: get("container").unwrap_or_default(), anchor: get("container_anchor").unwrap_or_default(), + also: get_list("anchor_also"), inner_css: get("inner_css"), inner_id: get("inner_id"), inner_text: get("inner_text").or_else(|| get("inner_name")), diff --git a/docs/authoring.md b/docs/authoring.md index 5b78366..c516271 100644 --- a/docs/authoring.md +++ b/docs/authoring.md @@ -223,6 +223,10 @@ content and then address the element inside it: # a container the `item` rung cannot see: name it - Click the "Ship" in the "css:.card" containing "Order 8801" + +# one column does not always name a row: require both +- Click the "Edit" in the item containing "John" and "Doe" +- assert: the "Email" column of the row containing "John" and "Doe" shows john@example.com ``` The same cell target composes with every predicate (`shows`, `is empty`, @@ -241,6 +245,7 @@ a real cell, which passes as confidently as the right one. | Form | Notes | |---|---| | `the "" column of the row containing ""` | a table cell; `in the row containing` also works - the of/in coin flip is one you should not have to remember | +| `… containing "" and ""` | on either scoped form: EVERY anchor must be in the SAME row or item. For when one column does not name a row - two people called John, two called Doe. The quotes delimit and `and` is a separator, exactly as in a multi-option `Select`. Anchors sitting in different rows match nothing rather than picking one, and a conjunction that is still ambiguous gives the same "matches N rows" error a single anchor does | | `the "" in the item containing ""` | `item` means exactly `li`, `[role=listitem]`, `[role=row]`, `[role=option]`, `[role=article]`, `tr` - a closed list, not a guess | | `the "" inside the item containing ""` | `inside` is a synonym for `in` | | `the "" in the "css:" containing ""` | any container, named explicitly; `"id:"` too | diff --git a/docs/trace-format.md b/docs/trace-format.md index 3d7a3bc..3dc330c 100644 --- a/docs/trace-format.md +++ b/docs/trace-format.md @@ -143,6 +143,14 @@ these fields existed) is byte-identical: matches nothing and so does an empty table, and the step that means zero is an `assert` with `element_count: 0`. + A `kind: "cell"` payload may carry `row_anchor_also: [...]`, and a + `kind: "scoped"` payload `anchor_also: [...]` — the ADDITIONAL anchors + that must all be present in the same row or container, beside the primary + `row_anchor`/`container_anchor`. A new key rather than a changed one, so a + reader that knows only one anchor still finds the field it expects; absent + in every trace written before conjunction existed, which decodes to the + single-anchor behaviour unchanged. + A `type_text` step may carry `params.values: [...]` — a **multi-selection**, the whole set committed at once. Where `values` is present it is authoritative; `params.text` repeats only the first option, so a reader