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

[Problem/Bug]: Why has this code stopped showing a a maximized popup window? #4568

Closed
ajtruckle opened this issue May 18, 2024 · 13 comments
Closed
Assignees
Labels
bug Something isn't working

Comments

@ajtruckle
Copy link

What happened?

This code used to show in a popup maximized window. Now it is showing in a Edge browser instance instead:

void CWebBrowser::ShowFullScreen(const CString strFilePath)
{
	if (m_pImpl->m_webView)
	{
		CRect rctSize;
		CWnd::GetDesktopWindow()->GetWindowRect(rctSize);
		rctSize.DeflateRect(10,10);
		CString strJS;

		strJS.Format(L"window.open('%s', 'customWindow', 'width=%d, height=%d, top=%d, left=%d')",
			strFilePath.IsEmpty() ? GetLocationURL() : EncodeFilePathToUrl(strFilePath),
			rctSize.Width(), rctSize.Height(), rctSize.top, rctSize.left);

		m_pImpl->m_webView->ExecuteScript(strJS,
			Callback<ICoreWebView2ExecuteScriptCompletedHandler>(
				[](HRESULT error, PCWSTR result) -> HRESULT { return S_OK; })
			.Get());
	}
}

Importance

Important. My app's user experience is significantly compromised.

Runtime Channel

Stable release (WebView2 Runtime)

Runtime Version

124.0.2478.97

SDK Version

2420.47

Framework

Win32

Operating System

Windows 11

OS Version

23H2

Repro steps

I have the aforementioned button handler that used to show the document in a maximized popup window. And now it shows in a Edge browser instance.

Repros in Edge Browser

No, issue does not reproduce in the corresponding Edge version

Regression

Don't know

Last working version (if regression)

Don't remember, sorry!

@ajtruckle ajtruckle added the bug Something isn't working label May 18, 2024
@johna-ms
Copy link
Contributor

@ajtruckle do you mean that code used to launch a maximized app window containing a webview instance and now shows a plain browser window? Could you provide screenshots of working/broken behavior? I think getting a working version may be difficult if it was last working on 123 since we just released 125. If that is not possible, just broken behavior would be fine.

If the answer to first question is yes, do you have a min repro demonstrating that hosting a new window in a webview by using put_NewWindowRequested handler is broken?

@ajtruckle
Copy link
Author

@johna-ms I think you are on to something.
This is my new window handler:

// Register a handler for the NewWindowRequested event.
	CHECK_FAILURE(m_pImpl->m_webView->add_NewWindowRequested(
		Callback<ICoreWebView2NewWindowRequestedEventHandler>(
			[this](ICoreWebView2* sender, ICoreWebView2NewWindowRequestedEventArgs* args) {

				// get the target Uri
				LPWSTR sUri;
				CHECK_FAILURE(args->get_Uri(&sUri));

				// and if it was user initiated (e.g. click on an anchor tag)
				BOOL bUserInit;
				CHECK_FAILURE(args->get_IsUserInitiated(&bUserInit));

				// and if it is a "file:" Uri
				if (bUserInit && wcsncmp(sUri, L"file:", 5) == 0) {
					// cancel default behavior
					args->put_Handled(TRUE);

					// and open Uri in default browser
					ShellExecute(nullptr, L"open", sUri, nullptr, nullptr, SW_SHOW);
				}

				return S_OK;
			})
		.Get(), nullptr))

By design I have code for viewing in an external browser. I still need this functionality. The last time it worked would have been before I added that code.

So my JavaScript ends up calling this code. How do we allow the JS to carry on for this specific situation?

@ajtruckle
Copy link
Author

@champnic Any suggestion?

@johna-ms
Copy link
Contributor

Sorry I missed the notification for your reply on Monday. So you are explicitly opening the Edge browser to view files that are loaded from disk sounds like. Is the issue that now non-file uris are also opening in the Edge browser? When you debug the code are you hitting your ShellExecute line in both cases?

@ajtruckle
Copy link
Author

@johna-ms
All the data files that my browser control uses are local to the PC. I don't remember why I had to implement this override but there must be a reason. I have done a debug and it ends up here.

So the fact remains that my JavaScript request to do a popup window with specific size of the underlying file is being overlooked. So that do we address that specific scenario because it should be respected.

@ajtruckle
Copy link
Author

@champnic
Is there no way around this?

@ajtruckle
Copy link
Author

I would still like a working solution for this please. A way to get the new event handler to be told to use the default handling rather than my custom handling for this specific context ...

@johna-ms
Copy link
Contributor

johna-ms commented May 31, 2024

