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

Editor scrolls back to top when returning to it after Quick Open #2951

Closed
peterflynn opened this issue Feb 26, 2013 · 9 comments
Closed

Editor scrolls back to top when returning to it after Quick Open #2951

peterflynn opened this issue Feb 26, 2013 · 9 comments
Assignees
@peterflynn
Labels
fixed but not closed
Milestone
sprint 22

Comments

@peterflynn
Copy link
Member

Finally nailed down some repro steps for this!

  1. Open file A, scroll down one or more screenfuls and place the cursor somewhere there
  2. Use Quick Open to open some other file B
  3. Switch back to file A (via whatever mechanism -- Ctrl+Tab, click in working set, etc.)
@peterflynn peterflynn mentioned this issue Feb 28, 2013
Closed
@ghost ghost assigned njx Mar 1, 2013
@pthiess
Copy link
Contributor

pthiess commented Mar 1, 2013

Reviewed, medium priority seems reasonable, @njx

@peterflynn
Copy link
Member Author

Looks somewhat unrelated to #2985 after all. I think the bug here is that the value of getCurrentFullEditor() during the ModalBar ctor isn't necessarily the same as the value of getCurrentFullEditor() during ModalBar.close(). Because closing happens at the end of QuickOpen._handleItemSelect() and is further delayed by a timeout, it's basically guaranteed that the editor will have already switched by the time close() is called.

Things brings two questions to mind:

  • Is it even possible to restore the scroll pos of the correct editor after it's invisible? Will CM choke trying to measure something in code that was triggered by scrollTo()? Will the update stick once the editor becomes visible and gets refreshed?
  • What about the editor you were switching to? If it's also affected by the ModalBar's presence (before it closes), does that mean we have to run the restore-scroll-pos code on both editors?

@peterflynn
Copy link
Member Author

Also note: the timing of this stuff is affected by whether file B is already in the working set -- the editor switch is much more synchronous if so.

peterflynn added a commit that referenced this issue Mar 2, 2013
@peterflynn
Fix bug #2951 (Many commands [including save] apply to wrong file after
1407aba 
using Quick Open) - Block re-entrant focus attempts. This bug occurs with
any blur handlers that call EditorManager.focusEditor(), since that can
cause such re-entrant focus attempts (with two different editors).

Blur handlers run synchronously while an editor is in the middle of
getting focus, so the state is a little funny - activeEditor hasn't been
updated yet (blur fires before focus), so focusEditor() tries to focus the
old Editor that's in the process of losing focus. Webkit seems to ignore
tht attempt; but CodeMirror assumes that programmatic focus requests always
succeed, so CodeMirror's state.focused flag becomes wrong. This has a
delayed effect the NEXT time that editor gains focus: it doesn't dispatch a
focus event since CM thought it already had focus; as a result, activeEditor
never updates and any code that uses activeEditor gets the wrong value.

I think we could fix this by triggering stuff on focusIn instead of blur
(which is how ModalBar works and why the Find bar doesn't have a similar
bug). But it seems safer just to cut it off at the root: re-entrant attempts
to focus Editors.
@peterflynn peterflynn mentioned this issue Mar 2, 2013
Merged
@dangoor
Copy link
Contributor

dangoor commented Mar 3, 2013

I can still reliably reproduce this after the landing of #3023, so I guess you're right about it not being quite related.

@RaymondLim
Copy link
Contributor

@dangoor I believe the root cause of this is also causing your Quick Open unit test failing (#2696).

@njx
Copy link

njx commented Mar 8, 2013

Nominating for sprint 22.

@njx
Copy link

njx commented Mar 13, 2013

Turns out this is due to a subtle race condition: if you call scrollTo() on a CodeMirror instance and then immediately hide the editor before the resulting scroll events are processed, CM ends up scrolling to (0,0). I submitted codemirror/codemirror5#1350 to fix this.

@ghost ghost assigned peterflynn Mar 13, 2013
@njx
Copy link

njx commented Mar 13, 2013

FBNC to @peterflynn

@dangoor dangoor mentioned this issue Mar 18, 2013
Merged
@peterflynn
Copy link
Member Author

Confirmed

@core-ai-bot core-ai-bot mentioned this issue Aug 29, 2021
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@peterflynn peterflynn

Labels
fixed but not closed
Projects
None yet
Milestone
sprint 22
Development

No branches or pull requests
5 participants
@dangoor @njx @peterflynn @RaymondLim @pthiess