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.

Sign up for GitHub

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

Jump to bottom

rulers + lineWrapping = unnecessary scrollbar #4042

Closed
mtaran-google opened this issue Jun 1, 2016 · 9 comments
Closed

rulers + lineWrapping = unnecessary scrollbar #4042

mtaran-google opened this issue Jun 1, 2016 · 9 comments

Comments

@mtaran-google
Copy link
Contributor

Repro:

  1. Enable a ruler at some column position (say 80)
  2. Enable line wrapping
  3. Type in a long line of text (say longer than the ruler)
  4. Make the editor narrower than the ruler position

Expected:

Ruler goes off the side of the editor (becoming unreachable), the text is wrapped to be fully visible in the viewport, and no scrollbar appears.

Actual:

A scrollbar appears, but the text is still wrapped to the width of the viewport. If you scroll right, you see a gap with no text in it.

I got it to happen in demo/rulers.html with the following content:

<!doctype html>

<title>CodeMirror: Ruler Demo</title>
<meta charset="utf-8"/>
<link rel=stylesheet href="../doc/docs.css">

<link rel="stylesheet" href="../lib/codemirror.css">
<script src="../lib/codemirror.js"></script>
<script src="../addon/display/rulers.js"></script>
<style type="text/css">
  .CodeMirror {border-top: 1px solid #888; border-bottom: 1px solid #888;}
</style>
<div id=nav>
  <a href="http://codemirror.net"><h1>CodeMirror</h1><img id=logo src="../doc/logo.png"></a>

  <ul>
    <li><a href="../index.html">Home</a>
    <li><a href="../doc/manual.html">Manual</a>
    <li><a href="https://github.com/codemirror/codemirror">Code</a>
  </ul>
  <ul>
    <li><a class=active href="#">Ruler demo</a>
  </ul>
</div>

<article>
<h2>Ruler Demo</h2>

<script type="text/javascript">
  var nums = "sasssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssss", space = "          ";
  var colors = ["#fcc", "#f5f577", "#cfc", "#aff", "#ccf", "#fcf"];
  var rulers = [], value = "";
  for (var i = 1; i <= 1; i++) {
    rulers.push({color: colors[i], column: i * 80, lineStyle: "dashed"});
    for (var j = 1; j < i; j++) value += space;
    value += nums + "\n";
  }
  var editor = CodeMirror(document.body.lastChild, {
    rulers: rulers,
    value: value + value + value,
    lineWrapping: true,
    lineNumbers: true
});
</script>

<p>Demonstration of
the <a href="../doc/manual.html#addon_rulers">rulers</a> addon, which
displays vertical lines at given column offsets.</p>

</article>
@marijnh marijnh closed this as completed in bb91581 Jun 2, 2016
@marijnh
Copy link
Member

marijnh commented Jun 2, 2016

I think attached patch will help.

@axil
Copy link

axil commented Sep 23, 2017

Hi @marijnh,

After applying this patch the ruler got shifted and now it strikes a letter instead of separating the letters:
image
Before the patch it was like that:
image
I looked into the html and saw that before the patch the .CodeMirror-rulers div was inserted inside the <div style="position: relative; top: 0px;"> tag while now it is inserted right before it. That's why now the ruler is shown 0.4em to the left of the expected position (0.4em is the padding of the .CodeMirror-lines div).

I've checked with two different setups on Chrome 61.0.3163.100 (Official Build) (64-bit) (latest as of today), on Windows 7 x64.

Do you observe the same symptoms? Is it possible to get the ruler back where it was before the patch?

axil pushed a commit to axil/CodeMirror that referenced this issue Sep 23, 2017
rollback the shift introduced when fixing codemirror#4042
5a5d9fa
@axil
Copy link

axil commented Sep 23, 2017

Here's a patch that reverts the shift. Not sure if it still fixes the original issue of the unnecessary scrollbar though.

@marijnh
Copy link
Member

marijnh commented Sep 23, 2017

I'm not seeing the shift in http://codemirror.net/demo/rulers.html. What triggers it?

@axil
Copy link

axil commented Sep 24, 2017

This is notebook==4.2.3:
image
Here div.CodeMirror-ruler is inside '<div style="position:relative; outline:none">' so its position is counted relative to the inside of .CodeMirror-lines i.e. 0.4em from its start.
Now this is notebook==5.1.0:
image
Here div.CoreMirror-ruler is outside of the '<div style="position:relative; outline:none">' so its position is counter relative to the start of the div.CodeMirror-lines without taking its padding into account which results in being 0.4em to the left of the proper position.

@axil
Copy link

axil commented Sep 26, 2017

Concerning the demo page.

The layout structure of the http://codemirror.net/demo/rulers.html is slightly different from jupyter-notebook.
Namely, element responsible for this padding
image
is different.

In jupyter it is

image

and in your demo it is

image

That's why the shift is not visible in the demo page.

@marijnh
Copy link
Member

marijnh commented Sep 26, 2017

The layout structure of the http://codemirror.net/demo/rulers.html is slightly different from jupyter-notebook.

Right, that's why codemirror.css has these comments:

.CodeMirror-lines {
  padding: 4px 0; /* Vertical padding around content */
}
.CodeMirror pre {
  padding: 0 4px; /* Horizontal padding of content */
}

I.e. the fact that horizontal padding should be set in .CodeMirror pre is an assumption the library makes (not just in the ruler addon), and which you should stick to.

@axil
Copy link

axil commented Sep 26, 2017

Emm, by 'you' you mean me as a jupyter developer or me as an end user? I'm afraid I'm the second, not the first :) I was sort of reporting an issue that broke the 'ruler' plugin from working in jupyter-notebook. And I've traced back the problem to the fix of this ticket. What's your suggestion on whom should I address with bugreport?

@marijnh
Copy link
Member

marijnh commented Sep 26, 2017

I guess filing an issue with Jupyter is appropriate then.

@axil axil mentioned this issue Sep 27, 2017
Closed
@jcb91 jcb91 mentioned this issue Sep 30, 2017
Merged
@jcb91 jcb91 mentioned this issue Oct 29, 2017
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@marijnh @axil @mtaran-google