Skip to content

Conversation

@nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Nov 15, 2025

User description

New pattern detected.

https://w3c.github.io/webdriver-bidi/#command-browsingContext-setViewport

browsingContext.SetViewportParameters = {
  ? context: browsingContext.BrowsingContext,
  ? viewport: browsingContext.Viewport / null,
  ? devicePixelRatio: float / null,
  ? userContexts: [+browser.UserContext],
}

Here viewport property is nullable. But if remote-end faces real null in json, then it behaves differently. So we just always send null if it is null.

🔧 Implementation Notes

Mark this special nullable with:

[property: JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]

🔄 Types of changes

  • Cleanup (formatting, renaming)
  • Bug fix (backwards compatible)

PR Type

Bug fix, Enhancement


Description

  • Add JsonIgnore attribute to Viewport property for null serialization

  • Enable viewport reset capability via BiDi setViewport command

  • Ensure null viewport values are properly sent to remote endpoint


Diagram Walkthrough

flowchart LR
  A["SetViewportParameters"] -->|"Add JsonIgnore attribute"| B["Viewport property"]
  B -->|"WhenWritingNull condition"| C["Null values serialized correctly"]
  C -->|"Enables"| D["Viewport reset capability"]
Loading

File Walkthrough

Relevant files
Bug fix
SetViewportCommand.cs
Add JsonIgnore attribute for null viewport serialization 

dotnet/src/webdriver/BiDi/BrowsingContext/SetViewportCommand.cs

  • Added System.Text.Json.Serialization using statement
  • Applied [property: JsonIgnore(Condition =
    JsonIgnoreCondition.WhenWritingNull)] attribute to Viewport? property
    in SetViewportParameters record
  • Enables proper serialization of null viewport values to reset viewport
    via BiDi protocol
+2/-1     

@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Nov 15, 2025

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
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
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: Comprehensive Audit Trails

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

Status:
No auditing: The new serialization behavior for nullable viewport is not accompanied by any logging or
audit trail, and it’s unclear whether this action is captured elsewhere.

Referred Code
using OpenQA.Selenium.BiDi.Communication;
using System.Text.Json.Serialization;

namespace OpenQA.Selenium.BiDi.BrowsingContext;

internal sealed class SetViewportCommand(SetViewportParameters @params)
    : Command<SetViewportParameters, SetViewportResult>(@params, "browsingContext.setViewport");

internal sealed record SetViewportParameters(BrowsingContext Context, [property: JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] Viewport? Viewport, double? DevicePixelRatio) : Parameters;

public sealed class SetViewportOptions : CommandOptions
{
    public Viewport? Viewport { get; set; }

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:
Null handling change: Changing to ignore null during serialization may mask intentional nulls versus omitted
fields without explicit error handling or validation shown in this diff.

Referred Code
internal sealed record SetViewportParameters(BrowsingContext Context, [property: JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] Viewport? Viewport, double? DevicePixelRatio) : Parameters;

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:
Input validation: The change adjusts serialization of nullable viewport but shows no validation for
devicePixelRatio or viewport content, which may rely on upstream checks not visible here.

Referred Code
internal sealed record SetViewportParameters(BrowsingContext Context, [property: JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] Viewport? Viewport, double? DevicePixelRatio) : Parameters;

public sealed class SetViewportOptions : CommandOptions
{
    public Viewport? Viewport { get; set; }

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

@selenium-ci selenium-ci added the C-dotnet .NET Bindings label Nov 15, 2025
: Command<SetViewportParameters, SetViewportResult>(@params, "browsingContext.setViewport");

internal sealed record SetViewportParameters(BrowsingContext Context, Viewport? Viewport, double? DevicePixelRatio) : Parameters;
internal sealed record SetViewportParameters(BrowsingContext Context, [property: JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] Viewport? Viewport, double? DevicePixelRatio) : Parameters;
Copy link
Member Author

@nvborisenko nvborisenko Nov 15, 2025

Choose a reason for hiding this comment

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

Nice catch, we already started doing it in another places. Let's find all occurences of this type (property: type / null) and mark all with this attribute.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, interesting: WhenWritingNull or WhenWritingDefault. We want to send json null obly if user explicitly wrote SomeNullableProperty = null.

Copy link
Member Author

@nvborisenko nvborisenko Nov 16, 2025

Choose a reason for hiding this comment

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

Verified, in both cases WhenWritingNull and WhenWritingDefault it doesn't serializes to json null.

await context.SetViewportAsync(new() { Viewport = null });
await context.SetViewportAsync();
    19:40:02.793 TRACE WebSocketTransport: BiDi SND --> {"id":3,"method":"browsingContext.setViewport","params":{"context":"D0E4F713FD009541B7FADB9D86BC5D6C"}}
19:40:02.795 TRACE WebSocketTransport: BiDi RCV <-- {"id":3,"result":{},"type":"success"}
19:40:02.796 TRACE WebSocketTransport: BiDi SND --> {"id":4,"method":"browsingContext.setViewport","params":{"context":"D0E4F713FD009541B7FADB9D86BC5D6C"}}
19:40:02.798 TRACE WebSocketTransport: BiDi RCV <-- {"id":4,"result":{},"type":"success"}

Copy link
Member Author

Choose a reason for hiding this comment

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

LOL, we have used JsonIgnoreCondition.Never for these cases :D

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it always sends null:

19:46:18.576 TRACE WebSocketTransport: BiDi SND --> {"id":4,"method":"browsingContext.setViewport","params":{"context":"EAEFDFDBD3A2386DD196A128BE21FF1E","viewport":null}}

But I am more interesting whether it's possibly to send null only and only when user explicitly set it to null. Is it poossible?

Copy link
Member Author

Choose a reason for hiding this comment

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

Bad news: if we send null then browser resets viewport, but user didn't want to do it.

So we really want to understand how to:

send null only and only when user explicitly set it to null

@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Nov 15, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Correct JSON serialization for optional parameters
Suggestion Impact:The commit modified the JsonIgnore behavior on the Viewport property, changing its condition to Never to ensure null is serialized, addressing the core issue raised about Viewport. It did not move the attribute to DevicePixelRatio as suggested.

code diff:

-internal sealed record SetViewportParameters(BrowsingContext Context, [property: JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] Viewport? Viewport, double? DevicePixelRatio) : Parameters;
+internal sealed record SetViewportParameters(BrowsingContext Context, [property: JsonIgnore(Condition = JsonIgnoreCondition.Never)] Viewport? Viewport, double? DevicePixelRatio) : Parameters;

To align with the WebDriver BiDi specification, correct the JSON serialization
by removing the JsonIgnore attribute from the Viewport property and adding it to
the DevicePixelRatio property.

dotnet/src/webdriver/BiDi/BrowsingContext/SetViewportCommand.cs [28]

-internal sealed record SetViewportParameters(BrowsingContext Context, [property: JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] Viewport? Viewport, double? DevicePixelRatio) : Parameters;
+internal sealed record SetViewportParameters(BrowsingContext Context, Viewport? Viewport, [property: JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] double? DevicePixelRatio) : Parameters;

[Suggestion processed]

Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies that the PR's change to Viewport is incorrect, as it prevents resetting the viewport by sending a null value. However, the proposed fix of moving the attribute to DevicePixelRatio is also incorrect, as the BiDi spec requires sending null to reset that value as well.

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.

2 participants