-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Conversation
@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. |
A more detailed description might help: 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. If anyone can think of a better approach, please go ahead. |
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. |
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 But as I just saw the CEF issue is Update 11.03.15: |
IMO we should wait and see if this gets fixed in the next release of CEF. |
Where do we find the CEF? |
@cbaldwinweb It's on Google Code. |
// on Windows, cancel every other scroll event (#10214) | ||
// TODO: remove this hack when we upgrade CEF to a build with this bug fixed: | ||
// https://code.google.com/p/chromiumembedded/issues/detail?id=1481 | ||
if (brackets.platform === "win") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add && !brackets.inBrowser
to this check
@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? |
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). |
@peterflynn Would you be ok with adding this as a preference (maybe also an undocumented, |
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. |
@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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
@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. |
@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. |
See https://bitbucket.org/chromiumembedded/cef/issue/1481 for more information
01695d3
to
4029840
Compare
@peterflynn Commits squashed. |
Closing since this was superseded by #10930, which is now merged into |
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...