@ajtruckle based on your posts I still don't have a clear understanding of your scenario. Are you exclusively opening file uri's in your scenario? If so, then all your new window requests would redirect to ShellExecute. The WebView2 team does not own that API and can make no guarantees as to its behavior.

I'm still not sure what the original behavior was. But when handling the NewWindowRequested event, you have 2 options. You can handle the event (as you are doing) and then open the new window in another WebView2 that you create. Or you can leave the new window unhandled and the dimension parameters passed to window.open should be honored.

You can achieve the first option following examples like the one here: https://learn.microsoft.com/en-us/microsoft-edge/webview2/reference/win32/icorewebview2?view=webview2-1.0.2478.35#add_newwindowrequested

`// Register a handler for the NewWindowRequested event.
CHECK_FAILURE(m_pImpl->m_webView->add_NewWindowRequested(
Callback(
[this](ICoreWebView2* sender, ICoreWebView2NewWindowRequestedEventArgs* args) {
// get the target Uri
LPWSTR sUri;
CHECK_FAILURE(args->get_Uri(&sUri));

			// and if it was user initiated (e.g. click on an anchor tag)
			BOOL bUserInit;
			CHECK_FAILURE(args->get_IsUserInitiated(&bUserInit));

			// and if it is a "file:" Uri
			if (bUserInit && wcsncmp(sUri, L"file:", 5) == 0) {
				// cancel default behavior
				args->put_Handled(TRUE);

                                // open the uri in a webview2 you create with the size you want
                                args->put_NewWindow(my_new_webview);
				// and open Uri in default browser
				// ShellExecute(nullptr, L"open", sUri, nullptr, nullptr, SW_SHOW);
			}

			return S_OK;
		})
	.Get(), nullptr))`

@ajtruckle
Copy link
Author

@johna-ms
The above is not ideal for me.

My application is a CDialog and the browser control sits on the right side of it. So it is never full screen. It always displays a local file that is created by my software on the PC.

But there are times when I want to view the current file in a popup, maximized to the display. My original ShowFullScreen did that and was fine. But there was a reason why I had to add the new window handler for certain situations. I just did not realise that adding the new window handler had the side affect of affecting all my ShowFullScreen calls.

If I added a variable to my browser class saying "we want to show full screen" set to true, then when the new window handler fires I can check the flag and thus let the browser do the normal stuff and show in popup. But then I have a problem of resetting that flag once the popup closes.

It has all gotten messy because new event handler is triggering for everything. and by the time it runs it is clueless that it was being started by javascript code.

The issue itself is simple ...

  1. we have new window handler that should only run for specific situations
  2. for all other situations it should run the default

And sadly I can't do that anymore, without removing the new window handler class which would then affect my parts of my software that needed it.

@johna-ms
Copy link
Contributor

johna-ms commented May 31, 2024

OK so is this a regression or bug in WebView2 code? From what I'm reading it sounds like the NewWindowRequested event is behaving as expected and firing for every new browser/popup window that gets created.

That said, it sounds like your scenario could be addressed pretty handily by hosting the new window in a webview2 that you create. If you create your own window on popup, then you know when that window opens and closes and can set flags accordingly.

@ajtruckle
Copy link
Author

ajtruckle commented May 31, 2024

@johna-ms I think it might be Ok for me to now remove my new window event handler actually. In the last few weeks I added new features that may render the need for it obsolete. I will update you.

@ajtruckle
Copy link
Author

ajtruckle commented May 31, 2024

@johna-ms
I now have a solution.

	CHECK_FAILURE(m_pImpl->m_webView->add_NewWindowRequested(
		Callback<ICoreWebView2NewWindowRequestedEventHandler>(
			[this](ICoreWebView2* sender, ICoreWebView2NewWindowRequestedEventArgs* args) {

				// get the target Uri
				LPWSTR sUri;
				CHECK_FAILURE(args->get_Uri(&sUri));

				// and if it was user initiated (e.g. click on an anchor tag)
				BOOL bUserInit;
				CHECK_FAILURE(args->get_IsUserInitiated(&bUserInit));

				if (bUserInit &&
					wcsncmp(sUri, L"https:", 6) == 0) {

					// cancel default behavior
					args->put_Handled(TRUE);

					// and open Uri in default browser
					ShellExecute(nullptr, L"open", sUri, nullptr, nullptr, SW_SHOW);
				}

				return S_OK;
			})
		.Get(), nullptr));

There is only one situation where I need to do my own handling, and it is when we are displaying a https url. In this case we redirect to pc browser.

Now it is all fine.

@johna-ms
Copy link
Contributor

Alright I'm glad you found a solution that works. I will close this issue now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants