FIX: Color.setStyle has parser gaps and silent failures for valid CSS color forms#33197
Open
MITHRAN-BALACHANDER wants to merge 2 commits intomrdoob:devfrom
Open
FIX: Color.setStyle has parser gaps and silent failures for valid CSS color forms#33197MITHRAN-BALACHANDER wants to merge 2 commits intomrdoob:devfrom
MITHRAN-BALACHANDER wants to merge 2 commits intomrdoob:devfrom
Conversation
Enhance Color.setStyle to robustly parse modern CSS color function syntaxes: accepts space-separated components, slash-delimited alpha, percent and numeric RGB values, hue with optional 'deg', and case-insensitive model names. Introduces helper parsers (parseAlpha, parseFunctionComponents, parseRGBValue, parseHue, parsePercent), stricter validation/clamping of components, and preserves behavior for alpha warnings. Adds unit tests covering space-separated RGB/HSL, alpha variants, percent inputs, uppercase model names, and invalid-component handling to prevent color mutation.
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
Owner
|
Nice! Did you use any AI for this? |
Author
|
Yes, I used AI only to help refine the documentation and wording. The issue identification, debugging, refactoring, and implementation were done by me, and I added the unit tests to ensure correctness and avoid regressions. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Color.setStyle()silently left colors unchanged for several valid CSS inputs.This PR fixes the parser gaps, adds case-insensitive model name matching, and adds explicit warnings instead of silent no-ops.
Closes #33195
Problem
rgb(255 0 0)123456(no-op)ff0000✓rgb(255 0 0 / 0.5)123456(no-op)ff0000+ alpha warning ✓hsl(120deg 100% 50%)123456(no-op)00ff00✓rgb(100%,50%,10%,50%)123456(no-op)ff801a+ alpha warning ✓RGB(255,0,0)ff0000✓HSL(120,100%,50%)00ff00✓rgb(255,0,oops)Root causes
1. Separate rigid regexes per token format.
The original parser used two distinct hardcoded regexes for
rgb(integer form, percent form) and one forhsl. Any input that did not match both patterns fell through theswitchwithout warning, returningthissilently.2. Case-sensitive
switchon the model name.RGB,RGBA,HSL,HSLAfell into thedefaultbranch and emitted "Unknown color model", leaving the color unchanged.3. No CSS Color 4 space/slash syntax support.
rgb(255 0 0)andrgb(255 0 0 / 0.5)/hsl(120deg 50% 50%)are valid CSS Color 4 forms. The parser had no path for space-separated values or slash-delimited alpha.4. Percentage alpha was rejected.
rgb(100%,50%,10%,50%)— the alpha50%failed the original\d*\.?\d+numeric-only regex.Changes
src/math/Color.jshandleAlpha— refactored to accept a pre-parsednumberinstead of a raw string, removing redundantparseFloat.parseAlpha(new) — parses an optional alpha token that may be a decimal (0.5) or percentage (50%); returnsundefinedfor absent,nullfor malformed, and a clamped[0,1]value otherwise.parseFunctionComponents(new) — single entry point for tokenizing the argument list of any CSS color function. Handles:255, 0, 0/255, 0, 0, 0.5255 0 0255 0 0 / 0.5parseRGBValue(new) — converts a single RGB channel token (integer0–255or percentage0–100%) to a[0,1]float.parseHue(new) — parses a hue token that may be a bare number or include the optionaldegunit suffix.parsePercent(new) — parses an%token and returnsn/100.m[1].toLowerCase()is applied before theswitch, makingRGB,Rgb, etc. all resolve correctly./^(\w+)\(([^\)]*)\)/to/^\s*([A-Za-z]+)\(([^\)]*)\)\s*$/to anchor the full string and avoid partial matches.rgb/rgbaandhsl/hslabranches now emitColor: Invalid color components <style>when the model was recognised but components could not be parsed, eliminating the silent no-op path.test/unit/src/math/Color.tests.jsSeven new test cases added after the existing
setStyle*block:setStyleRGBSpaceSeparatedrgb(255 0 0)setStyleRGBSpaceSeparatedWithAlphargb(255 0 0 / 0.5)setStyleRGBPercentAlphaPercentrgb(100%,50%,10%,50%)setStyleHSLSpaceSeparatedWithDegreeshsl(120deg 100% 50%)setStyleUpperCaseRGBModelRGB(255,0,0)setStyleUpperCaseHSLModelHSL(120,100%,50%)setStyleInvalidKnownModelDoesNotApplyrgb(255,0,oops)— asserts color is not mutatedTest results
All 67 Color tests pass. No regressions.
Scope / non-goals
hslto acceptrad,turn, orgradangle units — onlydeg(the CSS Color 4 default) is added. Additional unit support can follow in a separate PR.nonekeyword values (CSS Color 4 missing-component syntax) are not handled — out of scope.handleAlphacontinues to emit the existing warning when alpha < 1.Files changed
src/math/Color.jstest/unit/src/math/Color.tests.js