Restore Original Scroll Position After Full Page Screenshot #3967
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/ChromeDriverEx.cs (1)
35-36: Optimize Dimension Calculations if PossibleIf
document.documentElement.scrollWidthanddocument.documentElement.scrollHeightsuffice for capturing the full page dimensions, you might simplify the calculations by removing redundant properties.Consider:
-["width"] = driver.ExecuteScript("return Math.max(window.innerWidth, document.body.scrollWidth, document.documentElement.scrollWidth)"), -["height"] = driver.ExecuteScript("return Math.max(window.innerHeight, document.body.scrollHeight, document.documentElement.scrollHeight)"), +["width"] = driver.ExecuteScript("return document.documentElement.scrollWidth"), +["height"] = driver.ExecuteScript("return document.documentElement.scrollHeight"),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/ChromeDriverEx.cs (1 hunks)
🧰 Additional context used
🔇 Additional comments (6)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/ChromeDriverEx.cs (6)
29-30: Good Practice: Capturing Original Scroll PositionCapturing the original scroll position ensures that the user's view is restored after the screenshot is taken, enhancing user experience.
35-36: Efficient Calculation of Page DimensionsUsing
Math.maxonwindow.innerWidth,document.body.scrollWidth, anddocument.documentElement.scrollWidthaccurately captures the maximum width and height of the page, accommodating various document structures.
37-37: Robust Device Scale Factor RetrievalRetrieving
window.devicePixelRatiowith a fallback to1ensures that the device scale factor is correctly set, even if the script returnsnull.
41-41: Applying Device Metrics for Full Page ScreenshotSetting the device metrics override with the calculated dimensions ensures that the browser emulates the full page size, allowing for a complete screenshot.
44-44: Capturing the Full Page ScreenshotTaking the screenshot after adjusting the device metrics ensures that the entire page content is captured as intended.
48-49: Restoring Original Device Metrics and Scroll PositionClearing the device metrics override and resetting the scroll position maintains the original state of the browser, preventing side effects from the screenshot process.
| // Capture the original scroll position | ||
| Dictionary<string, object> originalScrollPosition = (Dictionary<string, object>)driver.ExecuteScript("return { x: window.pageXOffset, y: window.pageYOffset };"); | ||
|
|
||
| // Capture page dimensions and device metrics | ||
| Dictionary<string, Object> metrics = new Dictionary<string, Object> | ||
| { | ||
| ["width"] = driver.ExecuteScript("return Math.max(window.innerWidth,document.body.scrollWidth,document.documentElement.scrollWidth)"), | ||
| ["height"] = driver.ExecuteScript("return Math.max(window.innerHeight,document.body.scrollHeight,document.documentElement.scrollHeight)") | ||
| ["width"] = driver.ExecuteScript("return Math.max(window.innerWidth, document.body.scrollWidth, document.documentElement.scrollWidth)"), | ||
| ["height"] = driver.ExecuteScript("return Math.max(window.innerHeight, document.body.scrollHeight, document.documentElement.scrollHeight)"), | ||
| ["deviceScaleFactor"] = Convert.ToDouble(driver.ExecuteScript("return window.devicePixelRatio") ?? 1), | ||
| ["mobile"] = driver.ExecuteScript("return typeof window.orientation !== 'undefined'") | ||
| }; | ||
| object devicePixelRatio = driver.ExecuteScript("return window.devicePixelRatio"); | ||
| if (devicePixelRatio != null) | ||
| { | ||
| double doubleValue = 0; | ||
| if (double.TryParse(devicePixelRatio.ToString(), out doubleValue)) | ||
| { | ||
| metrics["deviceScaleFactor"] = doubleValue; | ||
| } | ||
| else | ||
| { | ||
| long longValue = 0; | ||
| if (long.TryParse(devicePixelRatio.ToString(), out longValue)) | ||
| { | ||
| metrics["deviceScaleFactor"] = longValue; | ||
| } | ||
| } | ||
| } | ||
| metrics["mobile"] = driver.ExecuteScript("return typeof window.orientation !== 'undefined'"); | ||
| //Execute the emulation Chrome Command to change browser to a custom device that is the size of the entire page | ||
|
|
||
| // Execute the emulation Chrome command to change browser to a custom device that is the size of the entire page | ||
| driver.ExecuteCdpCommand("Emulation.setDeviceMetricsOverride", metrics); | ||
| //You can then just screenshot it as it thinks everything is visible | ||
|
|
||
| // Take screenshot as everything is now visible | ||
| Screenshot screenshot = driver.GetScreenshot(); | ||
| //This command will return your browser back to a normal, usable form if you need to do anything else with it. | ||
| driver.ExecuteCdpCommand("Emulation.clearDeviceMetricsOverride", []); | ||
|
|
||
| // Reset the device metrics and scroll position to original state | ||
| driver.ExecuteCdpCommand("Emulation.clearDeviceMetricsOverride", new Dictionary<string, object>()); | ||
| driver.ExecuteScript($"window.scrollTo({{ top: {originalScrollPosition["y"]}, left: {originalScrollPosition["x"]}, behavior: 'instant' }});"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider Adding Error Handling for Script Executions
To enhance robustness, add error handling around script executions to manage potential exceptions or unexpected return values.
For example:
try
{
// Script execution code
}
catch (WebDriverException e)
{
// Handle exception
}| ["width"] = driver.ExecuteScript("return Math.max(window.innerWidth, document.body.scrollWidth, document.documentElement.scrollWidth)"), | ||
| ["height"] = driver.ExecuteScript("return Math.max(window.innerHeight, document.body.scrollHeight, document.documentElement.scrollHeight)"), | ||
| ["deviceScaleFactor"] = Convert.ToDouble(driver.ExecuteScript("return window.devicePixelRatio") ?? 1), | ||
| ["mobile"] = driver.ExecuteScript("return typeof window.orientation !== 'undefined'") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve Mobile Detection Logic
The current mobile detection checks if window.orientation is defined. However, this method may not be reliable across all devices and browsers.
Consider using navigator.userAgent or navigator.userAgentData for a more robust mobile detection:
-["mobile"] = driver.ExecuteScript("return typeof window.orientation !== 'undefined'")
+["mobile"] = driver.ExecuteScript("return /Mobi|Android/i.test(navigator.userAgent)")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ["mobile"] = driver.ExecuteScript("return typeof window.orientation !== 'undefined'") | |
| ["mobile"] = driver.ExecuteScript("return /Mobi|Android/i.test(navigator.userAgent)") |
| }; | ||
| object devicePixelRatio = driver.ExecuteScript("return window.devicePixelRatio"); | ||
| if (devicePixelRatio != null) |
There was a problem hiding this comment.
Let's keep the original code, it was added after JS returned inconsistent values in long or sometimes in double. and long fails to case to double.
There was a problem hiding this comment.
Made changes - see latest commit.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/ChromeDriverEx.cs (1)
Line range hint
40-56: Consider simplifying device pixel ratio parsingWhile the current implementation is thorough in handling different numeric types, it could be simplified.
Consider this more concise alternative:
- object devicePixelRatio = driver.ExecuteScript("return window.devicePixelRatio"); - if (devicePixelRatio != null) - { - double doubleValue = 0; - if (double.TryParse(devicePixelRatio.ToString(), out doubleValue)) - { - metrics["deviceScaleFactor"] = doubleValue; - } - else - { - long longValue = 0; - if (long.TryParse(devicePixelRatio.ToString(), out longValue)) - { - metrics["deviceScaleFactor"] = longValue; - } - } - } + object devicePixelRatio = driver.ExecuteScript("return window.devicePixelRatio"); + if (devicePixelRatio != null) + { + try { + metrics["deviceScaleFactor"] = Convert.ToDouble(devicePixelRatio); + } catch (InvalidCastException) { + // Fallback to default if conversion fails + metrics["deviceScaleFactor"] = 1.0; + } + }However, if you prefer the current explicit type handling for better debugging, that's also valid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/ChromeDriverEx.cs (2 hunks)
🔇 Additional comments (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/ChromeDriverEx.cs (1)
32-38: LGTM: Metrics collection implementation is solidThe implementation correctly captures page dimensions and mobile status. The window dimensions calculation properly considers various scenarios (innerWidth/Height, body, and documentElement).
| // Capture the original scroll position | ||
| Dictionary<string, object> originalScrollPosition = (Dictionary<string, object>)driver.ExecuteScript("return { x: window.pageXOffset, y: window.pageYOffset };"); | ||
|
|
There was a problem hiding this comment.
Add error handling and type safety for scroll position capture
While the scroll position capture works, it needs better error handling and type safety.
Apply this diff to improve robustness:
- Dictionary<string, object> originalScrollPosition = (Dictionary<string, object>)driver.ExecuteScript("return { x: window.pageXOffset, y: window.pageYOffset };");
+ Dictionary<string, object> originalScrollPosition;
+ try {
+ originalScrollPosition = (Dictionary<string, object>)driver.ExecuteScript("return { x: window.pageXOffset, y: window.pageYOffset };")
+ ?? new Dictionary<string, object> { ["x"] = 0, ["y"] = 0 };
+ } catch (WebDriverException ex) {
+ // Fallback to default values if script execution fails
+ originalScrollPosition = new Dictionary<string, object> { ["x"] = 0, ["y"] = 0 };
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Capture the original scroll position | |
| Dictionary<string, object> originalScrollPosition = (Dictionary<string, object>)driver.ExecuteScript("return { x: window.pageXOffset, y: window.pageYOffset };"); | |
| Dictionary<string, object> originalScrollPosition; | |
| try { | |
| originalScrollPosition = (Dictionary<string, object>)driver.ExecuteScript("return { x: window.pageXOffset, y: window.pageYOffset };") | |
| ?? new Dictionary<string, object> { ["x"] = 0, ["y"] = 0 }; | |
| } catch (WebDriverException ex) { | |
| // Fallback to default values if script execution fails | |
| originalScrollPosition = new Dictionary<string, object> { ["x"] = 0, ["y"] = 0 }; | |
| } |
| // Execute the emulation Chrome command to change browser to a custom device that is the size of the entire page | ||
| driver.ExecuteCdpCommand("Emulation.setDeviceMetricsOverride", metrics); | ||
| //You can then just screenshot it as it thinks everything is visible | ||
|
|
||
| // Take screenshot as everything is now visible | ||
| Screenshot screenshot = driver.GetScreenshot(); | ||
| //This command will return your browser back to a normal, usable form if you need to do anything else with it. | ||
| driver.ExecuteCdpCommand("Emulation.clearDeviceMetricsOverride", []); | ||
|
|
||
| // Reset the device metrics and scroll position to original state | ||
| driver.ExecuteCdpCommand("Emulation.clearDeviceMetricsOverride", new Dictionary<string, object>()); | ||
| driver.ExecuteScript($"window.scrollTo({{ top: {originalScrollPosition["y"]}, left: {originalScrollPosition["x"]}, behavior: 'instant' }});"); |
There was a problem hiding this comment.
Add error handling for CDP commands and scroll restoration
The screenshot capture and restoration logic needs error handling for CDP commands and scroll operations.
Apply this diff to improve robustness:
- driver.ExecuteCdpCommand("Emulation.setDeviceMetricsOverride", metrics);
-
- Screenshot screenshot = driver.GetScreenshot();
-
- driver.ExecuteCdpCommand("Emulation.clearDeviceMetricsOverride", new Dictionary<string, object>());
- driver.ExecuteScript($"window.scrollTo({{ top: {originalScrollPosition["y"]}, left: {originalScrollPosition["x"]}, behavior: 'instant' }});");
+ Screenshot screenshot = null;
+ try {
+ driver.ExecuteCdpCommand("Emulation.setDeviceMetricsOverride", metrics);
+ screenshot = driver.GetScreenshot();
+ } finally {
+ try {
+ // Always attempt to restore original state
+ driver.ExecuteCdpCommand("Emulation.clearDeviceMetricsOverride", new Dictionary<string, object>());
+ driver.ExecuteScript($"window.scrollTo({{ top: {originalScrollPosition["y"]}, left: {originalScrollPosition["x"]}, behavior: 'instant' }});");
+ } catch (WebDriverException) {
+ // Log warning if restoration fails
+ }
+ }
+
+ if (screenshot == null) {
+ throw new WebDriverException("Failed to capture screenshot");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Execute the emulation Chrome command to change browser to a custom device that is the size of the entire page | |
| driver.ExecuteCdpCommand("Emulation.setDeviceMetricsOverride", metrics); | |
| //You can then just screenshot it as it thinks everything is visible | |
| // Take screenshot as everything is now visible | |
| Screenshot screenshot = driver.GetScreenshot(); | |
| //This command will return your browser back to a normal, usable form if you need to do anything else with it. | |
| driver.ExecuteCdpCommand("Emulation.clearDeviceMetricsOverride", []); | |
| // Reset the device metrics and scroll position to original state | |
| driver.ExecuteCdpCommand("Emulation.clearDeviceMetricsOverride", new Dictionary<string, object>()); | |
| driver.ExecuteScript($"window.scrollTo({{ top: {originalScrollPosition["y"]}, left: {originalScrollPosition["x"]}, behavior: 'instant' }});"); | |
| // Execute the emulation Chrome command to change browser to a custom device that is the size of the entire page | |
| Screenshot screenshot = null; | |
| try { | |
| driver.ExecuteCdpCommand("Emulation.setDeviceMetricsOverride", metrics); | |
| screenshot = driver.GetScreenshot(); | |
| } finally { | |
| try { | |
| // Always attempt to restore original state | |
| driver.ExecuteCdpCommand("Emulation.clearDeviceMetricsOverride", new Dictionary<string, object>()); | |
| driver.ExecuteScript($"window.scrollTo({{ top: {originalScrollPosition["y"]}, left: {originalScrollPosition["x"]}, behavior: 'instant' }});"); | |
| } catch (WebDriverException) { | |
| // Log warning if restoration fails | |
| } | |
| } | |
| if (screenshot == null) { | |
| throw new WebDriverException("Failed to capture screenshot"); | |
| } |
This PR adds functionality to restore the original scroll position after a full page screenshot is taken. This helps avoid the need to add extra actions after screenshots to return the position.
=========================================================
Thank you for your contribution.
Before submitting this PR, please make sure:
Summary by CodeRabbit