Skip to content

Conversation

yvsvarma
Copy link
Contributor

@yvsvarma yvsvarma commented Oct 18, 2025

User description

Summary

  • Goal: Implement PrintOptions in JS mirroring other bindings (Python, Java, .NET, Ruby).
  • Issue: #15072

What’s in this PR

  • New: javascript/webdriver/print_options.js
    • Presets: A4, LETTER, LEGAL, TABLOID (cm; matches other bindings)
    • Custom size: width/height setters
    • Margins: default 1.0 cm each; non‑negative validation
    • Options: orientation (portrait/landscape), scale, shrinkToFit, background, pageRanges (string[])
    • Serialization:
      • { orientation, scale, background, shrinkToFit, page: { width, height }, margin: { top, right, bottom, left }, pageRanges }
  • Tests: javascript/webdriver/test/print_options_test.js
    • Defaults, presets, custom size, margins, pageRanges, serialization

Why

  • Parity with approved implementations across languages
  • Consistency in API and units (cm); Java PageSize values referenced

Scope / Impact

  • JS‑only addition (options + tests). No breaking changes.

Validation

bazel test //javascript/webdriver:test --test_output=errors

References


PR Type

Enhancement


Description

  • Implement PrintOptions class mirroring other language bindings

  • Support predefined paper sizes (A4, Letter, Legal, Tabloid)

  • Add custom page size, margins, orientation, scale configuration

  • Include comprehensive test suite for all PrintOptions features


Diagram Walkthrough

flowchart LR
  A["PrintOptions Class"] --> B["Paper Sizes"]
  A --> C["Margins Configuration"]
  A --> D["Page Settings"]
  B --> E["A4, Letter, Legal, Tabloid"]
  C --> F["Top, Right, Bottom, Left"]
  D --> G["Orientation, Scale, Background"]
  A --> H["Serialization"]
  H --> I["toDictionary Method"]
Loading

File Walkthrough

Relevant files
Enhancement
print_options.js
PrintOptions class with paper sizes and margins                   

javascript/webdriver/print_options.js

  • Introduces PrintOptions class with default settings (portrait
    orientation, 1.0 scale, A4 page size)
  • Defines PrintOrientation enum with portrait and landscape values
  • Implements PageSize class with four predefined paper sizes in
    centimeters
  • Implements Margins class with default 1.0 cm margins and validation
  • Provides setter methods for orientation, scale, page size, margins
    with input validation
  • Includes addPageRange method with regex validation for page range
    format
  • Implements toDictionary serialization method for WebDriver
    communication
+197/-0 
Tests
print_options_test.js
Comprehensive test suite for PrintOptions                               

javascript/webdriver/test/print_options_test.js

  • Tests default page size initialization to A4
  • Validates custom page size setting functionality
  • Tests invalid page size error handling
  • Verifies orientation setting with landscape option
  • Tests custom margins configuration and validation
  • Validates negative margin rejection
  • Tests page range addition and serialization
  • Includes serialization test with multiple options configured
+131/-0 

Copy link
Contributor

qodo-merge-pro bot commented Oct 18, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Test artifact left in

Description: Test file includes a stray Bazel macro-like block and an intentional failing test with
console logging that can cause CI instability and leak logs, which should not be
committed.
print_options_test.js [55-63]

Referred Code
closure_test_suite(
    name = "test",
    data = [
        ":all_files",
        ":deps",
        "//javascript/atoms:deps",
        "//javascript/webdriver/atoms/inject:deps",
    ],
)
Malformed serialization

Description: Serialization references non-existent properties (pageSize, margins) instead of defined
fields (page, margin), producing undefined values and potentially leaking malformed data
to downstream consumers.
print_options.js [179-191]

