Skip to content

Conversation

@BckupMuthu
Copy link

@BckupMuthu BckupMuthu commented Jan 19, 2026

User description

🔗 Related Issues

#16845

💥 What does this PR do?

Color utility in the JavaScript bindings that focuses on:

Parsing CSS color strings
Converting between formats (hex, rgb, rgba)

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • New feature (non-breaking change which adds functionality and tests!)

PR Type

Enhancement


Description

  • Adds Color utility class for parsing and converting CSS color formats

  • Supports multiple color formats: rgb, rgba, hex, hsl, hsla, named colors

  • Provides conversion methods between formats (asRgb, asRgba, asHex)

  • Includes comprehensive test suite with integration tests


Diagram Walkthrough

flowchart LR
  Input["CSS Color String<br/>rgb, rgba, hex, hsl, hsla, named"] -->|fromString| Color["Color Class<br/>RGBA representation"]
  Color -->|asRgb| RGB["rgb(r, g, b)"]
  Color -->|asRgba| RGBA["rgba(r, g, b, a)"]
  Color -->|asHex| HEX["#rrggbb"]
  Colors["Colors Object<br/>W3C named colors"] -->|lookup| Color
Loading

File Walkthrough

Relevant files
Enhancement
color.js
Core Color class implementation with format parsers           

javascript/selenium-webdriver/lib/color.js

  • Implements Color class with RGBA internal representation
  • Provides static fromString() method supporting 9 color format parsers
  • Implements conversion methods: asRgb(), asRgba(), asHex()
  • Includes Colors object with 147 W3C standard named colors
  • Implements equals() and toString() methods for comparison and
    debugging
+363/-0 
index.js
Export Color utilities in main module                                       

javascript/selenium-webdriver/index.js

  • Imports new color module
  • Exports Color and Colors classes for public API access
+3/-0     
Tests
color_test.js
Comprehensive Color class unit and integration tests         

javascript/selenium-webdriver/test/lib/color_test.js

  • Tests parsing of all supported color formats (rgb, rgba, hex, hsl,
    hsla, named)
  • Validates color format conversions and equality comparisons
  • Integration tests with getCssValue() to verify real-world browser
    color handling
  • Tests edge cases like percentage truncation and alpha channel handling
+143/-0 

@CLAassistant
Copy link

CLAassistant commented Jan 19, 2026

CLA assistant check
All committers have signed the CLA.

@selenium-ci selenium-ci added the C-nodejs JavaScript Bindings label Jan 19, 2026
@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Jan 19, 2026

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🟡
🎫 #16845
🟢 Introduce a small, public Color utility in the JavaScript bindings similar to Selenium
Java org.openqa.selenium.support.Color.
Support parsing CSS color strings returned from getCssValue() (e.g., rgb(...), rgba(...),
hex, named colors).
Provide conversion/normalization helpers between formats (at minimum: hex, rgb, rgba).
Keep scope focused on parsing/format conversion (no advanced color operations).
Confirm cross-browser consistency of getCssValue('background-color') outputs (especially
for % forms and hsl/hsla) across supported browsers/platforms beyond the included
integration coverage.
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
NaN handling missing: Numeric coercion/clamping allows NaN to propagate (e.g., via setOpacity() or direct new
Color(...)), leading to invalid color states/outputs instead of a clear validation error.

Referred Code
constructor(red, green, blue, alpha = 1) {
  this.red_ = Color.#clamp255(red)
  this.green_ = Color.#clamp255(green)
  this.blue_ = Color.#clamp255(blue)
  this.alpha_ = Color.#clamp01(alpha)
}

/**
 * Guesses the input color format and returns a Color instance.
 * @param {string} value
 * @returns {Color}
 */
