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

[v3] Gutter jiggles sideways when horizontal touch-scrolling #986

Closed
njx opened this issue Nov 19, 2012 · 28 comments
Closed

[v3] Gutter jiggles sideways when horizontal touch-scrolling #986

njx opened this issue Nov 19, 2012 · 28 comments

Comments

@njx
Copy link
Contributor

njx commented Nov 19, 2012

  1. Open v3 demo/theme.html
  2. Paste in a file with long lines (e.g. codemirror.js)
  3. Using the touchpad, scroll horizontally

Result: Gutter jiggles horizontally, probably because it's being scrolled natively and then being reset back to the correct position.

@njx
Copy link
Contributor Author

njx commented Nov 19, 2012

Sorry to file another one of these :( I just noticed it while regressing the previous bug. At least it doesn't involve throw scrolling...

I did some git bisecting, and I think the reason this didn't come up before is that before 5d3a42e, the horizontal jiggling of the gutter only occured during the initial wheel sampling period (similar to the behavior the flicker used to have), and since I was testing vertical scrolling first, I would never notice the horizontal scrolling problem. Now, though, it happens all the time, even after scrolling around for awhile.

@0b10011
Copy link
Contributor

0b10011 commented Nov 19, 2012

This also occurs with middle-click scrolling (both with autoscrolling turned on and off, although it appears to be more noticeable with it turned on).

marijnh added a commit that referenced this issue Nov 20, 2012
(And fix a really poor function name.)

Issue #986
@marijnh
Copy link
Member

marijnh commented Nov 20, 2012

That patch tries to minimize the effect. You'll still see a brief one-pixel shake on layouts where the rounding of the pixels happens to go the wrong way, and some more noticeable glitches on browsers where we get the initial wheel delta ratio wrong (but only for the first few wheel events).

@bfrohs: I do not know what middle-click scrolling is. Is it a mac thing? Does it generate any events?

@0b10011
Copy link
Contributor

0b10011 commented Nov 20, 2012

@marijnh: Windows/Linux (and possibly mac, with a mouse that has a scrollwheel/middle button). There are two middle-click scrolling methods.

The first is achieved by pushing the middle button (usually the scrollwheel) until it clicks (just like any other button) and then drag the mouse in a direction (in this case, to the left and right).

The second is achieved by first turning autoscrolling on in your browser (Chrome doesn't support it on Linux AFAIK, due to middle-click being used for pasting the current selection, but you can test it on Firefox on Linux). Once it's turned on, middle-clicking anywhere on the page (except on a link, which opens a new tab) will display a new cursor, with arrows in the directions that can be scrolled. You can then drag the mouse in any direction to scroll that way.

I believe it is handled the same way as throw scrolling (or very similar, anyway), but I'm not sure. It's a common way of scrolling for ThinkPad owners (who use the Trackpoint to move the cursor).

As a side note, there seems to have been a regression in performance for middle-click scrolling in Firefox on Linux since I last posted in this issue. It's quite noticeable, especially with the first method of scrolling (it only affects horizontal scrolling).

@marijnh
Copy link
Member

marijnh commented Nov 20, 2012

I'm unable to get middle-click-and-drag to work on any platform. But I did find the autoscrolling option in Firefox. I'm not seeing any slowness though, when scrolling through the Less mode demo page (and the gutter flicker fix seems to work when scrolling like that).

@0b10011
Copy link
Contributor

0b10011 commented Nov 20, 2012

It looks like autoscrolling gets fixed after scrolling back and forth a couple times. The middle-click-and-drag method gets really choppy though (to the point that it appears the editor is frozen). I wouldn't worry about it too much, as it only affects horizontal scrolling (esp. on wider documents) for the not-so-common scrolling method of middle-click-and-drag. You may want to test this newest version with throw-scrolling though, in case throw-scrolling is affected the same way.

@njx
Copy link
Contributor Author

njx commented Nov 20, 2012

@marijnh -- unfortunately, that patch doesn't seem to fix the problem for me in either Mac Chrome 23.0.1271.64 or our older Chromium shell in Brackets. The gutter still moves far out of position during horizontal touchpad scrolling.

@marijnh
Copy link
Member

marijnh commented Nov 20, 2012

@njx It seems to fix it partially -- the gutter will now often look stable during scrolls, but 'overshoot' at the end and start of a scroll in Webkit on OS X. Seems the deltas on the events really have no stable correspondence to actual scroll distances.

Such is life. This is, as far as I'm concerned, the lesser of several evils. The various constraints that we're under don't seem to allow a perfect solution.

@njx
Copy link
Contributor Author

njx commented Nov 20, 2012

Hmmm...I think what's happening with the new patch is that I'm sometimes getting the better behavior you're describing (mostly stable, but occasionally overshooting), but I'm also sometimes getting much worse behavior where it's going all over the place throughout the scroll. I'll try to find reproducible steps for the worse behavior.

@njx
Copy link
Contributor Author

njx commented Nov 20, 2012

Also, when I integrate it into Brackets I always get the worse behavior (whereas in current Chrome I sometimes seem to get the "mostly stable" behavior).

@njx
Copy link
Contributor Author

njx commented Nov 20, 2012

Very mysterious. For awhile I was in the state where I was seeing the "mostly stable" behavior you described (even if I reloaded the theme demo page), but immediately after quitting and restarting Chrome, I'm now seeing the highly unstable behavior again (and it doesn't seem to be going away). I've cleared my cache and am still seeing the unstable behavior.

@marijnh
Copy link
Member

marijnh commented Nov 21, 2012

It seems that my patch made the situation worse on several platforms. I've reverted it again -- the unreliability of wheel scrolling information makes this approach unpractical.

Ideally, we'd find some way to make the browser compute the positions for us using CSS, but I haven't been able to come up with one.

@marijnh
Copy link
Member

marijnh commented Nov 21, 2012

To more clearly delineate the issue:

  • We have elements (the gutter numbers and markers) that we want to scroll along with the content vertically but not horizontally.
  • CSS provides no way to base the vertical position of an element on a different parent than the horizontal position.
  • scroll events fire after the actual scrolling was done. Therefore, synchronizing positions in response to a scroll event seems to always result in glitches like the one described in this issue.

Thus, we seem to be in a bind. ACE solves this problem by stopping wheel events and handling them manually. We went down that road before, I think the problems with that (inconsistent scroll speeds across browsers, obviously non-native feel of scrolling) are also unacceptable.

Please help come up with possible hacks or workarounds.

@njx
Copy link
Contributor Author

njx commented Nov 21, 2012

The ideal solution would be to somehow be able to handle just the horizontal scrolling manually, but I can't think of a way to get that to happen. Is there some crazy way to tamper with events so that we can get the horizontal component but poke it to 0 before it gets to the native code? (I would guess not, but...)

The only thing I can think of would be to pop the gutter out of the scroller and handle its scrolling manually, which would lead to its vertical scrolling lagging slightly behind the main code's scrolling. Not great, but might be a better tradeoff than what's happening right now.

Which browsers/platforms had the most problems with manual wheel event handling? We haven't gotten any complaints so far about our original scrolling implementation in Brackets on Mac or Win, but that's only running in Chromium right now (though we'll need to deal with other browsers eventually). My recollection was that the main problem was with Firefox, which seemed to send out very low-resolution wheel events--perhaps we could lobby for that to be improved (and since I think they're doing silent auto-update now, perhaps we don't have to worry as much about downrev versions).

@njx
Copy link
Contributor Author

njx commented Nov 21, 2012

Another (probably dumb) idea: could we use requestAnimationFrame() while a scroll is happening, using it to check the native scroll position at a high rate and update the gutter's horizontal position? Maybe that would let us keep in sync better than scroll events do. It's basically polling, though, so don't know what the performance impact would be. Also, we would still have to know when to start polling, which we wouldn't until after we got the first scroll event, so we would still have an initial jump...but maybe we could get in fast enough that it wouldn't be noticeable.

@njx
Copy link
Contributor Author

njx commented Nov 21, 2012

Hmmm, maybe it's not such a dumb idea. requestAnimationFrame() seems to be supported in all current browser versions except Opera, and in some ways it seems appropriate to what we're trying to do (maintain smoothness during an animation by updating things before the browser renders).

@marijnh
Copy link
Member

marijnh commented Nov 21, 2012

  • Having the gutter lag slightly on vertical scrolling sounds like a non-starter to me. Vertical scrolling is much more common than horizontal, and the effect would look terrible.
  • I don't think polling, with or without requestAnimationFrame would improve the situation -- the scroll event will be pretty much the first thing fired off after a scroll happens, I didn't test this, but it would hugely surprise me if you could get any scripts to run in between, and even if, the effect would probably not be much improved.

My main issue with handling wheel events is that it causes a major usability problem when we get it right -- I found at least five different situations when I tested it, and if someone ends up using a version/platform combination that we didn't test (because it's old, or because it's new), they might end up with a scrolling that goes really slow, way too fast, or even the wrong way. Add to that that the scrolling just doesn't feel very native when you handle it, because the resolution of the wheel events differs from the actual scrolling, at least on FF.

@njx
Copy link
Contributor Author

njx commented Nov 21, 2012

Understood.

I'd like to prototype the requestAnimationFrame() idea just to see if it has any possibility of working, but most likely won't have time till next week due to the upcoming US holiday. It's true that you wouldn't catch the first scroll event, but maybe you could get called in between subsequent scroll events.

(I guess another reason for skepticism is that even if you do get called at a faster rate than scroll events, the native scrolling might not actually update the element's scrollLeft and scrollTop in between. But there's no way to know without trying it.)

@0b10011
Copy link
Contributor

0b10011 commented Nov 26, 2012

This is fixed for me in the most recent version (middle-click scrolling).

@marijnh
Copy link
Member

marijnh commented Nov 26, 2012

Still happens for me.

@marijnh
Copy link
Member

marijnh commented Nov 27, 2012

I think I got it this time. The fix is relatively low-impact & low-risk. Firefox (on which non-native wheel scrolling looks icky) seems to redraw only after the scroll event fired, which prevents this bug from occurring. So the fix is only turned on on non-Opera, non-Firefox browsers, only for horizontal wheel events, and only when a conversion from delta units to pixels is known (either hard coded or measured).

Please test!

@0b10011
Copy link
Contributor

0b10011 commented Nov 27, 2012

No regressions for me with middle-click scrolling 👍

@njx
Copy link
Contributor Author

njx commented Nov 27, 2012

Hey--getting closer :) Just tested it out and it does seem to fix the horizontal scrolling. However, that commit is somehow reintroducing the previous problem of vertical throw scrolling stopping early, in both Safari and Chrome. (It doesn't occur if I roll back to the previous commit.) The behavior is different though: if there's any horizontal component to the scrolling, the scroll stops after a short time, but if the scrolling is purely vertical it works fine.

@njx
Copy link
Contributor Author

njx commented Nov 27, 2012

Ah--it's because you're returning before setting cm.display.currentWheelTarget if you detect any horizontal motion. If I copy the code that sets it into the if (dx ...) block (above the call to setScrollTop()), it seems to fix the problem.

@marijnh
Copy link
Member

marijnh commented Nov 27, 2012

Oops, sorry, I pushed that patch before I realized you submitted a pull request. Does it work for you? It's slightly cleaner (I think).

@njx
Copy link
Contributor Author

njx commented Nov 27, 2012

Yes, that seems to work! I just did some light testing in Chrome, Safari and Brackets, and it seems fine so far. We'll be doing more CM work either later this week or next week so we can bang on it more then, but I'll knock on wood that this is it :) Thanks again for all your work on the scrolling stuff!

@njx njx closed this as completed Nov 27, 2012
@marijnh
Copy link
Member

marijnh commented Nov 27, 2012

Well, that was fun. I can't wait for the next scrollwheel-related bug to pop up!

@njx
Copy link
Contributor Author

njx commented Nov 27, 2012

Shhh, the evil eye will hear you.

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

No branches or pull requests

3 participants