Unverified Commit 8ac94c16 authored by chunhtai's avatar chunhtai Committed by GitHub

MinimumTapTargetGuideline skips nodes at scrollable boundaries (#124615)

fixes https://github.com/flutter/flutter/issues/107615

## Pre-launch Checklist

- [ ] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [ ] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [ ] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [ ] I signed the [CLA].
- [ ] I listed at least one issue that this PR fixes in the description
above.
- [ ] I updated/added relevant documentation (doc comments with `///`).
- [ ] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [ ] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#overview
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[test-exempt]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat
parent 70ca4697
...@@ -121,6 +121,13 @@ class MinimumTapTargetGuideline extends AccessibilityGuideline { ...@@ -121,6 +121,13 @@ class MinimumTapTargetGuideline extends AccessibilityGuideline {
/// A link describing the tap target guidelines for a platform. /// A link describing the tap target guidelines for a platform.
final String link; final String link;
/// The gap between targets to their parent scrollables to be consider as valid
/// tap targets.
///
/// This avoid cases where a tap target is partially scrolled off-screen that
/// result in a smaller tap area.
static const double _kMinimumGapToBoundary = 0.001;
@override @override
FutureOr<Evaluation> evaluate(WidgetTester tester) { FutureOr<Evaluation> evaluate(WidgetTester tester) {
Evaluation result = const Evaluation.pass(); Evaluation result = const Evaluation.pass();
...@@ -149,27 +156,30 @@ class MinimumTapTargetGuideline extends AccessibilityGuideline { ...@@ -149,27 +156,30 @@ class MinimumTapTargetGuideline extends AccessibilityGuideline {
} }
Rect paintBounds = node.rect; Rect paintBounds = node.rect;
SemanticsNode? current = node; SemanticsNode? current = node;
while (current != null) { while (current != null) {
final Matrix4? transform = current.transform; final Matrix4? transform = current.transform;
if (transform != null) { if (transform != null) {
paintBounds = MatrixUtils.transformRect(transform, paintBounds); paintBounds = MatrixUtils.transformRect(transform, paintBounds);
} }
// skip node if it is touching the edge scrollable, since it might
// be partially scrolled offscreen.
if (current.hasFlag(SemanticsFlag.hasImplicitScrolling) &&
_isAtBoundary(paintBounds, current.rect)) {
return result;
}
current = current.parent; current = current.parent;
} }
// skip node if it is touching the edge of the screen, since it might
// be partially scrolled offscreen. final Rect viewRect = Offset.zero & view.physicalSize;
const double delta = 0.001; if (_isAtBoundary(paintBounds, viewRect)) {
final Size physicalSize = view.physicalSize;
if (paintBounds.left <= delta ||
paintBounds.top <= delta ||
(paintBounds.bottom - physicalSize.height).abs() <= delta ||
(paintBounds.right - physicalSize.width).abs() <= delta) {
return result; return result;
} }
// shrink by device pixel ratio. // shrink by device pixel ratio.
final Size candidateSize = paintBounds.size / view.devicePixelRatio; final Size candidateSize = paintBounds.size / view.devicePixelRatio;
if (candidateSize.width < size.width - delta || if (candidateSize.width < size.width - precisionErrorTolerance ||
candidateSize.height < size.height - delta) { candidateSize.height < size.height - precisionErrorTolerance) {
result += Evaluation.fail( result += Evaluation.fail(
'$node: expected tap target size of at least $size, ' '$node: expected tap target size of at least $size, '
'but found $candidateSize\n' 'but found $candidateSize\n'
...@@ -179,6 +189,16 @@ class MinimumTapTargetGuideline extends AccessibilityGuideline { ...@@ -179,6 +189,16 @@ class MinimumTapTargetGuideline extends AccessibilityGuideline {
return result; return result;
} }
static bool _isAtBoundary(Rect child, Rect parent) {
if (child.left - parent.left > _kMinimumGapToBoundary &&
parent.right - child.right > _kMinimumGapToBoundary &&
child.top - parent.top > _kMinimumGapToBoundary &&
parent.bottom - child.bottom > _kMinimumGapToBoundary) {
return false;
}
return true;
}
/// Returns whether [SemanticsNode] should be skipped for minimum tap target /// Returns whether [SemanticsNode] should be skipped for minimum tap target
/// guideline. /// guideline.
/// ///
......
...@@ -4,6 +4,7 @@ ...@@ -4,6 +4,7 @@
import 'package:flutter/gestures.dart'; import 'package:flutter/gestures.dart';
import 'package:flutter/material.dart'; import 'package:flutter/material.dart';
import 'package:flutter/rendering.dart';
import 'package:flutter_test/flutter_test.dart'; import 'package:flutter_test/flutter_test.dart';
void main() { void main() {
...@@ -816,6 +817,36 @@ void main() { ...@@ -816,6 +817,36 @@ void main() {
await expectLater(tester, meetsGuideline(androidTapTargetGuideline)); await expectLater(tester, meetsGuideline(androidTapTargetGuideline));
handle.dispose(); handle.dispose();
}); });
testWidgets('Tap size test can handle partially off-screen items', (WidgetTester tester) async {
final ScrollController controller = ScrollController();
await tester.pumpWidget(
MaterialApp(
home: Scaffold(
appBar: AppBar(title: const Text('Foo')),
body: ListView(
controller: controller,
children: <Widget>[
Padding(
padding: const EdgeInsets.only(left: 10, right: 10),
child: SizedBox(
width: 100,
height: 100,
child: Semantics(container: true, onTap: () {}, child: const Text('hello'))),
),
Container(
height: 1000,
color: Colors.red,
),
]
),
),
)
);
controller.jumpTo(90);
await tester.pump();
await expectLater(tester, meetsGuideline(iOSTapTargetGuideline));
});
}); });
group('Labeled tappable node guideline', () { group('Labeled tappable node guideline', () {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment