fix: preserve semantic parameter values in overlays - #30
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates Tugboat’s interaction sampling so taps inside overlays (dialogs/popovers) preserve semantic parameter label/value pairs from the visible control, rather than inheriting misleading metadata from obscured controls beneath the overlay. It also adds regression coverage and bumps the package release to 0.4.18.
Changes:
- Prefer complete semantic parameter pairs when merging semantics and when resolving overlay taps via the global semantics tree.
- Add widget test coverage for semantic parameter pair preservation (library + example app).
- Release prep: bump version and update docs/changelog to 0.4.18.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/tugboat/lib/src/anchor_resolver.dart | Uses global semantics hits to resolve overlay interactions and prefer semantic parameter pairs. |
| packages/tugboat/lib/src/control_value.dart | Adjusts semantic annotation merging to preserve ancestor label/value parameter identity. |
| packages/tugboat/test/tugboat_replay_test.dart | Adds regression test ensuring dialog option taps emit semantic parameter label/value. |
| packages/tugboat/example/lib/screens/profile_screen.dart | Adds a semantic parameter-pair demo control for the example app. |
| packages/tugboat/example/test/widget_test.dart | Adds example app test verifying semantic parameter label/value emission. |
| packages/tugboat/README.md | Updates documented package version to 0.4.18. |
| packages/tugboat/pubspec.yaml | Bumps package version to 0.4.18. |
| packages/tugboat/CHANGELOG.md | Documents the overlay semantic parameter-pair fix in 0.4.18. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| final tap = TugboatReplay.controller!.session!.events | ||
| .where((event) => event.type == 'tap') | ||
| .last; | ||
| final semantic = tap.data['semanticAnnotation'] as Map<String, Object?>; |
| // An overlay can sit outside this capture boundary while the global | ||
| // semantics tree still contains the actual control at the tap point. Use | ||
| // a complete semantic parameter pair from that tree in preference to | ||
| // metadata from an obscured control underneath the overlay. | ||
| final hits = _semanticsNodesAt( | ||
| globalPosition: globalPosition, | ||
| rootContext: rootContext, | ||
| rootRender: rootRender, | ||
| ); | ||
| final semanticFromHits = _semanticAnnotationFromHits(hits); | ||
| final semanticPair = | ||
| semanticFromHits?.label != null && semanticFromHits?.value != null; | ||
| final localSemanticPair = | ||
| semanticAnnotation?.label != null && semanticAnnotation?.value != null; | ||
| if (semanticAnnotation == null || (!localSemanticPair && semanticPair)) { | ||
| semanticAnnotation = semanticFromHits ?? semanticAnnotation; | ||
| } | ||
|
|
||
| final controlFromHits = _controlValueFromSemanticsHits(hits); | ||
| if (controlValue == null || | ||
| (!localSemanticPair && | ||
| semanticPair && | ||
| controlValue.sources.contains('semantics'))) { | ||
| controlValue = controlFromHits ?? controlValue; | ||
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
packages/tugboat/lib/src/control_value.dart:428
- When
fallbackDescribesParameteris true, the merged annotation forceslabelto come fromfallbackbut still prefersprimary.value. That can produce a mismatched label/value pair if both are present (label from ancestor parameter, value from descendant), which undermines the “parameter/value pair” intent described in the new doc comment. Consider treatinglabelandvalueas a coupled pair in this case (i.e., if you take the parameter label fromfallback, also prefer the associatedfallback.valueunless there’s a clear reason not to).
final fallbackDescribesParameter =
fallback.label != null && fallback.value != null;
return TugboatSemanticAnnotation(
role: (primary.role != null && primary.role!.isNotEmpty)
? primary.role
: fallback.role,
identifier: primary.identifier ?? fallback.identifier,
label: fallbackDescribesParameter
? fallback.label
: primary.label ?? fallback.label,
value: primary.value ?? fallback.value,
packages/tugboat/lib/src/control_value.dart:413
- The first sentence (“preferring [primary] fields”) is no longer accurate now that
fallback.labelcan intentionally overrideprimary.labelwhen the fallback describes a parameter/value pair. Update the summary sentence to reflect the new precedence rules so future changes don’t accidentally “fix” the behavior back.
/// Merges two annotations, preferring [primary] fields and filling gaps.
///
/// An ancestor that supplies both a label and a value semantically describes a
/// parameter/value pair. Keep that label as the parameter identity instead of
/// replacing it with the descendant button's visible value text.
packages/tugboat/example/test/widget_test.dart:10
- This fixed-duration polling is likely to be flaky across machines/CI loads and also adds a constant ~600ms per call. Prefer waiting on a concrete condition (e.g., until a tap event is observed or until
TugboatReplay.controller?.sessionis non-null / event count increases) with a bounded timeout; that makes the test both faster when possible and more deterministic when slow.
Future<void> _waitForTugboatEvents(WidgetTester tester) async {
for (var attempt = 0; attempt < 12; attempt++) {
await tester.pump(const Duration(milliseconds: 50));
}
}
| final localSemanticPair = | ||
| semanticAnnotation?.label != null && semanticAnnotation?.value != null; | ||
| if (controlValue == null || !localSemanticPair) { | ||
| // An overlay can sit outside this capture boundary while the global | ||
| // semantics tree still contains the actual control at the tap point. | ||
| // Only inspect that tree when local hit-test metadata is incomplete: | ||
| // flushing and walking it is comparatively expensive for every tap. | ||
| final hits = _semanticsNodesAt( | ||
| globalPosition: globalPosition, | ||
| rootContext: rootContext, | ||
| rootRender: rootRender, | ||
| ); | ||
| if (controlValue == null && hits.isNotEmpty) { | ||
| controlValue = tugboatControlValueFromSemanticsNode(hits.last); | ||
| final semanticFromHits = _semanticAnnotationFromHits(hits); | ||
| final semanticPair = | ||
| semanticFromHits?.label != null && semanticFromHits?.value != null; | ||
| if (semanticAnnotation == null || (!localSemanticPair && semanticPair)) { | ||
| semanticAnnotation = semanticFromHits ?? semanticAnnotation; | ||
| } | ||
|
|
||
| final controlFromHits = _controlValueFromSemanticsHits(hits); | ||
| if (controlValue == null || | ||
| (!localSemanticPair && | ||
| semanticPair && | ||
| controlValue.sources.contains('semantics'))) { | ||
| controlValue = controlFromHits ?? controlValue; | ||
| } |
Summary
Validation
Note
The wider tugboat_replay_test.dart file has two existing asset-mask failures because test/assets/red_square.png is absent; they are unrelated to this change.