Scroll line up/down - Take 3

[TomMalbran]

Here we go again, but this time it should work. This implements scroll line up/down as mentioned in #2343 and should work with inline editors too.

[redmunds]

This works good in most cases.

I see that you don't do anything when a range is selected and cursor is inside of an inline editor. I think that's OK.

The most problematic case is when the font-size is changed. It seems to work in the first couple hundred lines of a file, so use a really big file (such as brackets/src/thirdparty/CodeMirror2/lib/codemirror.js) and go near the end.

I also tried an HTML file with an inline editor. it works perfectly with inline editor near top and scrolling down. Very nice! But there's a quirk with inline editor near bottom and scrolling up -- after cursor jumps over inline editor, it moves the next few lines (roughly about the height of the inline editor).

[redmunds]

When specifying different shortcuts across platforms, specify whatever you want for platform:mac, but do not specify a platform on the other shortcut key so that it gets picked up by all other platforms (e.g. win, linux, etc.)

[redmunds]

Done with initial review.

[TomMalbran]

@redmunds Thanks for the review!

• I also thing that is better to leave selections untouched when scrolling with this feature.
• I couldn't reproduce any really problem with scrolling on different font-sizes. The only problem I saw was when moving down from a scrollTop = 0, and I addressed that problem.
• it was really hard to make it work with inline editor, specially since I couldn't use any of the cursor/selection CodeMirror APIs. I just fixed the issue you mentioned. I just scrolls 1 extra line now, but I think that is ok.
• Please check my comment about your last review and let me know what you think about it. I'll remove it if you feel it doesn't add much, but I do like how scroll works using that feature.

[TomMalbran]

I think that Editor should have a new public method getPaddingTop that would return editor._getLineSpaceElement().offsetTop, and replaced _getLineSpaceElement. Checking through the code, the only other place where _getLineSpaceElement() is used is inside Editor to check the offsetTop, just like I did. Currently _getLineSpaceElement() is not public because is really specific to code mirror, but replacing it for getPaddingTop would make it less specific and could be changed to any other value if needed, even 0 if the padding would be removed at some point.

[redmunds]

Adding getPaddingTop sounds like a good idea. What about the bottom and sides? Maybe a getPadding method that returns an object with top, right, bottom, and left settings.

[redmunds]

Here's my recipe and results for the different font-size case:

1. Open src/thirdparty/CodeMirror2/lib/codemirror.js
2. Press Cmd/Ctrl-+ twice to increase font-size by 2 increments
3. Cmd-down (or Ctrl-End) to go to end of file
4. PageUp 10 or so times to give you some room to scroll around
5. Place cursor somewhere in middle of editor at left edge of a non-empty line
6. Line scroll down 1 line

Results: works as expected

1. Line scroll up 1 line

Results:
Win7: works as expected
Mac10.8: cursor is gone. Pressing right arrow causes editor to stay on same line (since it's non-empty) and scroll cursor into view, so you can find cursor. For me, it seems to jump up 40 lines or so. Note that my editor on Mac (and Win) has ~45 lines of text, so not sure if that is related.

1. Put cursor back in view and line scroll down until cursor hits top of view and should be re-positioned.

Results:
Both Win7 & Mac10.8: cursor scrolls out of view (i.e. does not get re-positioned) after 5 lines scrolls

1. Put cursor back in view and line scroll down up cursor hits bottom of view and should be re-positioned.

Results:
Win7: cursor scrolls out of view (i.e. does not get re-positioned) after 5 lines scrolls
Mac10.8: this cannot be tested because of problem described above.

Note: I see very similar results if I reduce font-size instead of increasing it.

[TomMalbran]

I can see now what you mean. I could even saw that error with the default font size near almost the end of codemirror.js, but it was better seen with a larger font size, since the lines in view calculation was way off.

It seems that CodeMirror makes every line not rendered have always a height of 15px, so when increasing the font-size, the top and height aren't calculated exactly. If you check the DOM and look for the parent of .CodeMirror-lines, and then grab the top CSS property and divide it by the real text size, the amount of lines isn't right. If you divide that amount with the number of the first rendered line, the result is always a few decimals more than 15 (maybe is off from the padding top or something else).

You can really see this being wrong done by increasing the font a lot (10 times do the job) in a medium size file like ViewCommandHandlers and then scrolling down really fast using the the scroller thumb to make it render almost the entire file at once, which gives the right height, but makes errors on the scrolling.

I also saw that doing Page Up after increasing the font a bit and using Ctrl+Down didn't showed the cursor on the visible lines. Sometimes it get fixed after using Page Down, but it can get broken again doing several fast Page Up, and I think is for the same reason.

So the solution for now was to do the calculations based on the rendered lines, by removing the top position to the scroll top and then adding the first rendered line to the result. But it could be solved later in CodeMirror and go back to the previews commit.

On getPadding, the only really useful one is the top one, although Brackets adds all 4 in the LESS files, which might make it harder to get all of them from the DOM. But this seems to be now a discussion for another issue.

[redmunds]

On Mac 10.8, This looks great in the font-size increased case -- the page scrolls and cursor stays in view perfectly. The font-size decreased case also looks great on line scroll down. But, when I decrease the font-size and try to line scroll up the page doesn't scroll at all -- it doesn't see to matter where the cursor is or if anything is selected.

On Windows 7, the cursor is never moved to keep it in the view (i.e. always stays on same line) even with default font-size.

[TomMalbran]

The line snapping was off by one line and paddingTop only fixed it for higher text sizes.

I couldn't reproduce the Win7 bug (maybe because it doesn't happen in WinXP?), but I found and fixed a similar bug that didn't set the cursor into view when it was above the visible area and moving up or was below the visible area and moving down.

[redmunds]

The Windows 7 problem was a user error -- I forgot to uninstall my line scroll extension before testing your line scroll code :)

This looks great. Merging.

[TomMalbran]

Great. Thanks.

Should I add an issue for the scrolling, line height rendering problem?

[redmunds]

It would be good to log somewhere. I also notice that the cursor doesn't stay in the same position when you use PageUp/Down with non-default font-size.

[TomMalbran]

Will do, and one for getPadding/getPaddingTop too.