Find Next doesn't scroll to the right (but Find Previous does)

[peterflynn]

1. Resize the window so some lines are cut off on the right side
2. Search for text that has an occurrence that's off the right side of the viewport
3. Find Next until you get to that match
4. Find Next again to go to the following match
5. Find Previous

Result:
3 - View doesn't scroll to the right. It looks like the 'current result' highlight has disappeared.
5 - View does scroll to the right.

The view also scrolls correctly if you place the cursor so that the first match is the one that's far to the right -- the incremental search as you type scrolls to the right as expected. It's just Find Next that doesn't.

[pthiess]

Reviewed - @RaymondLim should scroll.

[TomMalbran]

I wasn't able to reproduce this bug exactly like this in the latest version. What I could reproduce is when the last match is scrolled to the right, and the first isn't, so when you reach the last match and the editor is scrolled to display it and then use Find Next to go to the first match, then the editor doesn't scroll back to the left on the first match but it does on the second. The same thing happens using Find Previous from the first match.

[JeffryBooher]

This looks fixed with the latest codemirror. FBNC @peterflynn

[JeffryBooher]

Disregard. still broken -- I tried resizing in between F3 key presses and it failed when I did that. Maybe it had to do with resizing while searching or maybe I just didn't resize small enough.

[redmunds]

There's a related problem with horizontal scrolling (which I thought I filed, but I can't seem to find). After scrolling to the right, view will scroll back to the left, but only enough to see last char of match!. Entire match should be scrolled into view. Verified this is still broken in Sprint 30.

[lkcampbell]

OS: Windows 7

Build: sprint 32 development build 0.32.0-0 (master d40c64b)

Now it doesn't scroll at all regardless of whether you use Find Next or Find Previous. Not even enough to see the last character of the match.

It looks like the current behavior is now closer to issue #5064 than the behavior described in this issue.

[lkcampbell]

Okay, I am getting two different kinds of behavior with two different test files. One behavior is what is described by this issue and the second behavior is no scrolling at all.

I have to do some more research to figure out what is going on here.

[lkcampbell]

According to documentation comment on Editor.centerOnCursor():

This does not alter the horizontal scroll position.

If I create a text file with a single line that runs off the page and search on a common word, the editor never scrolls horizontally at all. I'm not even sure of the origin of that odd, "show one character" horizontal scroll behavior. Maybe it is some CodeMirror or CEF side effect of vertically scrolling multiple lines and text selection? It shouldn't be horizontally scrolling at all AFAIK.

At any rate, the current solution appears to be: Change the horizontal scroll position on Editor.centerOnCursor() to show the whole selection (or at least the first characters of the selection instead of the last). Unless someone knows of a specific reason why this behavior has been avoided in the past, that is the approach I am going to take.

Assigning to myself to work on a solution.

[njx]

Probably worth looking at all the callers of centerOnCursor() to make sure that it's okay to just change the behavior rather than adding a parameter to request the new behavior. /cc @dangoor who I think might have written this originally.

[redmunds]

@lkcampbell Without the "centering" code, the selection should still entirely be scrolled into view (although it could be against top or bottom and/or left or right). So, we should make sure that works first.

Then the vertically centering functionality should not break it. I don't think we should also try to horizontally center selection -- I would like to see "scroll all the way to the left unless you can't see selection" functionality. I think it's annoying if editor scroll to the right to show a hit, but does not scroll back to the left if next hit is viewable with previous horizontal scrolling.

[lkcampbell]

@redmunds, good point. It's not really a horizontal centering. I will focus on implementing the "scroll horizontally into view" functionality outside of the centerOnCursor() function.

[lkcampbell]

The problem with the left scrolling only showing the last letter of the highlighted word is reproducible in a CodeMirror demo, so I filed a CodeMirror issue for that: codemirror/codemirror5#1870

The other part of this issue is the right scrolling not working. I have a fix for this problem but I want to see what happens with the CodeMirror issue first before submitting the second part of the fix.

[lkcampbell]

The CodeMirror issue has been patched so we just need to update to the latest CodeMirror to fix the "left scrolling only showing the last letter of the highlighted word" issue.

I will submit a pull request for the right scrolling problem after the CodeMirror update.

[redmunds]

@lkcampbell CodeMirror was updated recently, so we may already have that fix.

[lkcampbell]

@redmunds, no, not yet. The fix was just submitted today. It's possible I'll need to make a few tweaks to Brackets find code as well since the API is slightly different now.

I assume you guys are going to update the CodeMirror repo again in the near future, I can just finish up this fix after the update happens.

[redmunds]

We try to update CodeMirror at the beginning of every Sprint. Is that soon enough?

[lkcampbell]

That's fine with me. I will just wait until next Sprint to fix this bug.

[lkcampbell]

@redmunds, any idea when the next CodeMirror update is going in?

[redmunds]

CodeMirror was last updated 4 days ago: 9a92eeb

[lkcampbell]

Guess I could have submitted this fix four days ago :). Anyway, here's the fix using the new CodeMirror fix.

[redmunds]

FBNC back to @peterflynn

[redmunds]

Closing.