Proposal: Static Analysis Rule Against Redundant ScrollIntoViewIfNeededAsync Before ClickAsync (AG0047)

[joeldickson]

Overview

This proposal suggests implementing a Roslyn-based static code analyzer rule that detects when developers redundantly call ScrollIntoViewIfNeededAsync() immediately before ClickAsync() on Playwright locators. This pattern is unnecessary as ClickAsync() already performs scrolling internally and may actually hide underlying test issues.

Background

Playwright's ClickAsync() method automatically performs several actionability checks before clicking, including:

1. Ensuring the element is visible
2. Scrolling the element into view if needed
3. Waiting for the element to be enabled
4. Waiting for the element to be stable

Explicitly calling ScrollIntoViewIfNeededAsync() right before ClickAsync() is redundant and can sometimes mask potential issues that would normally be caught by Playwright's built-in actionability checks.

The Problem

When developers use ScrollIntoViewIfNeededAsync() before ClickAsync():

1. Redundancy - The scroll operation is performed twice unnecessarily
2. Masks issues - Tests may pass locally but fail in different environments due to hidden timing problems
3. False security - Gives a false impression that the extra step is needed for reliability
4. Reduced maintainability - Creates more complex code with no benefits
5. Inconsistent patterns - Leads to inconsistent testing practices across the codebase

Bad Examples

// BAD: Redundant scroll before click
await element.ScrollIntoViewIfNeededAsync();
await element.ClickAsync();

// BAD: Redundant scroll with options before click
await element.ScrollIntoViewIfNeededAsync(new() { Timeout = 5000 });
await element.ClickAsync();

// BAD: Redundant scroll with awaited operation in between
await element.ScrollIntoViewIfNeededAsync();
await Task.Delay(100); // Small delay doesn't change the redundancy
await element.ClickAsync();

Good Examples

// GOOD: Let ClickAsync handle scrolling automatically
await element.ClickAsync();

// GOOD: Use timeout options if needed
await element.ClickAsync(new() { Timeout = 10000 });

// GOOD: If you need to specifically verify visibility before clicking
await element.WaitForAsync(new() { State = WaitForSelectorState.Visible });
await element.ClickAsync();

// GOOD: If scrolling is needed for a different operation before clicking
await element.ScrollIntoViewIfNeededAsync();
await element.HoverAsync(); // Non-click operation that benefits from scrolling
// ... other operations
await element.ClickAsync(); // Later click is fine

Detection Strategy

The analyzer will identify:

1. Sequential method calls where ScrollIntoViewIfNeededAsync() is followed by ClickAsync() on the same locator
2. Minimal or no meaningful operations between these method calls
3. Patterns where the same locator variable is used for both operations

Proposed Implementation

Create a Roslyn analyzer that:

1. Identifies invocation of ScrollIntoViewIfNeededAsync() method on Playwright locator objects
2. Tracks when the same locator object is used for a subsequent ClickAsync() call
3. Checks if there are significant operations between these calls
4. Reports a diagnostic when the redundant pattern is detected
5. Provides a code fix to remove the redundant scroll operation

Diagnostic Information

• ID: AG0047
• Category: Playwright
• Default Severity: Warning
• Title: Redundant ScrollIntoViewIfNeededAsync before ClickAsync
• Message: ScrollIntoViewIfNeededAsync is redundant before ClickAsync as clicking already performs scrolling automatically.

Code Fix Provider

The code fix provider will:

1. Remove the redundant ScrollIntoViewIfNeededAsync() call
2. Remove any intervening non-essential statements if appropriate
3. Preserve any comments by moving them to the remaining ClickAsync() line

Benefits

1. Cleaner, more concise test code
2. More reliable tests that properly utilize Playwright's built-in capabilities
3. Reduced test execution time by eliminating redundant operations
4. Consistent application of best practices across test projects
5. Better visibility of actual underlying issues in tests

Implementation Details

The analyzer will need to:

1. Identify method invocation syntax for ScrollIntoViewIfNeededAsync()
2. Track the receiver of this method (the locator variable)
3. Scan subsequent statements for ClickAsync() calls on the same receiver
4. Consider the context of surrounding operations to avoid false positives
5. Handle both synchronous code and async/await patterns

Complexity Considerations

1. Need to handle cases where the locator is re-assigned or modified between calls
2. Need to distinguish between meaningful intervening operations and simple delays
3. Handle cases with nested scopes and control structures

Questions for Discussion

1. Should the rule be extended to cover other similar patterns, like HoverAsync() followed immediately by ClickAsync()?
2. Should we consider specific exceptions where redundant scrolling might be justified?
3. What severity level is most appropriate - warning or error?

References

• [Playwright Actionability Documentation](https://playwright.dev/dotnet/docs/actionability)
• [Locator.ClickAsync Method](https://playwright.dev/dotnet/docs/api/class-locator#locator-click)
• [Locator.ScrollIntoViewIfNeededAsync Method](https://playwright.dev/dotnet/docs/api/class-locator#locator-scroll-into-view-if-needed)