Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

CodeMirror calculates incorrectly the min-height when the font size is not the default one #3115

Closed
TomMalbran opened this issue Mar 13, 2013 · 7 comments
Assignees

Comments

@TomMalbran
Copy link
Contributor

While doing #3068 i noticed that every line not rendered (not in the dom) was calculated always using the default font size (15px) even when adjusting the font size. You can test this in different ways:

  • EditorManager.getCurrentFullEditor()._codeMirror.getLineHandle(0).height when the scroll is not at the top.
  • Get the top from the parent of .CodeMirror-lines with:
$(".CodeMirror-lines",
        EditorManager.getCurrentFullEditor().getRootElement()).parent().position().top);

And the first rendered line with EditorManager.getCurrentFullEditor()._codeMirror.getViewport().from and then divide the top with the lines.

This is causing some the next visible problems:

    1. Open a medium size file like ViewCommandHandlers.
    2. Increase the font size a lot (10 times do the job).
    3. Scroll down really fast using the the scroller thumb to make CodeMirror render a big part of the entire file at once, which would give a bigger height that what it was calculated and makes errors on the scrolling. Smaller font size would cause this problem too, but isn't as visible as with big ones.
    1. Open 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. Check that even the first PageUp doesn't show the cursor inside the visible area and the cursor doesn't stay in the same position doing more PageUp/PageDown.
@njx
Copy link
Contributor

njx commented Mar 18, 2013

@TomMalbran -- could you take a stab at reproducing this in standalone CodeMirror (outside of Brackets)? I think the only thing CM is supposed to require after changing the font size is a refresh(), but we've noticed when looking at other previous issues that various bits of cached line height state don't all seem to get thrown out on a refresh. If you could reproduce this in CM and file it in the main CM repo, that would be very helpful. Thanks!

@ghost ghost assigned TomMalbran Mar 18, 2013
@njx
Copy link
Contributor

njx commented Mar 18, 2013

Also, could you characterize how bad the user-visible aspects of the bug are? If it's just that the cursor doesn't always end up visible after a page down, that's not terrible. But if you can't scroll to see the whole document, for example, that would be pretty bad.

@TomMalbran
Copy link
Contributor Author

I tested in a standalone CodeMirror and understood what is the problem: refresh() doesn't updates the min-height of the container, so things like PageUp from the bottom of the editor, or sometimes scrolling doesn't work as intended and keep modifying the min-height until it can get to the right size and fix everything.

This is what I did:

  1. Went to http://codemirror.net/demo/theme.html.
  2. Copied the whole content of codemirror.js and pasted it in the editor.
  3. Opened the developer tools, selected the .CodeMirror div and checked the font size.
  4. Selected the .CodeMirror-sizer div and checked the min-height.
  5. Right now the min-height / amount of lines is similar to the font size.
  6. Went back to the .CodeMirror div and increased the font size to 25px.
  7. Used the console to refresh the editor with editor.refresh()
  8. Checked in .CodeMirror-sizer div that the min-height didn't changed.
  9. With the cursor at the last line, press PageUp multiple times and check how the min-height changes, until it gets to the start of the document and it has the right height.

@peterflynn
Copy link
Member

It looks like the codemirror/codemirror5#2157 may have fixed this a couple months back. @TomMalbran are you still seeing this? Comments in #7093 would imply you still are..?

@TomMalbran
Copy link
Contributor Author

I haven't checked it in the code, but there is still something wrong. Open a file with over 300 lines and place the scroll at the top, increase the font size a bit and then scroll fast to the end of the file. You should see some issues with the scroll thumb which gets smaller as it reaches the end of the file.

@TomMalbran
Copy link
Contributor Author

It does seem to be fixed. I was testing in Sprint 36 before and not in master. I will test a bit more and close if it is fixed.

@TomMalbran
Copy link
Contributor Author

Cool, closing as fixed. Will submit a PR soon to remove the hack done to avoid this bug.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants