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

Cursor movement inconsistent around collapsed inclusiveLeft/Right #3658

Closed
vincentwoo opened this issue Nov 20, 2015 · 10 comments
Closed

Cursor movement inconsistent around collapsed inclusiveLeft/Right #3658

vincentwoo opened this issue Nov 20, 2015 · 10 comments

Comments

@vincentwoo
Copy link
Contributor

Description

When using collapsed ranges with inclusiveLeft, CodeMirror has trouble moving the cursor around the ranges, but seems to handle inclusiveRight naturally enough.

Reproduction

  1. Go to https://codemirror.net

  2. Open a console and run

    editor.setValue("1\n2\n3\n4\n5");
    for (var i = 0; i < 5; i++) {
      editor.markText(
        { line: i, ch: 0 },
        { line: i, ch: 1 },
        { inclusiveLeft: true, collapsed: true }
      );  
    }
  3. Place your cursor anywhere in the document and hit up or down arrow keys.

  4. The cursor will move one line downward on down, but two lines upwards on up.

  5. Now try again with inclusiveRight:

    editor.setValue("1\n2\n3\n4\n5");
    for (var i = 0; i < 5; i++) {
      editor.markText(
        { line: i, ch: 0 },
        { line: i, ch: 1 },
        { inclusiveRight: true, collapsed: true }
      );  
    }
  6. Cursor movement should work as expected, one line per press

Related

Problem seems very similar to the one mentioned in a previous issue I opened, #2635, and is probably the root cause of FirebaseExtended/firepad#82.

Firepad uses collapsed line sentinels at the start of every line to mark metadata, but struggles with blank lines where the only character on a line is a collapsed range with inclusiveLeft.

@marijnh
Copy link
Member

marijnh commented Nov 21, 2015

This may not be consistent in terms of symmetry between these cases, but it does seem to be consistent in terms of actual behavior. Since you're setting inclusiveLeft, the position at the start of each line is inside of a collapsed range, and thus not a valid cursor position. This means that, when moving the cursor, selections that land on such a position have to be corrected. To avoid the cursor getting stuck, this correction is biased to keep moving it in the direction of the cursor motion. In this case, that means it'll be moved somewhere before the range, which takes it to the previous line.

Why are you including your sentinels in the actual text at all? Wouldn't it make more sense to keep them out of the document, and track them in your own data structure?

@vincentwoo
Copy link
Contributor Author

I'd agree that the approach you describe is probably better, but I'm mostly trying to maintain an extant library. It'd probably be very difficult to rewrite this bit at the moment.

Firepad is trying to have invisible sentinels at the start of each line. Using inclusiveLeft with a collapsed range seemed like the blessed way to do that in CodeMirror. Conceptually, should there be any difference between such an empty line and a line with nothing but such a collapsed range on it? To my mind, it DOES seem like both of those are the same thing. You should be able to place your cursor to the right of a collapsed range with inclusiveLeft, no?

@marijnh
Copy link
Member

marijnh commented Nov 23, 2015

Could you say more about these sentinels? Why aren't they implicit in the fact that the line starts there?

@vincentwoo
Copy link
Contributor Author

Well, in Firepad, these sentinels store additional rich-text formatting information. I don't quite understand the entire system back to front, but basically Firepad is using an OT system wherein metadata like that has to be part of the document in order to be versioned appropriately by OT. You can read part of the implementation that deals with sentinels here: https://github.com/firebase/firepad/blob/master/lib/rich-text-codemirror.js.

I'm mostly trying to follow along and diagnose cursor movement bugs in this part of the code. To me it seems that it is not entirely clear what the difference between inclusiveRight and inclusiveLeft are in a collapsed, atomic range. If a range has 0 width, can you not place your cursor before OR after it? What does before vs after mean when it comes to an element with no width?

@marijnh
Copy link
Member

marijnh commented Nov 24, 2015

If you have a line ABCD and you wrap BC in a collapsed range then...

  • If inclusiveLeft and inclusiveRight are both on, the collapsed part includes both cursor position 2 and 4 (and obviously 3), so there is no valid cursor position left between A and D.
  • In both are false, the collapsed part's sides are both valid cursor positions, so you have to press the left/right arrow twice to move across the position between A and D. This is what I guess you are trying to avoid with your use of inclusiveLeft/Right here.
  • If only inclusiveLeft is on, then the range covers cursor position 2, but not 4, so if you put the cursor between A and D its line offset is 4.
  • Similarly, if only inclusiveRight is on, the only valid cursor position between A and D has offset 2.

