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

Fix the CEF scroll bug on Windows #10681

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@MarcelGerber
Contributor

MarcelGerber commented Mar 3, 2015

This fix is a little hacky and it will have weird effects for those using thirdparty software like X-Mouse or KatMouse (as suggested in #10214), but I confirmed it fixes #10214 and it isn't too wonky.
Obviously, a CEF-side fix would be way better, but apparently the issue doesn't have much priority...

@nethip

This comment has been minimized.

Show comment
Hide comment
@nethip

nethip Mar 5, 2015

Contributor

@MarcelGerber Is this bringing back the old behavior. I recall @JeffryBooher mentioning about no of lines scrolled to be 11 against 7. Can you share this data?. And also you mentioned some unwanted behavior might be introduced when using third party software. What are those? If the effects are weird and cause problems in using other applications, can we put this under a preference? Does that make sense.

Contributor

nethip commented Mar 5, 2015

@MarcelGerber Is this bringing back the old behavior. I recall @JeffryBooher mentioning about no of lines scrolled to be 11 against 7. Can you share this data?. And also you mentioned some unwanted behavior might be introduced when using third party software. What are those? If the effects are weird and cause problems in using other applications, can we put this under a preference? Does that make sense.

@MarcelGerber

This comment has been minimized.

Show comment
Hide comment
@MarcelGerber

MarcelGerber Mar 5, 2015

Contributor

A more detailed description might help:
In the CEF version we use, there are 2 mousewheel events fired per wheel tick, at least on Windows, while Chrome/Chromium and every other browser only trigger one (which is, obviously, the expected behaviour).
A normal Chrome scroll (triggered by a mouse wheel, not others like touchpad) scrolls 200px, so this bug causes a scroll to span 400px.
The two events both look like "real" events with a different timestamp and both with proper coordinates etc., so it's not easy to decide whether a scroll event is "real" or caused by the bug. The only things they have in common is the scroll distance (deltaX, deltaY) and usually the mouse position (clientX, clientY). The timestamps are not too far apart, but there can well be ~100ms between them (because a render happens in between).
This bug does not only affect the editor (= CodeMirror), but the whole Brackets UI, including dialogs, the sidebar, extension manager, ...

So, I guess @JeffryBooher didn't measure exact values as the scroll distance should have doubled in Release 1.1, compared to Release 1.0. And apparently, the issue is much worse for other users, including me where it currently scrolls 24 lines.

AFAICT, the thirdparty software mentioned above was installed to fix this very problem. I don't know of any other software that hooks the mouse, so with users that didn't explicitly install one of the aforementioned tools, there shouldn't be any issue either.
This fix doesn't cause problems with other software, but for users that use one of those tools, only every second wheel event (= wheel tick) will get through to Brackets (which is bad, I know).

If anyone can think of a better approach, please go ahead.
One could be to either include the patch posted on the CEF issue in our CEF or enable the "--off-screen-rendering-enabled" flag in CEF, as that apparently fixes the issue.

cc @JeffryBooher

Contributor

MarcelGerber commented Mar 5, 2015

A more detailed description might help:
In the CEF version we use, there are 2 mousewheel events fired per wheel tick, at least on Windows, while Chrome/Chromium and every other browser only trigger one (which is, obviously, the expected behaviour).
A normal Chrome scroll (triggered by a mouse wheel, not others like touchpad) scrolls 200px, so this bug causes a scroll to span 400px.
The two events both look like "real" events with a different timestamp and both with proper coordinates etc., so it's not easy to decide whether a scroll event is "real" or caused by the bug. The only things they have in common is the scroll distance (deltaX, deltaY) and usually the mouse position (clientX, clientY). The timestamps are not too far apart, but there can well be ~100ms between them (because a render happens in between).
This bug does not only affect the editor (= CodeMirror), but the whole Brackets UI, including dialogs, the sidebar, extension manager, ...

So, I guess @JeffryBooher didn't measure exact values as the scroll distance should have doubled in Release 1.1, compared to Release 1.0. And apparently, the issue is much worse for other users, including me where it currently scrolls 24 lines.

AFAICT, the thirdparty software mentioned above was installed to fix this very problem. I don't know of any other software that hooks the mouse, so with users that didn't explicitly install one of the aforementioned tools, there shouldn't be any issue either.
This fix doesn't cause problems with other software, but for users that use one of those tools, only every second wheel event (= wheel tick) will get through to Brackets (which is bad, I know).

If anyone can think of a better approach, please go ahead.
One could be to either include the patch posted on the CEF issue in our CEF or enable the "--off-screen-rendering-enabled" flag in CEF, as that apparently fixes the issue.

cc @JeffryBooher

@JeffryBooher

This comment has been minimized.

Show comment
Hide comment
@JeffryBooher

JeffryBooher Mar 5, 2015

Contributor

Just to be clear: Everyone I talked to that experienced this issue had a different result; for some it was 24 lines of code scrolling but for me the delta wasn't as severe but it was definitely noticeable.

The delta could have been differences between OS versions, System Settings, Hardware and even system timing.

Enabling Off-Screen rendering is definitely worth a shot but it may introduce other issues that may complicate the issue. It will definitely require quite a bit of testing.

I think if everyone upvotes the CEF issue then Marshall will fix the problem for everyone.

Contributor

JeffryBooher commented Mar 5, 2015

Just to be clear: Everyone I talked to that experienced this issue had a different result; for some it was 24 lines of code scrolling but for me the delta wasn't as severe but it was definitely noticeable.

The delta could have been differences between OS versions, System Settings, Hardware and even system timing.

Enabling Off-Screen rendering is definitely worth a shot but it may introduce other issues that may complicate the issue. It will definitely require quite a bit of testing.

I think if everyone upvotes the CEF issue then Marshall will fix the problem for everyone.

@MarcelGerber

This comment has been minimized.

Show comment
Hide comment
@MarcelGerber

MarcelGerber Mar 5, 2015

Contributor

Reading through https://code.google.com/p/chromiumembedded/wiki/GeneralUsage#Off-Screen_Rendering, I no longer think enabling offscreen rendering is a good idea ("Off-screen rendering does not currently support accelerated compositing so performance may suffer as compared to a windowed browser").

I think the differences in impact are not caused by OS differences (it's a Win-only bug, and I don't see why that should make a difference in Chrome's lazy scroll implementation), but rather on differing font sizes & font (would probably be better to give distance in px; for me, it's 400px; one way to get this is to scroll all the way up, scroll one tick down and then issue $(".CodeMirror-scroll").scrollTop()) and probably also on mouse drivers (drivers/software that implement smooth scrolling, for example).
Also, when line wrapping is enabled, you can't just simply calculate the difference of line numbers, you have to instead take wrapped lines into account, too.

But as I just saw the CEF issue is the fifth-most starred CEF issue, I hope this issue will be fixed soon.

Update 11.03.15:
The issue is actually the third-most starred CEF issue

Contributor

MarcelGerber commented Mar 5, 2015

Reading through https://code.google.com/p/chromiumembedded/wiki/GeneralUsage#Off-Screen_Rendering, I no longer think enabling offscreen rendering is a good idea ("Off-screen rendering does not currently support accelerated compositing so performance may suffer as compared to a windowed browser").

I think the differences in impact are not caused by OS differences (it's a Win-only bug, and I don't see why that should make a difference in Chrome's lazy scroll implementation), but rather on differing font sizes & font (would probably be better to give distance in px; for me, it's 400px; one way to get this is to scroll all the way up, scroll one tick down and then issue $(".CodeMirror-scroll").scrollTop()) and probably also on mouse drivers (drivers/software that implement smooth scrolling, for example).
Also, when line wrapping is enabled, you can't just simply calculate the difference of line numbers, you have to instead take wrapped lines into account, too.

But as I just saw the CEF issue is the fifth-most starred CEF issue, I hope this issue will be fixed soon.

Update 11.03.15:
The issue is actually the third-most starred CEF issue

@nethip

This comment has been minimized.

Show comment
Hide comment
@nethip

nethip Mar 7, 2015

Contributor

IMO we should wait and see if this gets fixed in the next release of CEF.

Contributor

nethip commented Mar 7, 2015

IMO we should wait and see if this gets fixed in the next release of CEF.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Mar 10, 2015

Where do we find the CEF?

ghost commented Mar 10, 2015

Where do we find the CEF?

@MarcelGerber

This comment has been minimized.

Show comment
Hide comment
@MarcelGerber

MarcelGerber Mar 11, 2015

Contributor

@cbaldwinweb It's on Google Code.
You can find builds on http://cefbuilds.com/ (we currently use Branch 2171 on Windows/Mac).
Also, it'd be great if you could star the corresponding CEF issue.

Contributor

MarcelGerber commented Mar 11, 2015

@cbaldwinweb It's on Google Code.
You can find builds on http://cefbuilds.com/ (we currently use Branch 2171 on Windows/Mac).
Also, it'd be great if you could star the corresponding CEF issue.

Show outdated Hide outdated src/brackets.js
Show outdated Hide outdated src/brackets.js
@peterflynn

This comment has been minimized.

Show comment
Hide comment
@peterflynn

peterflynn Apr 15, 2015

Member

@MarcelGerber I tested this out on my Win 8 machine and it works great. I'm in favor of merging this for 1.3 since many people have described it as a blocker to upgrading past 1.0, and it looks like it will be a while before we could get a CEF fix (the CEF bug seems stalled now, and upgrading CEFs takes a lot of time). Whenever we do pick up a CEF with a fix this will be easy to back out, so I like this as a stopgap workaround until then.

@nethip any concerns about that plan?

Member

peterflynn commented Apr 15, 2015

@MarcelGerber I tested this out on my Win 8 machine and it works great. I'm in favor of merging this for 1.3 since many people have described it as a blocker to upgrading past 1.0, and it looks like it will be a while before we could get a CEF fix (the CEF bug seems stalled now, and upgrading CEFs takes a lot of time). Whenever we do pick up a CEF with a fix this will be easy to back out, so I like this as a stopgap workaround until then.

@nethip any concerns about that plan?

@peterflynn peterflynn added this to the Release 1.3 milestone Apr 15, 2015

@MarcelGerber

This comment has been minimized.

Show comment
Hide comment
@MarcelGerber

MarcelGerber Apr 15, 2015

Contributor

I've tested this for some time a while ago and it all seemed to work well, but at some point I wondered why it it suppressed the first and sometimes the second event, and even though that didn't cause any issue, I wonder if that had to do with a bigger issue (like, CEF not always firing doubled scroll events).

Also, I assume you've read the wall(s) of text above, correct? There's some risk of CEF not firing doubled scroll events in all scenarios (possibly depending on mouse drivers, software like KatMouse, Compatibility mode, ...). Obviously, we haven't heard of the one's where scrolling just works (so we don't know how many they are) and if, with this fix merged, only every other wheel tick takes effect, the UX would greatly decrease for them.

I don't quite get though why Marshall, the CEF maintainer, almost completely ignores this issue, which is currently the second-most upvoted issue on the board. Yeah, it's propably some work to figure out what's wrong, but there's plenty of interest in fixing this bug, too (this exact same bug has also caught me in the Windows Spotify client, and it's annoying there too).

Contributor

MarcelGerber commented Apr 15, 2015

I've tested this for some time a while ago and it all seemed to work well, but at some point I wondered why it it suppressed the first and sometimes the second event, and even though that didn't cause any issue, I wonder if that had to do with a bigger issue (like, CEF not always firing doubled scroll events).

Also, I assume you've read the wall(s) of text above, correct? There's some risk of CEF not firing doubled scroll events in all scenarios (possibly depending on mouse drivers, software like KatMouse, Compatibility mode, ...). Obviously, we haven't heard of the one's where scrolling just works (so we don't know how many they are) and if, with this fix merged, only every other wheel tick takes effect, the UX would greatly decrease for them.

I don't quite get though why Marshall, the CEF maintainer, almost completely ignores this issue, which is currently the second-most upvoted issue on the board. Yeah, it's propably some work to figure out what's wrong, but there's plenty of interest in fixing this bug, too (this exact same bug has also caught me in the Windows Spotify client, and it's annoying there too).

@MarcelGerber

This comment has been minimized.

Show comment
Hide comment
@MarcelGerber

MarcelGerber Apr 15, 2015

Contributor

@peterflynn Would you be ok with adding this as a preference (maybe also an undocumented, _-prefixed one, like _winScrollFix) defaulting to true, just in case a scenario like the above should happen?

Contributor

MarcelGerber commented Apr 15, 2015

@peterflynn Would you be ok with adding this as a preference (maybe also an undocumented, _-prefixed one, like _winScrollFix) defaulting to true, just in case a scenario like the above should happen?

@MarcelGerber

This comment has been minimized.

Show comment
Hide comment
@MarcelGerber

MarcelGerber Apr 15, 2015

Contributor

I've just added another commit so this is now preference-based (it even supports on-the-fly changes), so it's now ready to land from my side.
Please let me know if/when you'd like to merge so I can squash commits. I may though not be able to do that tomorrow, but no later than Friday.

Contributor

MarcelGerber commented Apr 15, 2015

I've just added another commit so this is now preference-based (it even supports on-the-fly changes), so it's now ready to land from my side.
Please let me know if/when you'd like to merge so I can squash commits. I may though not be able to do that tomorrow, but no later than Friday.

@peterflynn

This comment has been minimized.

Show comment
Hide comment
@peterflynn

peterflynn Apr 15, 2015

Member

@MarcelGerber It seems unnecessary... I'm not aware of any Windows machines where the bug doesn't reproduce. Have you seen any reports of that?

If someone has applied a KatMouse patch specific to Brackets to halve the scroll rate in order to work around this bug, there's no reason they'd need to leave that patch in place now that Brackets is 'fixed.' It doesn't seem like this fix should interfere with any other use of those mouse hack tools.

I'm not saying we should give up on fixing the CEF bug. But realistically even if it was fixed today it would probably be 2+ months before we could get that fix into the hands of users. That's why I think we should merge this as a stopgap.

Member

peterflynn commented Apr 15, 2015

@MarcelGerber It seems unnecessary... I'm not aware of any Windows machines where the bug doesn't reproduce. Have you seen any reports of that?

If someone has applied a KatMouse patch specific to Brackets to halve the scroll rate in order to work around this bug, there's no reason they'd need to leave that patch in place now that Brackets is 'fixed.' It doesn't seem like this fix should interfere with any other use of those mouse hack tools.

I'm not saying we should give up on fixing the CEF bug. But realistically even if it was fixed today it would probably be 2+ months before we could get that fix into the hands of users. That's why I think we should merge this as a stopgap.

// on Windows, cancel every other scroll event (#10214)
// TODO: remove this hack when we upgrade CEF to a build with this bug fixed:
// https://bitbucket.org/chromiumembedded/cef/issue/1481
var winCancelWheelEvent = true;

This comment has been minimized.

@peterflynn

peterflynn Apr 15, 2015

Member

I wonder if it would make it feel more responsive to start with this 'false'? That way the first scroll in the pair is actually rendered, the second is ignored. It didn't feel at all laggy in my testing, but it might be worth trying out...

@peterflynn

peterflynn Apr 15, 2015

Member

I wonder if it would make it feel more responsive to start with this 'false'? That way the first scroll in the pair is actually rendered, the second is ignored. It didn't feel at all laggy in my testing, but it might be worth trying out...

This comment has been minimized.

@MarcelGerber

MarcelGerber Apr 16, 2015

Contributor

It's actually already like this - see the function call, where winCancelWheelEvent is first negated (so in the first call, it's now set to false) and then, we check whether it's truthy.
I don't know why I did it this way round (yeah, the other would probably make more sense), though.

@MarcelGerber

MarcelGerber Apr 16, 2015

Contributor

It's actually already like this - see the function call, where winCancelWheelEvent is first negated (so in the first call, it's now set to false) and then, we check whether it's truthy.
I don't know why I did it this way round (yeah, the other would probably make more sense), though.

@peterflynn

This comment has been minimized.

Show comment
Hide comment
@peterflynn

peterflynn Apr 15, 2015

Member

This pref change seems fair though. I'm ok with merging this -- @nethip lmk if you'd have any concerns about this going into 1.3.

Member

peterflynn commented Apr 15, 2015

This pref change seems fair though. I'm ok with merging this -- @nethip lmk if you'd have any concerns about this going into 1.3.

@nethip

This comment has been minimized.

Show comment
Hide comment
@nethip

nethip Apr 16, 2015

Contributor

@peterflynn No issues. I was waiting for this to be fixed from CEF side, but looks like that is going to take some time. Meanwhile we should push in this fix.

Contributor

nethip commented Apr 16, 2015

@peterflynn No issues. I was waiting for this to be fixed from CEF side, but looks like that is going to take some time. Meanwhile we should push in this fix.

@peterflynn

This comment has been minimized.

Show comment
Hide comment
@peterflynn

peterflynn Apr 16, 2015

Member

@MarcelGerber Did you want to squash before this is merged? If you have time on Thursday that would be great. Otherwise I can just go ahead and merge as-is.

Member

peterflynn commented Apr 16, 2015

@MarcelGerber Did you want to squash before this is merged? If you have time on Thursday that would be great. Otherwise I can just go ahead and merge as-is.

@MarcelGerber

This comment has been minimized.

Show comment
Hide comment
@MarcelGerber

MarcelGerber Apr 16, 2015

Contributor

@peterflynn Commits squashed.

Contributor

MarcelGerber commented Apr 16, 2015

@peterflynn Commits squashed.

@peterflynn

This comment has been minimized.

Show comment
Hide comment
@peterflynn

peterflynn Apr 17, 2015

Member

Closing since this was superseded by #10930, which is now merged into release

Member

peterflynn commented Apr 17, 2015

Closing since this was superseded by #10930, which is now merged into release

@peterflynn peterflynn closed this Apr 17, 2015

@MarcelGerber MarcelGerber deleted the MarcelGerber:win-cef-scroll-fix branch Apr 17, 2015

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