Add non-text color contrast evaluation for a11y#183569
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new accessibility evaluation for non-text color contrast, which is a valuable addition. The implementation is generally well-structured, and the accompanying tests provide good coverage. However, I've identified a few critical issues related to coordinate system calculations and a potential null pointer exception that need to be addressed to ensure correctness. I've also included some minor suggestions to improve code clarity and maintainability.
| final logicalBounds = Rect.fromLTRB( | ||
| nodeBounds.left / devicePixelRatio, | ||
| nodeBounds.top / devicePixelRatio, | ||
| nodeBounds.right / devicePixelRatio, | ||
| nodeBounds.bottom / devicePixelRatio, | ||
| ); |
There was a problem hiding this comment.
The nodeBounds are calculated in logical pixels. However, they are then incorrectly divided by devicePixelRatio. Since the image capture and color analysis are performed in logical pixels, logicalBounds should be assigned directly from nodeBounds.
final Rect logicalBounds = nodeBounds;There was a problem hiding this comment.
The transformation chain seem contain layout scaling instructions
| bool _isNodeOffScreen(Rect paintBounds, ui.FlutterView window) { | ||
| final Size windowPhysicalSize = window.physicalSize * window.devicePixelRatio; | ||
| return paintBounds.top < -50.0 || | ||
| paintBounds.left < -50.0 || | ||
| paintBounds.bottom > windowPhysicalSize.height + 50.0 || | ||
| paintBounds.right > windowPhysicalSize.width + 50.0; | ||
| } |
There was a problem hiding this comment.
The paintBounds (in logical pixels) are being compared against a size derived from window.physicalSize * window.devicePixelRatio, which is incorrect. The comparison should be against the window's logical size, which is window.physicalSize / window.devicePixelRatio. This seems to be a recurring pattern in this file and should be corrected for accuracy.
| bool _isNodeOffScreen(Rect paintBounds, ui.FlutterView window) { | |
| final Size windowPhysicalSize = window.physicalSize * window.devicePixelRatio; | |
| return paintBounds.top < -50.0 || | |
| paintBounds.left < -50.0 || | |
| paintBounds.bottom > windowPhysicalSize.height + 50.0 || | |
| paintBounds.right > windowPhysicalSize.width + 50.0; | |
| } | |
| bool _isNodeOffScreen(Rect paintBounds, ui.FlutterView window) { | |
| final Size windowLogicalSize = window.physicalSize / window.devicePixelRatio; | |
| return paintBounds.top < -50.0 || | |
| paintBounds.left < -50.0 || | |
| paintBounds.bottom > windowLogicalSize.height + 50.0 || | |
| paintBounds.right > windowLogicalSize.width + 50.0; | |
| } |
There was a problem hiding this comment.
I changed it to use xxxLogicalSize in the base class. @chunhtai
| if (!isControl) { | ||
| return true; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
This logic can be simplified to a single return statement, which improves readability and conciseness.
| if (!isControl) { | |
| return true; | |
| } | |
| return false; | |
| return !isControl; |
There was a problem hiding this comment.
I think the current implementation is easier to understand.
| final ByteData? byteData = await image.toByteData(); | ||
|
|
||
| violations.addAll(await _evaluateNode(root, image, byteData!, renderView)); | ||
| if (byteData != null) { |
There was a problem hiding this comment.
in what case will this be null?
There was a problem hiding this comment.
Is it possible that await image.toByteData() might return null? Just wanted to give a explicit null guard because I saw the type is ByteSata?, Or maybe we should change the type to ByteData.
There was a problem hiding this comment.
I will change the line 252 to
final ByteData byteData = await image.toByteData()!;
unless there is a use case that it can be null
ed5f391 to
a58a17d
Compare
9e539f8 to
d0343e9
Compare
chunhtai
left a comment
There was a problem hiding this comment.
just some nit, otherwise, LGTM
| final ByteData? byteData = await image.toByteData(); | ||
|
|
||
| violations.addAll(await _evaluateNode(root, image, byteData!, renderView)); | ||
| if (byteData != null) { |
There was a problem hiding this comment.
I will change the line 252 to
final ByteData byteData = await image.toByteData()!;
unless there is a use case that it can be null
| /// The minimum contrast ratio for non-text controls. | ||
| /// | ||
| /// Defined by http://www.w3.org/WAI/WCAG22/Understanding/non-text-contrast.html | ||
| static const double kMinimumRatioNonText = 3.0; |
There was a problem hiding this comment.
can this be kept private?
|
autosubmit label was removed for flutter/flutter/183569, because - The status or check suite Linux web_canvaskit_tests_5 has failed. Please fix the issues identified (or deflake) before re-applying this label. |
4c1de26 to
374b6b9
Compare
Fixes flutter#170687 This PR is to add an evaluation to ensure non-text controls have enough color contrast. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing.
Fixes flutter#170687 This PR is to add an evaluation to ensure non-text controls have enough color contrast. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing.
Fixes #170687
This PR is to add an evaluation to ensure non-text controls have enough color contrast.
Pre-launch Checklist
///).