static fromString(value) {
  const v = String(value)
  for (const conv of [
    Color.#fromRgb,
    Color.#fromRgbPct,
    Color.#fromRgba,
    Color.#fromRgbaPct,
    Color.#fromHex6,
    Color.#fromHex3,


 ... (clipped 152 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Missing numeric validation: Public APIs constructor(...) and setOpacity(alpha) do not validate that inputs are finite
numbers, allowing NaN/non-numeric values to produce malformed color strings rather than
rejecting unsafe/invalid input.

Referred Code
constructor(red, green, blue, alpha = 1) {
  this.red_ = Color.#clamp255(red)
  this.green_ = Color.#clamp255(green)
  this.blue_ = Color.#clamp255(blue)
  this.alpha_ = Color.#clamp01(alpha)
}

/**
 * Guesses the input color format and returns a Color instance.
 * @param {string} value
 * @returns {Color}
 */
static fromString(value) {
  const v = String(value)
  for (const conv of [
    Color.#fromRgb,
    Color.#fromRgbPct,
    Color.#fromRgba,
    Color.#fromRgbaPct,
    Color.#fromHex6,
    Color.#fromHex3,


 ... (clipped 17 lines)

Learn more about managing compliance generic rules or creating your own custom rules

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Jan 19, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Return existing instance for named colors

Refactor the fromNamed method to return the existing Color instance directly
from the Colors map instead of creating a new, redundant one.

javascript/selenium-webdriver/lib/color.js [191-195]

 static #fromNamed(v) {
   const name = String(v).trim().toLowerCase()
-  const c = Colors[name]
-  return c ? new Color(c.red_, c.green_, c.blue_, c.alpha_) : null
+  return Colors[name] || null
 }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: This suggestion correctly points out that creating a new Color instance in fromNamed is redundant. Returning the existing instance from the Colors map is more efficient and ensures that the predefined color objects are treated as singletons.

Medium
Ensure consistent color value rounding

In the clamp255 method, replace Math.round(n) with Math.floor(n) to ensure
consistent color value truncation, aligning with the behavior of the fromRgbPct
parser and the Java implementation.

javascript/selenium-webdriver/lib/color.js [197-199]

 static #clamp255(n) {
-  return Math.max(0, Math.min(255, Math.round(n)))
+  return Math.max(0, Math.min(255, Math.floor(n)))
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies an inconsistency between Math.round() in clamp255 and Math.floor() in fromRgbPct, which is explicitly tested. Aligning them improves consistency and predictability, matching the behavior of the Java implementation.

Low
Make setOpacity chainable

Modify the setOpacity method to return this to enable method chaining.

javascript/selenium-webdriver/lib/color.js [66-68]

 setOpacity(alpha) {
   this.alpha_ = Color.#clamp01(alpha)
+  return this
 }
  • Apply / Chat
Suggestion importance[1-10]: 4

__

Why: This is a good suggestion for improving the API design. Making setOpacity chainable aligns with the fluent interface pattern used elsewhere in the Builder class, enhancing developer experience and code readability.

Low
Learned
best practice
Prevent mutation of shared colors

Colors exports shared Color instances that can currently be mutated via
setOpacity, causing global side effects; make Color immutable (e.g., return a
new instance instead of mutating) and freeze Colors (and/or its entries).

javascript/selenium-webdriver/lib/color.js [66-358]

-setOpacity(alpha) {
-  this.alpha_ = Color.#clamp01(alpha)
+withOpacity(alpha) {
+  return new Color(this.red_, this.green_, this.blue_, Color.#clamp01(alpha))
 }
 ...
-const Colors = {
-  transparent: new Color(0, 0, 0, 0),
-  aliceblue: new Color(240, 248, 255, 1),
-  antiquewhite: new Color(250, 235, 215, 1),
-  aqua: new Color(0, 255, 255, 1),
-  aquamarine: new Color(127, 255, 212, 1),
-  azure: new Color(240, 255, 255, 1),
-  beige: new Color(245, 245, 220, 1),
-  bisque: new Color(255, 228, 196, 1),
-  black: new Color(0, 0, 0, 1),
-  blanchedalmond: new Color(255, 235, 205, 1),
+const Colors = Object.freeze({
+  transparent: Object.freeze(new Color(0, 0, 0, 0)),
+  aliceblue: Object.freeze(new Color(240, 248, 255, 1)),
+  antiquewhite: Object.freeze(new Color(250, 235, 215, 1)),
+  aqua: Object.freeze(new Color(0, 255, 255, 1)),
+  aquamarine: Object.freeze(new Color(127, 255, 212, 1)),
+  azure: Object.freeze(new Color(240, 255, 255, 1)),
+  beige: Object.freeze(new Color(245, 245, 220, 1)),
+  bisque: Object.freeze(new Color(255, 228, 196, 1)),
+  black: Object.freeze(new Color(0, 0, 0, 1)),
+  blanchedalmond: Object.freeze(new Color(255, 235, 205, 1)),
   ...
-}
+})

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - When exposing shared collection-like state, avoid returning mutable/shared instances that callers can mutate (use immutable objects or defensive copies to prevent side effects).

Low
Validate and trim input values

Guard against null/undefined and blank strings and trim the input before parsing
so error messages are clearer and parsing is more robust.

javascript/selenium-webdriver/lib/color.js [43-60]

 static fromString(value) {
-  const v = String(value)
+  if (value == null) {
+    throw new TypeError('Color.fromString requires a non-null string')
+  }
+  const v = String(value).trim()
+  if (!v) {
+    throw new Error('Did not know how to convert empty string into color')
+  }
   for (const conv of [
     Color.#fromRgb,
     Color.#fromRgbPct,
     Color.#fromRgba,
     Color.#fromRgbaPct,
     Color.#fromHex6,
     Color.#fromHex3,
     Color.#fromHsl,
     Color.#fromHsla,
     Color.#fromNamed,
   ]) {
     const c = conv(v)
     if (c) return c
   }
   throw new Error(`Did not know how to convert ${value} into color`)
 }
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why:
Relevant best practice - Add explicit validation/guards at integration boundaries by trimming inputs and checking presence/type before use.

Low
Possible issue
Allow leading‐decimal alpha values

Update the regular expression in fromRgba to correctly parse alpha channel
values that have a leading decimal point but no leading zero (e.g., .5).

javascript/selenium-webdriver/lib/color.js [127-131]

 static #fromRgba(v) {
   const m =
-    /^\s*rgba\(\s*(\d{1,3})\s*,\s*(\d{1,3})\s*,\s*(\d{1,3})\s*,\s*(0|1|0\.\d+)\s*\)\s*$/i.exec(v)
+    /^\s*rgba\(\s*(\d{1,3})\s*,\s*(\d{1,3})\s*,\s*(\d{1,3})\s*,\s*(?:1|0|(?:\d?\.\d+))\s*\)\s*$/i.exec(v)
   return m ? new Color(+m[1], +m[2], +m[3], parseFloat(m[4])) : null
 }
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies that the regex for fromRgba is too restrictive for the alpha channel. While the proposed regex is not perfect as it fails to match 1.0, it improves robustness by allowing valid CSS values like .5, which would otherwise fail to parse.

Low
  • Update

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants