Select All in inline editor behaves differently based on position

[jasonsanjose]

1. Create this project https://gist.github.com/8a5019167cc737543ae0
2. Open an inline editor on <div>
3. With the cursor at col:1, CMD+A to select all
4. Move cursor to col:6 (end of line in inline editor), CMD+A to select all

In step #4, only "div" is selected.

[njx]

This is probably a CodeMirror bug. My guess is that it's related to https://github.com/marijnh/CodeMirror2/issues/453 -- in that case there's a crash, but I think there's probably an underlying issue with the way selections get fixed up around hidden lines that is causing both bugs. I'll spend a little time and see if I can fix it in the core code.

[njx]

I have a potential fix in CodeMirror, but it's risky and I'd rather not go that route right now until we can get Marijn involved. I have an alternative that's much safer--basically, in the Editor code, handle selectAll() ourselves and explicitly set it to the visible range of the inline editor. However, we should wait until I finish the implementation for inline syncing in the next sprint, since that's where the code that tracks the visible range lives. Moving to sprint 6.

[njx]

QRB reviewed

[peterflynn]

Pull #522 fixes this in Brackets via a workaround -- the underlying bug in CodeMirror is still there. NJ and I agreed to leave this issue open until we've filed a bug on CodeMirror too.

Fwiw, there's an existing (closed) CM bug where NJ alluded to this problem: marijnh/CodeMirror2#453.

[peterflynn]

Oh, and a quick analysis of the problem based on my skim of the CM code...

The CM selectAll command just calls setSelection() with a range that spans the entire file, regardless of whether some of those lines might be hidden. CM allows a selection to include hidden lines as long as it doesn't start or end on a hidden line; setSelection() calls a helper, skipHidden(), to move the range's start/endpoints to ensure they lie on non-hidden lines. But when skipHidden() moves the start/endpoint to a new line, it preserves the column position (ch) from the old line. So, when the selectAll endpoint is moved up into the visible range, its column position might not cover the entirety of that last visible line (if the last hidden line was shorter).

It seems like the right thing would be for skipHidden() to change ch to either 0 or the last col of the new line (depending whether the point in question is the selection start vs. the end, and whether it was moved up vs. down). However, there's some funny logic in skipHidden() that I think tries to take into account the previous selection position before setSelection() was called, and I don't understand that bit well enough to know for sure if my suggested change will do the right thing in all cases.

[peterflynn]

Ugh, GitHub automatically closed this because of a keyword in my commit message. Intended it to stay open...

[njx]

Filed underlying CodeMirror bug as https://github.com/marijnh/CodeMirror2/issues/484. Reassigning to @jason-sanjose as FBNC to test that the workaround is working properly.

[njx]

Actually, I just made a slight tweak to this as part of another bug fix (refactoring out a tiny bit of logic), so I'll mark it FBNC again when that is pulled.

[njx]

FBNC to @jason-sanjose

[jasonsanjose]

Confirmed fixed. Closing.