Does that help?

marijnh added a commit that referenced this issue Nov 24, 2015
The code is now clearer, and will prefer the nearer end of the atomic
range as long as that means there is still progress (i.e. the cursor
does not get stuck).

Issue #3658
@marijnh
Copy link
Member

marijnh commented Nov 24, 2015

Looking through the (terribly dense) code for this, I realized that it was actually workable to change the behavior so that, when coming from one side into an atomic range, if there is a valid position that is at the side that we are coming from, but is not equal to our old position, we choose that. I've implemented that in attached patch. This should address your issue, I believe.

@marijnh marijnh closed this as completed Nov 24, 2015
@vincentwoo
Copy link
Contributor Author

@marijnh Thank you very much again for the patch, it almost addresses all of our issues (I think). Here's a nagging case where it doesn't quite work:

  1. Get a blank editor on HEAD CodeMirror.

  2. Run

    editor.setValue("1foo\n2");
    for (var i = 0; i < 2; i++) {
      editor.markText(
        { line: i, ch: 0 },
        { line: i, ch: 1 },
        { inclusiveLeft: true, collapsed: true }
      );  
    }
  3. Place cursor before the f in foo on the first line

  4. Hit down

At this point you would expect the cursor to move to the second line, but instead moves to the end of the first (after the o in foo).

I dug into your refactor and I suspect (through my admittedly limited understanding) that the issue is related to this line: https://github.com/codemirror/CodeMirror/blob/710f43a1c55ffa4102ef4c3fce46329ad5643da7/lib/codemirror.js#L2253.

The way I read that line seems to say to me "If we are moving rightwards, and approach a span with inclusiveLeft set, move the cursor to the nearest eligible direction in the opposite direction". However, I think it should be in the current direction, no?

My logging seems to indicate that skipAtomic is called with a pos of the first character of the second line and an oldPos of the second character of the first line, and it hits the collapsed range at the start of the second line. At this point it decides to move the cursor backwards one, to get it out of the range. I think, based on your earlier explanation, that the correct behavior is to move it past the range.

marijnh added a commit that referenced this issue Nov 25, 2015
Which is kind of arbitrary but seems to give less surprising
results.

Issue #3658
@marijnh
Copy link
Member

marijnh commented Nov 25, 2015

Hm. That is, again, the correct outcome of the algorithm, but indeed surprising. So I've changed the algorithm again, and now it only uses the 'near' side of an atomic marker when it is on the same line as the origin goal pos. That's somewhat arbitrary, but fixes this specific case.

@vincentwoo
Copy link
Contributor Author

Thank you very much for the followup patch. It seems to address our issues, which I very much appreciate.

@MiguelCastillo
Copy link

We are in the process of updating Brackets to codemirror 5.11 and found that this change affected some of our unit tests. The issues are around the specific check near.line == pos.line in this commit 1fae537

Here is a lil history if you folks wanna take a quick peek.
adobe/brackets#12177

To see the issue:

1 go to https://codemirror.net/index.html
2 open the browser console. We are going to execute the following commands

visibleRange1 = editor.markText({line: 0, ch: 0}, {line: 4, ch: 0}, {collapsed: true, inclusiveLeft: true, inclusiveRight: true, clearWhenEmpty: false});
visibleRange2 = editor.markText({line: 5, ch: 0}, {line: 8, ch: 0}, {collapsed: true, inclusiveLeft: true, inclusiveRight: true, clearWhenEmpty: false});

3 Select anything spanning multiple lines. I expect (and the unit tests as well) that all three of the following selections would generate the same result... But the first selection has unexpected results.

// This line shows the issue
editor.setSelection({line: 4, ch: 0}, {line: 5, ch: 1});

// And should it be the same behavior as?
editor.setSelection({line: 4, ch: 0}, {line: 5, ch: 0});

// And should it be the same behavior as?
editor.setSelection({line: 4, ch: 0}, {line: 5, ch: 2});

// Or even?
editor.setSelection({line: 0, ch: 0}, {line: 8, ch: 0});

4 The selection issue can be revealed by clearing the ranges after trying each selection in the previous step.

visibleRange1.clear();
visibleRange2.clear()

5 Checkout the selected range.

editor.doc.sel

Please let me know if I can help clarify follow the steps or understand more of the issue.

Thanks a million!!

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