adobe / brackets

An open source code editor for the web, written in JavaScript, HTML and CSS.
http://brackets.io
MIT License
33.27k stars 7.63k forks source link

Clicking on multi-line selection inside inline editor jumps scroll position way downward #10590

Open peterflynn opened 9 years ago

peterflynn commented 9 years ago

New repro steps with Getting Started project

  1. Open Getting Started project and open index.html
  2. Scroll down to the <h3> on line 61 ("Quick Edit for CSS and JavaScript")
  3. Invoke Quick Edit on the <h3> tag
  4. Within the inline editor, create a text selection that spans > 1 line
  5. Click the selection

If it doesn't repro, scroll so the <h3> is further down from the top of the viewport before step 3, or make your window less tall.

Old repro steps with JS inline editor

  1. Open Editor.js in the Brackets source
  2. In the Editor constructor, place the cursor on the _getModeFromDocument() call and press Ctrl-E
  3. Inside the inline editor, drag to select some text
  4. Click on the selected text

Result: scroll position jumps down about 250 lines, and focus is lost from the editor

Expected: selection cleared and cursor remains in inline editor

This looks like a regression due to PR #9584 -- if I disable text drag & drop via preferences, the bug goes away. CC @MarcelGerber

I'm only able to repro this in the JS inline editor -- not in the CSS inline editor. If it occurred in the CSS inline editor I think we'd want to block the 1.2 release until a fix is available (or make drag & drop off by default). If it really only repros in the JS inline editor, I'm less sure... Seems like we should investigate to at least see if there's an easy fix, though.

peterflynn commented 9 years ago

The "scroll" event callstack shows nothing -- it's being triggered natively, not via JS. The scroll happens immediately on mousedown (no mousemove, mouseup, or click needed).

Looking at a Timelines capture, codemirror responds to the mousedown with leftButtonStartDrag(), which sets display.scroller.draggable = true;. Then there's an almost instananeous blur-focus event pair, and by the time that's over it looks like the scroll is a foregone conclusion. There are three scroll events logged: first on CM's hidden textarea, then on some div (can't tell which one), then on the .CodeMirror-scroll div.

My guess is that CM's hidden textarea is getting positioned incorrectly, and when it's focused the browser then tries to scroll it into view (which is standard behavior when focusing any element that's out of view).

The focus change itself may be a bug, since focus appears to be nowhere useful after you mouseup. Or it may be expected, while the badly positioned focus element is the bug. Either way, seems like it's likely to be a bug on CM's side, not ours...

peterflynn commented 9 years ago

The blur event is changing focus from the inline editor's hidden textarea to the inline editor's .CodeMirror-scroll (i.e. the thing draggable=true was just set on). The focus event immediately after it is focus moving right back the other way. Since the scroller div and hidden textarea are both already in view, it's unclear why these focus changes would trigger any sort of scroll.

peterflynn commented 9 years ago

Turns out the draggable=true is a red herring. The momentary focus change is enough to cause the problem even with the draggable stuff commented out in CM.

With drag & drop disabled, CodeMirror calls preventDefault() on mousedown, blocking any momentary focus change. With drag & drop enabled, CodeMirror can't do that, because it will also block the browser from starting a drag event (this is one of my pet peeves with the DOM Events API -- preventDefault() is an overly generic catch-all that can block several things when you only want to block one of them). Hence the focus is briefly lost on every mousedown. (This is also why a barely-noticeable flicker is seen, a small but unavoidable manifestation of bug #141).

But the momentary focus change happens on every mousedown in any CM widget when drag & drop is enabled. Yet it only causes spurious scrolling in a JS inline editor -- hopefully poking at that will lead to a fix.

marcelgerber commented 9 years ago

Just FYI, I'm completely unable to repro this bug. I've followed the steps multiple times and every time I get the expected behaviour: the dragged selection becomes a cursor.

I'm on Windows 8.1, using the latest (downloaded) prerelase shell and with brackets @27a6b79e994ccecc9dd6126a28ddffbb499c755b

peterflynn commented 9 years ago

@MarcelGerber It repros 100% of the time for me on release branch (4131f1b) with no extensions installed and no local code changes. Happens with both the official 1.1 shell build and my locally built master/1.2 shell build.

I'm on Mac 10.9 -- it's possible this could be a Mac-only bug... Can anyone else confirm?

RaymondLim commented 9 years ago

@peterflynn and @MarcelGerber I can reproduce it on mac, but only if you select text that spans over multiple lines in step 3.

JeffryBooher commented 9 years ago

I don't see it on Windows 8.1

RaymondLim commented 9 years ago

I can't reproduce it on windows either. Selecting the highlighted with double-click always ends up with selecting only the word that clicked on.

peterflynn commented 9 years ago

Good observation @RaymondLim! Same for me -- only happens if selection spans > 1 line.

As a test, I wired up the inline editor to always just show the top of index.html, and it still happens (so the content of the inline editor need not be a JS file). I also rewired the provider to respond 100% of the time, and it still happens even when the outer editor is just a plain text file.

This means it does repro even with the CSS inline editor, in the right circumstances -- making this higher priority than I'd originally thought.

The bug is sensitive to a number of factors: how far below the top of the viewport the inline editor is opened (too high up and it won't repro; lower down the scroll jumps gets larger), window height (taller window requires opening the editor lower down before it will start to repro), and how many lines are spanned by the selection inside the inline editor (selecting more lines makes it start reproing higher up, and it scrolls down more lines total). That last bit reminds me of #3376, which also varies with how many lines you have selected -- and which we never got to the bottom of either :-(

peterflynn commented 9 years ago

@RaymondLim What's the connection to double-clicking when you select? I'm able to repro by simply dragging a selection (or even using Shift-arrows) -- it doesn't seem to matter how you create the selection initially...

RaymondLim commented 9 years ago

Disregard double-clicking. I was referring to the step 4 to click inside the selected text. Single click just sets the cursor to where you click on Windows, but double-click always selects the word you click on.

peterflynn commented 9 years ago

I'm able to reproduce this running Brackets in-browser, but so far I'm not able to reproduce this with a simpler standalone CM testcase (https://gist.github.com/peterflynn/0f6cd0af22edb543f56f).

That's both good and bad news: it suggests there's not a fundamental problem with CM's drag/drop mechanism, but the bad news is it could be very hard to tease apart what's different between Brackets and the simpler CM testcase...

peterflynn commented 9 years ago

Spoke too soon! If you scroll things just right, it will in fact repro in the standalone CM testcase too -- score!

peterflynn commented 9 years ago

Filed codemirror/CodeMirror#3081

le717 commented 9 years ago

@peterflynn @MarcelGerber's upstream fix at codemirror/CodeMirror#3100 has been merged, so add this to v1.3 milestone, I guess?

nethip commented 9 years ago

I am moving this out to 1.4 as we are going to pull latest version of code mirror only in 1.4

nethip commented 9 years ago

@rroshan1 Could you check if this is fixed with the latest version of code mirror in Brackets?