Skip to content

[vector_graphics_compiler] Add modern HSL color parsing#11657

Open
fondoger wants to merge 2 commits intoflutter:mainfrom
fondoger:improve-hsl-color-parsing
Open

[vector_graphics_compiler] Add modern HSL color parsing#11657
fondoger wants to merge 2 commits intoflutter:mainfrom
fondoger:improve-hsl-color-parsing

Conversation

@fondoger
Copy link
Copy Markdown
Contributor

@fondoger fondoger commented May 6, 2026

Description

Follow-up to #11619 and flutter/flutter#185833.

This moves HSL/HSLA function parsing from parser.dart into colors.dart, following the existing parseCssRgb / parseRgbFunction structure. It also adds support for the same practical legacy/modern syntax split already supported for RGB:

  • legacy comma-separated syntax, e.g. hsl(270, 100%, 76%)
  • modern space-separated syntax, e.g. hsl(270 100% 76% / 0.5)

This is intentionally scoped to the existing regex-based numeric parser style; it does not attempt to implement the full CSS Color 4 grammar.

Tests

  • Added parseCssHsl record parser tests covering legacy syntax, modern syntax, whitespace/case variations, decimal/negative tokens, and invalid syntax.
  • Added SvgParser.parseColor tests for modern HSL/HSLA conversion and HSLA alpha behavior.
  • Added HSLA alpha boundary and clamp tests for 0, 0%, 1, 100%, 50%, -0.5, -10%, 2, and 150%.

Ran locally:

flutter test test/colors_test.dart test/parsers_test.dart
dart analyze lib/src/svg/colors.dart lib/src/svg/parser.dart test/colors_test.dart test/parsers_test.dart

Related Issues

Fixes flutter/flutter#186153

Move HSL parsing into colors.dart alongside RGB parsing and add support
for modern space-separated HSL/HSLA syntax. Add HSL parser coverage for
legacy and modern forms, invalid syntax, and alpha clamp behavior.
@github-actions github-actions Bot added p: vector_graphics triage-engine Should be looked at in engine triage labels May 6, 2026
@fondoger
Copy link
Copy Markdown
Contributor Author

fondoger commented May 6, 2026

@andywolff @gaaclarke this is a follow-up PR to #11619.

It addresses the follow-up suggestions from that review:

  • Moves HSL/HSLA parsing into colors.dart as parseCssHsl / parseHslFunction, following the existing RGB parser structure and adding modern space-separated HSL/HSLA syntax support.
  • Adds explicit HSLA alpha boundary/clamp coverage for values such as 0, 0%, 1, 100%, 50%, -0.5, -10%, 2, and 150%.

Related issue: flutter/flutter#186153

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces support for modern space-separated HSL and HSLA color syntax in the vector_graphics_compiler package. It adds new parsing logic and regex patterns, refactors the SVG parser to use these centralized functions, and includes comprehensive unit tests. Feedback recommends optimizing the HSL-to-RGB conversion by reducing list allocations and ensuring that saturation, luminance, and final RGB values are properly clamped.

I am having trouble creating individual review comments. Click here to see my feedback.

packages/vector_graphics_compiler/lib/src/svg/colors.dart (351-391)

medium

The HSL to RGB conversion logic can be optimized by avoiding multiple list allocations and reassignments. Using a simple loop or direct index access is more efficient. Additionally, saturation and luminance values should be clamped to the [0, 100] range, and the final RGB values should be clamped to [0, 255] to ensure they are valid for Color.fromARGB.

Color _cssHslRecordToColor(CssHslRecord record) {
  final double hue = _parseHslValue(record.h) / 360 % 1;
  final double saturation = _parseHslValue(record.s).clamp(0, 100) / 100;
  final double luminance = _parseHslValue(record.l).clamp(0, 100) / 100;
  final int alpha = _parseHslAlpha(record.a);
  final List<double> rgb = <double>[0, 0, 0];

  if (hue < 1 / 6) {
    rgb[0] = 1;
    rgb[1] = hue * 6;
  } else if (hue < 2 / 6) {
    rgb[0] = 2 - hue * 6;
    rgb[1] = 1;
  } else if (hue < 3 / 6) {
    rgb[1] = 1;
    rgb[2] = hue * 6 - 2;
  } else if (hue < 4 / 6) {
    rgb[1] = 4 - hue * 6;
    rgb[2] = 1;
  } else if (hue < 5 / 6) {
    rgb[0] = hue * 6 - 4;
    rgb[2] = 1;
  } else {
    rgb[0] = 1;
    rgb[2] = 6 - hue * 6;
  }

  for (int i = 0; i < 3; i++) {
    rgb[i] += (1 - saturation) * (0.5 - rgb[i]);
    if (luminance < 0.5) {
      rgb[i] *= luminance * 2;
    } else {
      rgb[i] = luminance * 2 * (1 - rgb[i]) + 2 * rgb[i] - 1;
    }
    rgb[i] *= 255;
  }

  return Color.fromARGB(
    alpha,
    rgb[0].round().clamp(0, 255),
    rgb[1].round().clamp(0, 255),
    rgb[2].round().clamp(0, 255),
  );
}

@gaaclarke gaaclarke requested a review from andywolff May 6, 2026 17:56
Clamp HSL saturation and lightness to percentage bounds and clamp final
RGB channel values during HSL conversion. Also avoid repeated list
allocations in the conversion path.
@fondoger
Copy link
Copy Markdown
Contributor Author

fondoger commented May 6, 2026

Thanks for the review note. I pushed a follow-up commit addressing this feedback:

  • Clamps HSL saturation and lightness to the supported percentage range before conversion.
  • Clamps final RGB channels before creating Color.fromARGB.
  • Reworked the HSL-to-RGB conversion to avoid repeated map(...).toList() allocations.
  • Added saturation/lightness clamp tests for values below 0% and above 100%.

The alpha percentage conversion still uses percent / 100 * 255 (rather than * 2.55) to avoid floating-point rounding cases such as 50% producing 127.

Ran locally:

flutter test test/colors_test.dart test/parsers_test.dart
dart analyze lib/src/svg/colors.dart lib/src/svg/parser.dart test/colors_test.dart test/parsers_test.dart

Copy link
Copy Markdown

@andywolff andywolff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@gaaclarke gaaclarke added the CICD Run CI/CD label May 6, 2026
Copy link
Copy Markdown
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks

@gaaclarke gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label May 6, 2026
@auto-submit auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label May 6, 2026
@auto-submit
Copy link
Copy Markdown
Contributor

auto-submit Bot commented May 6, 2026

autosubmit label was removed for flutter/packages/11657, because - The status or check suite Mac_x64 ios_build_all_packages master has failed. Please fix the issues identified (or deflake) before re-applying this label.

@fondoger
Copy link
Copy Markdown
Contributor Author

fondoger commented May 7, 2026

Hi @gaaclarke, could you please trigger another CI/CD run? This run failed due to some transient issues.

@gaaclarke
Copy link
Copy Markdown
Member

Hi @gaaclarke, could you please trigger another CI/CD run? This run failed due to some transient issues.

👍 hit rerun

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CICD Run CI/CD p: vector_graphics triage-engine Should be looked at in engine triage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[vector_graphics_compiler] Align HSL color parsing with RGB parser support

3 participants