Referred Code
toDictionary() {
  return {
    orientation: this.orientation,
    scale: this.scale,
    page: { width: this.pageSize.width, height: this.pageSize.height },
    margin: {
      top: this.margins.top,
      right: this.margins.right,
      bottom: this.margins.bottom,
      left: this.margins.left,
    },
    pageRanges: this.pageRanges,
  };
Ticket Compliance
🟡
🎫 #15072
🟢 Add JavaScript support for PrintOptions with predefined default paper sizes (e.g., A4,
Letter, Legal, Tabloid).
Support custom page sizes and margins with sensible defaults and validation.
🔴 Provide options for orientation, scale, shrinkToFit, background, and pageRanges.
Implement serialization of options into the expected structure for WebDriver.
Include tests covering defaults, presets, custom size, margins, pageRanges, and
serialization.
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
No custom compliance provided

Follow the guide to enable custom compliance check.

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

Copy link
Contributor

qodo-merge-pro bot commented Oct 18, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix incorrect property access in serialization

In the toDictionary method, replace the incorrect property names this.pageSize
and this.margins with the correct ones, this.page and this.margin, to prevent a
TypeError during serialization.

javascript/webdriver/print_options.js [180-191]

 return {
   orientation: this.orientation,
   scale: this.scale,
-  page: { width: this.pageSize.width, height: this.pageSize.height },
+  page: { width: this.page.width, height: this.page.height },
   margin: {
-    top: this.margins.top,
-    right: this.margins.right,
-    bottom: this.margins.bottom,
-    left: this.margins.left,
+    top: this.margin.top,
+    right: this.margin.right,
+    bottom: this.margin.bottom,
+    left: this.margin.left,
   },
   pageRanges: this.pageRanges,
 };
  • Apply / Chat
Suggestion importance[1-10]: 10

__

Why: The suggestion correctly identifies a critical bug in the toDictionary method where it accesses incorrect properties (this.pageSize and this.margins), which would cause a TypeError and crash the application.

High
Fix broken serialization test case

Update the testSerialization function to fix multiple errors. Call the correct
method options.toDictionary() instead of options.toJSON(), and adjust the
assertion to expect pageRanges as an array and to match the actual object
returned.

javascript/webdriver/test/print_options_test.js [113-131]

 function testSerialization() {
   const options = new webdriver.PrintOptions();
   options.setOrientation(webdriver.PrintOrientation.LANDSCAPE);
   options.setScale(1.5);
   options.setPageSize(webdriver.PrintOptions.PaperSize.LETTER);
   options.setMargins({ top: 1, right: 1, bottom: 1, left: 1 });
   options.addPageRange('1-5');
 
-  const dict = options.toJSON();
+  const dict = options.toDictionary();
   assertObjectEquals(dict, {
     orientation: 'landscape',
     scale: 1.5,
-    background: false,
-    shrinkToFit: true,
     page: { width: 21.59, height: 27.94 },
     margin: { top: 1, right: 1, bottom: 1, left: 1 },
-    pageRanges: '1-5',
+    pageRanges: ['1-5'],
   });
 }
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies multiple errors in the testSerialization test that would cause it to fail, including a call to a non-existent method and incorrect assertions for the pageRanges type and the overall object structure.

High
High-level
Unify design around dedicated classes

Refactor PrintOptions to consistently use instances of the PageSize and Margins
classes instead of plain objects. This will centralize validation logic and
create a more robust and consistent API.

Examples:

javascript/webdriver/print_options.js [84-159]
class PrintOptions {
  /**
   * Initializes a new instance of the PrintOptions class.
   */
  constructor() {
    this.orientation = PrintOrientation.PORTRAIT;
    this.scale = 1.0;
    this.background = false;
    this.shrinkToFit = true;
    this.page = PrintOptions.PaperSize.A4;

 ... (clipped 66 lines)

Solution Walkthrough:

Before:

class PageSize {
  constructor(width, height) { /* validation */ }
  static get A4() { return new PageSize(21.0, 29.7); }
}

class PrintOptions {
  constructor() {
    this.page = PrintOptions.PaperSize.A4; // Uses plain object
  }
  static get PaperSize() {
    return { A4: { width: 21.0, height: 29.7 } }; // Returns plain object
  }
  setPageSize(pageSize) { // Expects plain object
    if (!pageSize || pageSize.width <= 0 || pageSize.height <= 0) { // Duplicates validation
      throw new Error('Invalid page size dimensions.');
    }
    this.page = pageSize;
  }
}

After:

class PageSize {
  constructor(width, height) { /* validation */ }
  static get A4() { return new PageSize(21.0, 29.7); }
}

class PrintOptions {
  constructor() {
    this.page = PageSize.A4; // Uses PageSize instance
  }
  // static get PaperSize() is removed.

  setPageSize(pageSize) { // Expects PageSize instance
    if (!(pageSize instanceof PageSize)) {
      throw new Error('pageSize must be an instance of PageSize.');
    }
    this.page = pageSize;
  }
}
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a major design inconsistency where PageSize and Margins classes are defined but PrintOptions uses plain objects, leading to redundant validation and a confusing API.

High
General
Correct the expected test error message

In testInvalidPageRanges, update the expected error message in assertThrows from
'Invalid page range format.' to 'Invalid page range: 1-2-3' to match the actual
error thrown by the addPageRange method.

javascript/webdriver/test/print_options_test.js [106-111]

 function testInvalidPageRanges() {
   const options = new webdriver.PrintOptions();
   assertThrows(() => {
     options.addPageRange('1-2-3');
-  }, 'Invalid page range format.');
+  }, 'Invalid page range: 1-2-3');
 }
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that the testInvalidPageRanges test will fail due to a mismatched error message in the assertion, and provides the correct fix to align the test with the implementation.

Medium
  • Update

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant