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"]
File Walkthrough
| Relevant files |
|---|
| Enhancement |
print_options.jsPrintOptions 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.jsComprehensive 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 |
|
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 --update_compliance=true -->
|
Compliance status legend
🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label
PR Code Suggestions ✨
Explore these optional code suggestions:
| Category | Suggestion | 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 <!-- /improve --apply_suggestion=0 -->
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 <!-- /improve --apply_suggestion=1 -->
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 <!-- /improve --apply_suggestion=3 -->
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 <!-- /improve_multi --more_suggestions=true -->
| |
HI @yvsvarma ,
JS does this already.
https://github.com/SeleniumHQ/selenium/blob/da46ab01cec2108f31b149b507b8bc9b47d28642/javascript/selenium-webdriver/lib/webdriver.js#L1157C3-L1157C26
SeleniumHQ
![qodo-code-review[bot] avatar](https://avatars.githubusercontent.com/in/484649?v=4 "Published by a pub.dev verified publisher"){kind=small}
