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

testWindowMaximize is prone to failure #66

Open
uuf6429 opened this issue May 21, 2023 · 10 comments · May be fixed by #75
Open

testWindowMaximize is prone to failure #66

uuf6429 opened this issue May 21, 2023 · 10 comments · May be fixed by #75

Comments

@uuf6429
Copy link
Member

uuf6429 commented May 21, 2023

First of all, the code is not very clear - it took me a few moments to realise what it was doing, all the while I was thinking "but where is the screen height defined and is 100 a sane maximised window height?"

$this->assertLessThanOrEqual(100, $session->evaluateScript($script));

It turns out that the test assumes that when a window is maximized, it will be the height of the screen minus some height for desktop widgets such as a taskbar. This extra height is defined as a buffer of up to 100px.

(it could be helpful to assign values to aptly named variables)

Anyway, while testing with Firefox on Selenium (standalone v2.53.1), it seems that the OS needs 102px - causing the test to fail.

A cursory search resulted in this fairly available API that could be used instead of manually checking the window size:
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/windows/WindowState#browser_compatibility

@aik099
Copy link
Member

aik099 commented May 25, 2023

Anyway, while testing with Firefox on Selenium (standalone v2.53.1), it seems that the OS needs 102px - causing the test to fail.

Does it work with a different Selenium version/Web Browser?

A cursory search resulted in this fairly available API that could be used instead of manually checking the window size:
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/windows/WindowState#browser_compatibility

I like that. Could you please change that test to use that API instead? Also please check if it works fine with Google Chrome.

@uuf6429
Copy link
Member Author

uuf6429 commented May 27, 2023

I did find what the main problem was - the infobar thing in chrome is what was causing the height change. Firefox tests were in fact not failing.

I'll implement the change, no worries.

@uuf6429
Copy link
Member Author

uuf6429 commented May 28, 2023

Hmm, sadly that API is only available to web-extensions.

I would consider changing to the following code though:

        $unusedWidth = $session->evaluateScript('screen.availWidth - window.outerWidth');
        $unusedHeight = $session->evaluateScript('screen.availHeight - window.outerHeight');
        $this->assertLessThanOrEqual(0, $unusedWidth);
        $this->assertLessThanOrEqual(0, $unusedHeight);

Presumably it shouldn't care about negative values. Wdyt?

@aik099
Copy link
Member

aik099 commented Jun 2, 2023

I like this. Adding a window width check was a wise move.

Whatever the 100 number meant it is no longer relevant, because currently window.outerWidth/window.outerHeight returns the dimensions of the Web Browser window itself and not the area, where the website is displayed.

@stof
Copy link
Member

stof commented Jun 14, 2023

@uuf6429 can you send a PR ?

@uuf6429
Copy link
Member Author

uuf6429 commented Jun 17, 2023

I like this. Adding a window width check was a wise move.

Whatever the 100 number meant it is no longer relevant, because currently window.outerWidth/window.outerHeight returns the dimensions of the Web Browser window itself and not the area, where the website is displayed.

Unfortunately, that's not the case. It seems that it depends on the browser and platform. Even worse, these variations haven't been reported as bugs and are undocumentation (fox example mdn/caniuse report full support)

A quick check on Win11+Chrome yields the following:
image
Apparently, even a window resized manually is 8px larger than the available height (and it really didn't fully cover all available area, otherwise pinning would have kicked in). 🤦

@uuf6429 can you send a PR ?

I'll send one soon, however I still feel this test is more trouble than it's worth. 😞

@uuf6429 uuf6429 linked a pull request Jun 17, 2023 that will close this issue
@aik099
Copy link
Member

aik099 commented Jul 2, 2023

@uuf6429 , then the only way to proceed is to:

  1. keep a mysterious 100px margin (which turned out was for the browser incompatibility compensation feature)
  2. add a width checking

?

Then window maximization test (as before) would be testing the window state close to maximized :( , but be cross-browser compatible.

@uuf6429
Copy link
Member Author

uuf6429 commented Jul 2, 2023

@aik099 I see two other possibilities:

  1. make a browser extension, (and ship it as a fixture), that exposes the window state api mentioned before - according to mdn, it should be largely compatible with most browsers: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions
  2. or change the test to not calculate maximized window size and just detect that the window has been moved and resized, e.g.:
    $this->getSession()->visit($this->pathTo('/index.html'));
    $session = $this->getSession();
    $popupName = 'testPopup';
    $createWindowJs =<<<"JS"
        window.open("about:blank", "$popupName", "left=20,top=40,width=300,height=200")
    JS;
    $getWindowPosJs =<<<'JS'
        return {
            top: window.screenY,
            left: window.screenX,
            right: window.screenX + window.innerWidth,
            bottom: window.screenX + window.innerHeight
        }
    JS;
    
    $session->executeScript($createWindowJs);
    $session->switchToWindow($popupName);
    
    $oldDim = (array)$session->evaluateScript($getWindowPosJs);
    
    $session->maximizeWindow($popupName);
    $newDim = (array)$session->evaluateScript($getWindowPosJs);
    
    foreach (array_keys($oldDim) as $name){
        $this->assertNotEquals($oldDim[$name],$newDim[$name], "The popup's $name position should not be the same after maximizing");
    }

@aik099
Copy link
Member

aik099 commented Jul 9, 2023

The 2nd option sounds useful. Since we can't exactly tell if the window has been maximized we at least can tell it has been resized.

@uuf6429
Copy link
Member Author

uuf6429 commented Jul 9, 2023

The 2nd option sounds useful. Since we can't exactly tell if the window has been maximized we at least can tell it has been resized.

Done: #75

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 a pull request may close this issue.

3 participants