fix: adjust ANSI colors for light/dark DevTools theme - #9986
fix: adjust ANSI colors for light/dark DevTools theme#9986enesbugrahankilic wants to merge 1 commit into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces theme-aware ANSI color adjustments to ensure readable text contrast on both light and dark backgrounds across various DevTools screens (logging, console, VM instance display). It updates textSpansFromAnsi and ansiToColor to accept a brightness parameter and adds corresponding unit tests. The review feedback highlights a syntax error in the test file (an extra unmatched }); that breaks the build) and a potential contrast issue where background colors are adjusted using the same logic as foreground colors, suggesting an isBackground flag to skip adjustment for backgrounds.
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
[MUST-FIX] There is an extra unmatched }); on line 711, which will cause a compilation error and break the test suite build. Please remove it.
});
}References
- Prefix every comment with a severity: [MUST-FIX] for logical bugs. (link)
| List<TextSpan> textSpansFromAnsi( | ||
| String input, | ||
| TextStyle defaultStyle, { | ||
| required Brightness brightness, | ||
| }) { | ||
| final parser = AnsiParser(input); | ||
| return parser.parse().map((entry) { | ||
| final styled = entry.bold || entry.fgColor != null || entry.bgColor != null; | ||
| return TextSpan( | ||
| text: entry.text, | ||
| style: styled | ||
| ? TextStyle( | ||
| color: ansiToColor(entry.fgColor), | ||
| backgroundColor: ansiToColor(entry.bgColor), | ||
| color: ansiToColor(entry.fgColor, brightness: brightness), | ||
| backgroundColor: ansiToColor(entry.bgColor, brightness: brightness), | ||
| fontWeight: entry.bold ? FontWeight.bold : FontWeight.normal, | ||
| ) | ||
| : defaultStyle, | ||
| ); | ||
| }).toList(); | ||
| } | ||
|
|
||
| Color? ansiToColor(List<int>? ansiInput) { | ||
| Color? ansiToColor(List<int>? ansiInput, {required Brightness brightness}) { | ||
| if (ansiInput == null) { | ||
| return null; | ||
| } | ||
|
|
||
| assert(ansiInput.length == 3, 'Ansi color list should contain 3 elements'); | ||
| return Color.fromRGBO(ansiInput[0], ansiInput[1], ansiInput[2], 1); | ||
| final color = Color.fromRGBO(ansiInput[0], ansiInput[1], ansiInput[2], 1); | ||
| return _ansiColorVisibleOnBackground(color, brightness); | ||
| } |
There was a problem hiding this comment.
[CONCERN] Adjusting background colors using the same visibility logic as foreground colors can lead to severe contrast issues. For example, if a log specifies a black background and white text on a dark theme, the background color (lightness 0.0 < 0.2) will be adjusted to a light color (lightness 0.65), while the white text (lightness 1.0) remains unadjusted. This results in white text on a light gray background, which is unreadable.
To prevent this, we should avoid adjusting the background color, as it already provides its own contrast boundary for the text. We can add an optional isBackground parameter to ansiToColor to skip adjustment for background colors.
List<TextSpan> textSpansFromAnsi(
String input,
TextStyle defaultStyle, {
required Brightness brightness,
}) {
final parser = AnsiParser(input);
return parser.parse().map((entry) {
final styled = entry.bold || entry.fgColor != null || entry.bgColor != null;
return TextSpan(
text: entry.text,
style: styled
? TextStyle(
color: ansiToColor(entry.fgColor, brightness: brightness),
backgroundColor: ansiToColor(
entry.bgColor,
brightness: brightness,
isBackground: true,
),
fontWeight: entry.bold ? FontWeight.bold : FontWeight.normal,
)
: defaultStyle,
);
}).toList();
}
Color? ansiToColor(
List<int>? ansiInput, {
required Brightness brightness,
bool isBackground = false,
}) {
if (ansiInput == null) {
return null;
}
assert(ansiInput.length == 3, 'Ansi color list should contain 3 elements');
final color = Color.fromRGBO(ansiInput[0], ansiInput[1], ansiInput[2], 1);
return isBackground ? color : _ansiColorVisibleOnBackground(color, brightness);
}References
- Prefix every comment with a severity: [CONCERN] for maintainability issues or logical flaws. (link)
|
Signed the Google CLA — please re-check when convenient. |
1fc2189 to
69fa777
Compare
|
Updated Google Individual CLA with eneskilicresmi@gmail.com and GitHub username enesbugrahankilic — please re-check. |
|
I signed it! |
824f704 to
e54ed31
Compare
ANSI escape codes in log/console output often specify colors that are unreadable against the current DevTools background. Lighten dark colors on dark themes and darken light colors on light themes. Fixes flutter#6345
e54ed31 to
5c99664
Compare
1 participant
Summary
ANSI escape codes in log/console output often specify colors that are unreadable against the current DevTools background (e.g. black text on a dark theme).
This PR passes the current theme brightness into
textSpansFromAnsiand adjusts foreground/background ANSI colors when they would have insufficient contrast:Fixes #6345
Test plan
ansiToColorcontrast adjustment