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

WPF - Capture/Release mouse to allow scrolling outside bounds of browser control #2871

Closed
wants to merge 2 commits into from
Closed

WPF - Capture/Release mouse to allow scrolling outside bounds of browser control #2871

wants to merge 2 commits into from

Conversation

mol
Copy link
Contributor

@mol mol commented Aug 22, 2019

Fixes #2870 and #2060

Summary:
I re-added and improved the previously suggested fix to the no scrolling outside browser issue, which it turns out also fixes the selection issue when dragging past the viewable area.

I know you're not super keen on Mouse.Capture, which I totally understand, but I think it's the only way to accomplish it after having read up on how it works and its implementation in the reference source. PreviewMouseDownOutsideCapturedElement I don't believe would accomplish anything related to this.

Changes:
I re-added the Mouse.Capture(this) and Mouse.Capture(null) calls in OnMouseDown and OnMouseUp respectively, but in a way so the mouse is only captured on left mouse down (I can't think of a reason to capture the other buttons).

I also removed the code in OnMouseLeave that was sending a mouse up event when the left mouse is pressed, as it didn't have any effect when testing with these other changes (so I assume is no longer required). But yeah it doesn't seem to do anything at all in relation with the changes though so it could also be kept.

How Has This Been Tested?

Tested in Windows 10 x64. I believe I've looked through all of the issues, commits and pull requests in relation to these issues and I've tested that combo boxes still work and doesn't cause any crashes, as well as every imaginable scenario of button presses to make sure nothing has been broken as a result. I'm happy to run this out to our users and report back in a while if you'd prefer to err on the side of caution though.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Updated documentation

Checklist:

  • Tested the code(if applicable)
  • Commented my code
  • Changed the documentation(if applicable)
  • New files have a license disclaimer
  • The formatting is consistent with the project (project supports .editorconfig)

@AppVeyorBot
Copy link

@amaitland
Copy link
Member

I know you're not super keen on Mouse.Capture, which I totally understand, but I think it's the only way to accomplish it after having read up on how it works and its implementation in the reference source.

I'll admit a capture is likely the only way to get the usability that everyone expects. Adding a link to #1723 for future reference, if this does crash then the stack trace should be compared.

I re-added the Mouse.Capture(this) and Mouse.Capture(null) calls in OnMouseDown and OnMouseUp respectively, but in a way so the mouse is only captured on left mouse down (I can't think of a reason to capture the other buttons).

I was trying to avoid performing a capture on every down/up combination. As part of #2258 my initial attempt was c843317 That was a long time ago, the only major problem I rememeber it had was leaving would always perform a capture even if a parent control had a capture at the time, an additional not null check on Mouse.Captured should probably be enough to resolve that. The change was released as part version 65, so it was in use for a short period of time and I don't remember any reports of crashing.

But yeah it doesn't seem to do anything at all in relation with the changes though so it could also be kept.

With a capture in place the code should be removed.

I've tested that combo boxes still work and doesn't cause any crashes,

The mouse capture was removed and replaced with directly forwarding the events from the WPF Popup, subsequently the Popup was removed and replaced with drawing the image on a canvas directly over the rendered image. So there shouldn't be any conflicts. The downside of rendering the Popup on a Canvas is it's constrained within the Control. We may need to revert to using a WPF Popup as part of #2820

I'm happy to run this out to our users and report back in a while if you'd prefer to err on the side of caution though.

Sounds good to me, let me know how it goes 👍

@amaitland amaitland changed the title Fixes selection issue and the issue with no scrolling when outside the browser WPF - Capture/Release mouse to allow scrolling outside bounds of browser Aug 23, 2019
@amaitland amaitland changed the title WPF - Capture/Release mouse to allow scrolling outside bounds of browser WPF - Capture/Release mouse to allow scrolling outside bounds of browser control Aug 23, 2019
@amaitland amaitland added the wpf label Aug 23, 2019
@amaitland
Copy link
Member

Took me a while to remember the reason I actually abandoned the initial set of changes in #2258 (c843317). So this will actually reintroduce #2060 when the mouse exits the WPF Window (same problem occurs with my attempted fix).

Slightly more detailed steps to reproduce than in #2060 would be:

  • Open WPF Example
  • Open new Tab (google.com should be displayed, this is important as we need to leave the window whilst scrolling)
  • Press the Restore Down menu bar shortcut (top right) or alternatively press windows key + down
  • Drag scroll bar down using mouse, leaving the window
  • Release mouse button outside of window
  • Move mouse up and down over scroll bar without pressing any buttons

@mol
Copy link
Contributor Author

mol commented Aug 23, 2019

Took me a while to remember the reason I actually abandoned the initial set of changes in #2258 (c843317). So this will actually reintroduce #2060 when the mouse exits the WPF Window (same problem occurs with my attempted fix).

Slightly more detailed steps to reproduce than in #2060 would be:

  • Open WPF Example
  • Open new Tab (google.com should be displayed, this is important as we need to leave the window whilst scrolling)
  • Press the Restore Down menu bar shortcut (top right) or alternatively press windows key + down
  • Drag scroll bar down using mouse, leaving the window
  • Release mouse button outside of window
  • Move mouse up and down over scroll bar without pressing any buttons

You just added this for reference right? I followed the steps and it didn't reproduce the issue for me. Everything looks to be working fine. I also checked that drag/drop to and from works (in Mailbird).

@AppVeyorBot
Copy link

@amaitland
Copy link
Member

You just added this for reference right?

Unfortunately no. I just did a clean checkout of 3566995 and can reliably reproduce the issue.

@amaitland
Copy link
Member

Screen capture that demos the problem, you should be able to visually see when I have the mouse pressed, as it has a circle. After I've released the mouse the browser still thinks the mouse down as it didn't get the correct message, moving the mouse anywhere on the scroll bar triggers a scroll when no mouse is actually pressed. I tried upgrading CefSharp.Wpf.Example to .Net 4.7.2 and that made no difference.

cefsharpmousecapture

@mol
Copy link
Contributor Author

mol commented Aug 25, 2019

Screen capture that demos the problem, you should be able to visually see when I have the mouse pressed, as it has a circle. After I've released the mouse the browser still thinks the mouse down as it didn't get the correct message, moving the mouse anywhere on the scroll bar triggers a scroll when no mouse is actually pressed. I tried upgrading CefSharp.Wpf.Example to .Net 4.7.2 and that made no difference.

cefsharpmousecapture

That's really weird. I'm running the same branch/commit and with no custom changes and it's not happening for me...
CdxVUUr44P
When the mouse is captured all mouse events should route to the browser right, so the only thing I can think of is something else is capturing the mouse for you but not me. Any ideas? Any test code that might be running for you but not me?

@mol
Copy link
Contributor Author

mol commented Aug 25, 2019

What version of Windows are you running? I'm on Windows 10 (1903).

@amaitland
Copy link
Member

When the mouse is captured all mouse events should route to the browser right, so the only thing I can think of is something else is capturing the mouse for you but not me.

I cannot really tell from the screen capture when you released the mouse, so it's really hard to say if you are doing anything different.

Any test code that might be running for you but not me?

No, I did a clean checkout to confirm that it is still a problem. It's the exact reason that I reverted the previous mouse capture code last year.

What version of Windows are you running? I'm on Windows 10 (1903).

I recently upgraded to 1903 (Microsoft finally fixed the bug that was preventing me from upgrading).

I'll test again on a different machine when I can, probably in a week or so.

@mol
Copy link
Contributor Author

mol commented Aug 26, 2019

When the mouse is captured all mouse events should route to the browser right, so the only thing I can think of is something else is capturing the mouse for you but not me.

I cannot really tell from the screen capture when you released the mouse, so it's really hard to say if you are doing anything different.

Any test code that might be running for you but not me?

No, I did a clean checkout to confirm that it is still a problem. It's the exact reason that I reverted the previous mouse capture code last year.

What version of Windows are you running? I'm on Windows 10 (1903).

I recently upgraded to 1903 (Microsoft finally fixed the bug that was preventing me from upgrading).

I'll test again on a different machine when I can, probably in a week or so.

I'm releasing it outside the window so definitely the same as you.

Well we've released it to all of our users and so far no issues. I have a task for myself to update this pull request in a couple of weeks (earlier if I hear of any issues) so we'll see how it goes :)

@mol
Copy link
Contributor Author

mol commented Aug 26, 2019

I just had Dimas from the team test it and he can reproduce it. He's using a button on a track pad, not a mouse - so if you're doing the same then I think that's why it happens to him and you and not me.

@mol
Copy link
Contributor Author

mol commented Aug 26, 2019

Turns out this is related to this issue dotnet/wpf#1323

Adding the workaround mentioned in dotnet/wpf#1323 (comment) to the WPF example fixes the issue. It disables tablet support though and might not be something you want to do in CefSharp. We're already doing it in Mailbird only for the two affected versions.

@amaitland
Copy link
Member

He's using a button on a track pad, not a mouse

I am using a Mouse, though this machine does have a touch screen.

Turns out this is related to this issue dotnet/wpf#1323

The stated versions of 1809 and 1903 don't match with my comments in #2258 I don't remember which machine I would have tested on, perhaps it was this one, could have been a desktop. Either way, well before 1809 was released. If time permits I'll do some more testing.

Adding the workaround mentioned in dotnet/wpf#1323 (comment) to the WPF example fixes the issue.

Just as an FYI you should be able to set Switch.System.Windows.Input.Stylus.DisableStylusAndTouchSupport switch, via https://docs.microsoft.com/en-us/dotnet/api/system.appcontext.setswitch?view=netframework-4.8 to disable touch and stylus support. (also configurable via registry for what it's worth).

It disables tablet support though and might not be something you want to do in CefSharp

I don't think it's the place of a third party framework like CefSharp to disable Stylus and Touch support.

@timaiv
Copy link

timaiv commented Sep 4, 2019

I fixed this issue with capturing half year ago for my project like you (but more custom code existed).
More, you need add browser internal call, when capture lost (LostMouseCapture Event). (mouse button up)
All works fine.

@mol
Copy link
Contributor Author

mol commented Sep 4, 2019

I fixed this issue with capturing half year ago for my project like you (but more custom code existed).
More, you need add browser internal call, when capture lost (LostMouseCapture Event). (mouse button up)
All works fine.

Thanks. What exactly did you add for the LostMouseCapture event? Do you have some code to share? How does it differ from 3566995?

Also, have you tested this on a laptop with a touch screen? The issue with the mouse button not being released is only an issue on laptops with a touch screen as far as I know, and as far as I know is due to the referenced bug in WPF.

@timaiv
Copy link

timaiv commented Sep 4, 2019

So. Firstly, cefsharp had bug with content jumping while resizing. And i fixed it by replacing canvas to grid. I extend grid with my custom Grid class.

internal class eCEFGrid : Grid
{
	private bool isMouseCaptured;

	public event Action<MouseEvent> OnCaptureLostAfterMouseUp;

	public eCEFGrid()
	{
		Mouse.AddLostMouseCaptureHandler(this, LostMouseCapture);
	}

	private void LostMouseCapture(object sender, MouseEventArgs e)
	{
		isMouseCaptured = false;
	}

	protected override void OnMouseDown(MouseButtonEventArgs e)
	{
		CaptureMouse();
		isMouseCaptured = true;
		base.OnMouseDown(e);
	}

	protected override void OnMouseUp(MouseButtonEventArgs e)
	{
		base.OnMouseUp(e);
		Point position = e.GetPosition(this);
		if (isMouseCaptured)
		{
			this.OnCaptureLostAfterMouseUp?.Invoke(new MouseEvent((int)position.X, (int)position.Y, CefEventFlags.None));
		}
		ReleaseMouseCapture();
		isMouseCaptured = false;
	}
}

Now, in subclass to ChromiumWebBrowser i do following:

[TemplatePart(Name = "PART_grid", Type = typeof(eCEFGrid))]
internal abstract class eCefBrowserBase : ChromiumWebBrowser
{
	private eCEFGrid grid;

	public eCefBrowserBase()
	{
		base.MenuHandler = new eCefCustomMenuHandler();
	}

	public override void OnApplyTemplate()
	{
		base.OnApplyTemplate();
		grid = (GetTemplateChild("PART_grid") as eCEFGrid);
		if (grid != null)
		{
			grid.OnCaptureLostAfterMouseUp += Grid_OnCaptureLostAfterMouseUp;
			grid.Unloaded += Grid_Unloaded;
		}
	}

	private void Grid_Unloaded(object sender, RoutedEventArgs e)
	{
		grid.OnCaptureLostAfterMouseUp -= Grid_OnCaptureLostAfterMouseUp;
		grid.Unloaded -= Grid_Unloaded;
	}

	private void Grid_OnCaptureLostAfterMouseUp(MouseEvent e)
	{
		GetBrowser().GetHost().SendMouseClickEvent(e, MouseButtonType.Left, mouseUp: true, 1);
	}
}

Tested on tablet with touch. All works fine everywhere. SendMouseClickEvent i added because i had same issue that mouse not relased.
P.S. This code need be cleaned and add more if else's. For ex. CaptureMouse() can return false.

@mol
Copy link
Contributor Author

mol commented Sep 5, 2019

Thanks a lot @timaiv I'll take a look at that when I have some time. About the "jumping" issue - I think I've had users experience that. Can you tell me how to reproduce it?

@amaitland
Copy link
Member

About the "jumping" issue - I think I've had users experience that. Can you tell me how to reproduce it?

There was an issue resolved in version 75 (#2844)

@timaiv If you can still reproduce this issue you were seeing then please open a new issue (it's unrelated to mouse capture).

For ex. CaptureMouse() can return false.

@mol This is something we should handle, store a local variable and only release the mouse capture if it was successfully obtained.

//We won't get here if e.g. the right mouse button is pressed and released
//while the left is still held, but in that case the left mouse capture seems
//to be released implicitly (even without the left mouse SendMouseClickEvent in leave below)
Mouse.Capture(null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we switch to UIElement.ReleaseMouseCapture(); it has an additional Mouse.Captured == this check.

https://referencesource.microsoft.com/#PresentationCore/Core/CSharp/System/Windows/UIElement.cs,2514

May as well switch to UIElementCaptureMouse() as well for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You already made these changes right? It's a little unclear from the commit with the ChromiumWebBrowserWithMouseCapture but it looks like it.

How are you envisioning this drag fix will make it into the next release now that it's experimental? Should I do something? :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You already made these changes right? It's a little unclear from the commit with the ChromiumWebBrowserWithMouseCapture but it looks like it.

I did. Those changes are only in the cefsharp/75 branch at the moment. ChromiumWebBrowserWithMouseCapture is already marked as Obsolete.

How are you envisioning this drag fix will make it into the next release now that it's experimental?

I'm reluctant to assign a milestone until I get a better understanding of which versions of Windows 10 the fix will be applied to. Leave this for now, will revisit once there's some clarity. Thanks 👍

amaitland added a commit that referenced this pull request Sep 6, 2019
@timaiv
Copy link

timaiv commented Sep 6, 2019

Also, you need really subscribe LostMouseCapture event (and do SendMouseClickEvent in it), because you can lose capture when your button is pressed to notify browser.
Alt+tab with pressed left button or any other window dialog.
http://www.shujaat.net/2010/11/wpf-windows-forceful-release-of-mouse.html


From my experience, wpf automatically forces to lose capture in some cases if you didnt subscribe any special capture event, i got it. But, now i cant reproduce it on win7, maybe it lie.


I wrote that cefsharp had jumping issue. I wrote reason why i used grid.

@timaiv
Copy link

timaiv commented Sep 10, 2019

Finally!
I tried to remove grid, and fix it normally, as i reproduced that mouse still has pressed button on win10 before grid using.
SendMouseClickEvent need be sent before ReleaseMouseCapture(), that why i had problems without my second SendMouseClickEvent.
I did this order implicit it in grid and why it worked.
Experimental ChromiumWebBrowserWithMouseCapture works normal. But you need more logic, that i wrote (LostMouseCapture event).

@amaitland
Copy link
Member

Experimental ChromiumWebBrowserWithMouseCapture works normal. But you need more logic, that i wrote (LostMouseCapture event).

@timaiv You are welcome to submit a PR with improvements, please make sure you include a detailed list of steps to reproduce the problem your changes fix. Thanks

@timaiv
Copy link

timaiv commented Sep 16, 2019

I added next:

protected override void OnLostMouseCapture(MouseEventArgs e)
{
    GetBrowser().GetHost().SendCaptureLostEvent();
    base.OnLostMouseCapture(e);
}

and saw that some bug still exists with alt+tab (my custom scrollbar stopped jumping after mouse returning on host, that is normal, but style was like pressed). I checked that chrome, firefox scrollbars have same behavior (but it also continue jumps on hovers), other apps like VS don't do it. SendMouseClickEvent also in this case doesn't help.
UWP Mail client scrollbar even continue working after alt+tab unless fully button up.

@amaitland
Copy link
Member

Manually merged with additional lost capture call in commit 21c89b1

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

Successfully merging this pull request may close these issues.

WPF - Selecting text continuing below the visible area does not scroll
4 participants