Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reliable Screenshots (resizeWindow and SELENIUM_RESIZE_OFFSET_HEIGHT) #47

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

htho
Copy link

@htho htho commented Mar 1, 2024

Hi,

screenshots are unreliable, because resizeWindow is unreliable.

t.resizeWindow directly uses Seleniums Window.setRect() method, which worked fine for headless mode, but not when used in windowed mode.

This should be fixed now.

But somehow the screenshots I took were 1px smaller than the size set using resizeWindow.
To workaround this, I added SELENIUM_RESIZE_OFFSET_HEIGHT which allows me to set this offset for selenium instead of inside my test code.

I hope you like what I did, and am looking forward to your feedback, which I am happy to apply to my PR.

@alexschwantes
Copy link
Owner

Hi thanks for the contribution.
I would say that for the majority of users, they would run this in headless mode, for which the original solution works cleanly.
I would have thought it worked for windowed mode as well.

Can you describe what happens when you try to do it when running in a real browser, including your setup, and the command that leads to it not working?

@htho
Copy link
Author

htho commented Mar 4, 2024

Hi, there was a comment of yours in the sources of resizeWindow(): // this sets the browser size, not the size of the visible screen so output may vary.
This PR fixes this issue.

The size of the View Port (window.innerWidth and window.innerHeight) is the size of the window minus the window chrome (frame, browser-bars etc.).
So we need to determine the size of the chrome, and add that to the size we want to set using resizeWindow().

When you use t.resizeWindow, you don't care about the browsers chrome, you care about the View Port.

The TestCafe browser tools solve the same problem. I took that algorithm (even reused the variable names) and used that implementation here.

@alexschwantes
Copy link
Owner

alexschwantes commented Mar 8, 2024

I haven't had a chance to test the code. Guess I was interested in what the actuall result of the existing code was when run in a real window, like if you had some sample code and a screenshot for example.

My other thought is, has this problem been identified before with selenium? I had a quick search but couldn't find anything which seems unusal. Based on your description, I would have thought such an issue would have affected others... Because essentially what you are saying is setRect() doesn't work on real windows in selenium? Have you tested it in older browser versions or environments?

Also, I'm not keen on the breaking change. I think there is a more elegant solution to "overload" the method so that it doesn't break existing code.
Ie. all existing async resizeWindow (id, width, height) method calls will continue to work as usual

There are a couple ways to do it, one way is..

/**
    .. jsdoc stuff (add square brackets around optional parameters) check testcafe's resize.js for a full jsdoc.)
    @param {number} [currentWidth] Optional width of the current window
    @param {number} [currentHeight] Optional height of the current window
**/
async resizeWindow (id, width, height, currentWidth, currentHeight) {
    if (typeof currentWidth  === "undefined") {
        // use existing code
        await this.openedBrowsers[id].manage().window().setRect({ width: width, height: height });
    }
    else {
        // alternative code

Additionally is there a way for the code to know if it is being run headless or not and use the new code for normal browsers and existing code for headless operation?

@htho
Copy link
Author

htho commented Mar 11, 2024

Hi,

so I created a repository wit numerous configurations: https://github.com/htho/testcafe-browser-provider-selenium-repro-screenshot

Please note that I was unable to test an old Chrome browser.
I did not find out yet, how to let selenium select a specific version of a ("portable") browser.
Maybe you can help me?

@alexschwantes
Copy link
Owner

Thanks for the repo, it helped frame the issue. I spent some time investigating it.

To summarise the issue.

  1. When resizing a normal browser window with this selenium plugin, the viewport is less than the size specified.
  2. When resizing a headless browser window with this selenium plugin, the viewport is the same size as the specified size (and you believe it should be the same size).
  3. Weird issue where screenshot is 1px less in height (in headless mode).

Explanation

  1. I think is the expected behavior. Let me explain:
    Say for arguement sake, you have a windows machine with a physical screen with a resolution of 800x600. If you maximise a chrome browser on that screen, the viewable area of the browser will be less than 800x600, as there is a taskbar at the top a scroll bar on the side, not to mention potentially a operating system taskbar at the bottom of the screen. From my tests, this results in a viewable area of 784x462. There is always 138px less height and 16px less width than specified. For other browsers, these values are different. This is exactly how a user would experience it on that screen resultuion, and this I believe is the intended result. This is the way the native Selenium webdriver works with its setSize/setRect methods, and the way that native Testcafe's t.resizeWindow method works. See example code below.

  2. For point 2, the headless browser, it seems that Selenium has no concept of the taskbar and scrollbar etc. so when setting the browser size, it actually results in setting the viewable area to that size. This is how native Selenium works (see example below). With native Testcafe however, the headless browser behaves exactly as a normal browser, where the viewport/screenshot is always smaller than the resized window.

  3. Then there is also the unusual bug where the screenshot is 1px less in height in headless mode. I'm not sure what is causing this issue. It doesn't occur in native selenium...

Example code for native Testcafe. There is a bug in your code in the use of window.innerWidth and window.innerHeight, see below for details.
Running this code in windowed mode (npx testcafe chrome resizeWindow.test.js) and headless mode (npx testcafe chrome:headless resizeWindow.test.js) actually produces the exact same results. The Viewport is never equal to the window resized size.

import { ClientFunction } from 'testcafe';
const { PNG } = require( "pngjs");
const { readFileSync } = require('fs');

const targetWidth = 800;
const targetHeight = 600;
const getWindowDimension = ClientFunction(() => {
    return {
        width: window.innerWidth,
        height: window.innerHeight
    };
});

export async function getScreenSize() {
	const fn = ClientFunction(() => {
		return {
			width: $(window).width(),
			height: $(window).height()
		};
	});
	
	return fn();
}

fixture `TestController.resizeWindow`
    .page `https://devexpress.github.io/testcafe/example/`
    .beforeEach(async t => {
        // set size to something else so resize is necessary
        await t.resizeWindow(targetWidth+200, targetHeight+200);
    });

// This passes, but it is not doing what you expect, it is a false positive!
// The window.innerHeight and window.innerWidth actually return the height and width of the window, not the viewport.
// When you run this test you will see that they resize to the same size on the screen as the other tests that fail.
// Windowed: Expected to Pass as it checks window size
// Headless: Expected to Pass as it checks window size
test('check window.innerHeight/innerWidth is equal to window size', async t => {
    await t.resizeWindow(targetWidth, targetHeight);

    let { width, height } = await getWindowDimension();
    console.log(width, height);
    await t.expect([width, height]).eql([targetWidth, targetHeight]); 
});

// This will fail as it checks the real viewport size with jquery. The size matches the screenshot size.
// Windowed: Expected to fail as viewport (783x583px) is not equal to window size 
// Headless: Expected to fail as viewport (783x583px) is not equal to window size 
test('check resize viewport is equal to window size with jquery', async t => {
    await t.resizeWindow(targetWidth, targetHeight);

    let { width, height } = await getScreenSize();
    console.log(width, height);
    await t.expect([width, height]).eql([targetWidth, targetHeight]); 
});

// This will fail as viewport != window size
// Windowed: Expected to fail as screenshot (783x583px) is not equal to window size 
// Headless: Expected to fail as screenshot (783x583px) is not equal to window size 
test('resize screenshot size is equal to window size', async t => {
    await t.resizeWindow(targetWidth, targetHeight);

    await t.takeScreenshot({path: 'resize-window.png'});
    const png = PNG.sync.read(readFileSync('./screenshots/resize-window.png'));
    await t.expect([png.width, png.height]).eql([targetWidth, targetHeight]); 
});

Example code for native Selenium.
It seems the only time that selenium sets the viewport size to the window size is in headless mode. I would say that this is the actual unexpected behavior.

const { PNG } = require( "pngjs");
const { readFileSync } = require('fs');
let fs = require('fs');
const assert = require("assert");
const { Builder, By, until } = require("selenium-webdriver");
const {Options} = require("selenium-webdriver/chrome.js");


const targetHeight = 600;
const targetWidth = 800;
const GRID_HOST = 'http://sundari.local:4444/wd/hub';
const npmRunScript = process.env.npm_lifecycle_event ?? fail("test not run via npm run!");

const allowedChars = "0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ-_";
function mkValidPath(path) {
	return path.split("").map(char => allowedChars.includes(char) ? char : "_").join("");
}

describe("Test resizing", function() {
    let driver;
    const options = new Options();

    beforeEach(async function() {
        driver = await new Builder().usingServer(GRID_HOST).forBrowser("chrome").setChromeOptions(options.addArguments('--headless')).build();
        // driver = await new Builder().usingServer(GRID_HOST).forBrowser("chrome").build();
        // driver = await new Builder().usingServer(GRID_HOST).forBrowser("firefox").build();
        // driver = await new Builder().usingServer(GRID_HOST).forBrowser("MicrosoftEdge").build();

        await driver.get("about:blank");
        await driver.manage().window().setRect({width: targetWidth+200, height: targetHeight+200});
    });

    afterEach(async function() {
        await driver.quit();
    });

    // Note, this checks the actual window size, not the viewport size.
    // Windowed: Expected to pass as window size is 800x600.
    // Headless: Expected to pass as window size is 800x600.
    it("check physical window size is what it is set to", async function() {
        let rect = await driver.manage().window().getRect()
        console.log(rect)
        assert.notEqual(rect.width, targetWidth, "Width not expected to be targetWidth yet!");
        assert.notEqual(rect.height, targetHeight, "Height not expected to be targetHeight yet!");

        await driver.manage().window().setRect({width: targetWidth, height: targetHeight});
        rect = await driver.manage().window().getRect()
        console.log(rect)
        assert.deepEqual([rect.height, rect.width], [targetHeight, targetWidth], "Window size is wrong.");
    });

    // Windowed: Expected to fail as screenshot size is 784x462 px compared to a window size of 800x600.
    // Headless: Expected to pass as screenshot size is 800x600 px compared to a window size of 800x600.
    it("check screenshot size, ie actual viewport size", async function() {
        let initialSize = await driver.manage().window().getRect()
        console.log(initialSize)

        await driver.manage().window().setRect({width: targetWidth, height: targetHeight});

        let newSize = await driver.manage().window().getRect()
        console.log(newSize)
        assert.notEqual(newSize.width, initialSize.width, "Width did not change!");
        assert.notEqual(newSize.height, initialSize.height, "Height did not change!");

        const screenshotPath = `./screenshots/${mkValidPath(npmRunScript)}--${mkValidPath(this.test.title)}.png`;
        
        let encodedString = await driver.takeScreenshot();
        await fs.writeFileSync(screenshotPath, encodedString, 'base64');

        const png = PNG.sync.read(readFileSync(screenshotPath));
        assert.deepEqual([png.height, png.width], [targetHeight, targetWidth], "Screenshot does not have the same number of pixels as the window!");
    });
});

Conclusion

So in final conclusion,

  1. In windowed mode, the plugin works exactly the same as native Testcafe and Selenium, ie viewport < window size.
  2. In headless mode, the plugin works the same way as native Selenium ie. the viewport = window size. There is no way around this unfortunately. Headless Testcafe on the other hand maintains viewport < window size.

If you want to have a set viewport, then I suggest implementing your own method to resizeViewport() instead of setting the window size. I can see from your code that this is what browsertools is actually doing when the 'resize the window', which I find confusing as it is not doing what the method says.

@htho
Copy link
Author

htho commented Mar 15, 2024

Thanks for the effort you made.

Before we continues this discussion, we must make sure we are on the same page.

(1) For me the most important requirement is, that the results of the plugin should be exactly the same as with testcafes built-in browser-providers (which use testcafe-browser-tools).

(2) I am not sure about the wording you use. According to MDN The Viewport is the part of the document you're viewing which is currently visible in its window.
Therefore the Viewport does not include the chrome and it also does not include any content that is only visible when the window is scrolled.

(3) I can observe that for native testcafe browsers, when the size is set using t.resizeWindow(), then the viewport has the dimensions that were passed to resizeWindow. Also the screenshot has the same size as the viewport. This is true for windowed and headless mode.

While TestCafes documentation is ambigous about the exact meaning of t.resizeWindow (Resizes a window to match the height and width parameters.), the documentation of the browser plugin is clearer: Resizes the browser window’s client area to the specified width and height.

(4) We can agree, that the name t.resizeWindow is misleading, and it actually should be named t.resizeViewport.

Can we agree on these 4 points?

@alexschwantes
Copy link
Owner

Thanks for the effort you made.

Before we continues this discussion, we must make sure we are on the same page.

(1) For me the most important requirement is, that the results of the plugin should be exactly the same as with testcafes built-in browser-providers (which use testcafe-browser-tools).

Somewhat agree. I say somewhat, because I don't think its 100% possible. (see point 2 from my conclusion above, ie not possible for headless selenium)

(2) I am not sure about the wording you use. According to MDN The Viewport is the part of the document you're viewing which is currently visible in its window. Therefore the Viewport does not include the chrome and it also does not include any content that is only visible when the window is scrolled.

Yes, it is essentially the size of the screenshot, ie. the visible part of the web page within the browser windw.

(3) I can observe that for native testcafe browsers, when the size is set using t.resizeWindow(), then the viewport has the dimensions that were passed to resizeWindow. Also the screenshot has the same size as the viewport. This is true for windowed and headless mode.

No. This is not the case. Please see my code above for native Testcafe that demonstrates this.
These are the tests for resizing the window size to 800x600.

  • 'check window.innerHeight/innerWidth is equal to window size' tests the screen size with .innerWidth and .innerHeight. This always reads 800x600. which matches the size set. Seems correct, but the following tests prove this is a false positive.
  • 'check resize viewport is equal to window size with jquery' tests the screen size with jQuery to measure the width and height. This gives 783x583px which is smaller than the value from the test above.
  • check screenshot size, ie actual viewport size saves a screenshot of the visible page/viewport and checks it size in pixels. It matches the jQuery size of 783x583px, not the innerWidth/Height size from the first test, which shows that the actual visible page/viewport set by Testcafe is smaller than the resizeWindow of 800x600, and that innerWidth/innerHeight are producing the wrong values.

When you watch the test run, you will see the windows in each test resize to the same physical size on the screen, thus you would expect all tests to have the same final resolution, but they don't. Therefore the first test case is wrong, and the other tests prove that TestCafe resizeWindow() is referring to the physical window, not the viewport or the visible part of the screen.

This is how testing should work. You evaluate what screen resolution most of your users use, and you test for that screen resolution, which would result in having a smaller viewport within a browser that has had its window set to that size.

While TestCafes documentation is ambigous about the exact meaning of t.resizeWindow (Resizes a window to match the height and width parameters.), the documentation of the browser plugin is clearer: Resizes the browser window’s client area to the specified width and height.

Given the above, the documentation seems to be inconsistent. It seems odd that they would require it to work one way with native Testcafe and a different way with a browser provider plugin. Regardless, the actual Testcafe functionality supports the fact that resizeWindow() is referring to the physical browser windows, so I maintain the browser provider plugin documentation is wrong.

(4) We can agree, that the name t.resizeWindow is misleading, and it actually should be named t.resizeViewport.

Can we agree on these 4 points?

@htho
Copy link
Author

htho commented Mar 18, 2024

Hi,

I don't have much time today, but I want to share some results with you: https://github.com/htho/testcafe-repro-screenshot-height

Notice how innerWidth/innerHeight always passes, regardless of content and scrollbars. For our purpose of resizing the window to fit the viewport - the scrollbars should be taken into account.

Scrollbars are 13px wide.

Although jQuerys docucumentation claims it: $( window ).height(); does NOT return the viewports height!
It somehow takes into account the height (but not the width) of the content. It also includes this default 8px margin browsers add to the body.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants