-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Fix Flipview focus issues #12060
Fix Flipview focus issues #12060
Conversation
@petetnt That's really quick Pete. Thanks a lot for staying up and providing the fixes so quickly. It's really appreciated and makes a lot of difference 👍 I will do a detailed unit test on flip scenarios and if every thing goes fine will merge this later today. Also , pane.retainFocusAfterFlip , having a default value of false makes sense. Infact , I will suggest not to introduce this preference and rather keep the existing behaviour. User will flip a view only when he/she wants to bring in another view(some other doc) side by side. If we are keeping the focus in the current pane , it takes only a single click to get a new document in the source pane. But, if the focus is also flipped to the next pane then opening a new doc in the source pane would require 2 clicks. So, I am fine with retaining the focus in the source pane. I was also under the same impression about the focus/blur events. I found the pointer regarding the timing issue when tried to set a break point in setActivePane() and Pane.focus(). I wasn't able to reproduce it at all. But the moment I was executing the scenario without breakpoints , I was able to reproduce the issue2 10/10 times. With breakpoint it was passing 10/10 times! Regarding the last point, I guess clicking on flip button should not change the focus state of the panes. Which means if focus is in first pane and I click on the flip button of first pane , after flip first pane should retain focus. But if focus was on 2nd pane and I click flip button of pane 1 , focus should remain on pane 2. |
Actually I am totally okay with removing the |
@petetnt I did some unit testing with the changes. The 2nd issue is fixed completely but I see a change in behavior regarding the first issue. When I am flipping a view from pane 1 to pane 2, even if pane 1 is in focus, after the flip pane 2 is getting focused. I thought this shouldn't be the case. Pane focus shouldn't change based on flip. Noticed one more problem. Attaching the snapshots in sequence. Notice the working set behavior. Index.html is lost from pane 2 working set. |
One open question regarding flip and same doc split. Should we disable flip of document when it is already present in the other pane? I am seeing some strange behavior and confusing requirements. Let's assume doc1 is present in pane 1 with inline widgets ( quick edits) opened at line 3, 30 and 100. The same doc is present in pane 2 with different set of inline widgets. If we flip any of these now to the other pane , what should be the set of inline widgets in the target pane ? |
@swmitra Oh, I seem to have understood the comment from #12060 (comment) wrong: when
|
I'll look into that MRU-issue later today... Regarding #12060 (comment) I think that the optimal case would be merging the panes, keeping inline widgets open from both views and scrollling to the position to the view was flipped from, not to. Currently the behavior is mixed:
Not entirely sure ATM why that is the case |
thanks a lot @petetnt . I will try to look into the same doc flip case in more details. |
This PR fixes the following
FlipView
focus issues:The implementation is surprisingly non-trivial due to how MainViewManager, CodeMirror and CEF in general handle their
focus
events.This PR adds a new preference called
pane.retainFocusAfterFlip
which controls which panel gets focused after the flip. By default the setting isfalse
, which makes the focus end up in the new panel after pressing the FlipView button; setting ittrue
will make the currently active pane keep its focus. The default setting is purely out of my personal preference: the biggest reason for me needing a FlipView button was to have a quick way to move the new file to other pane so I can look at the one under it while I edit the new file, not the other way around. If the consensus is the other way, the default can of course be flipped.In the implementation the focus event is deferred with
setTimeout
to happen after the other focus events have occurred: AFAIK there's no way to prevent all the focus/blur events that happen in editor concurrently. In theretainFocusAfterFlip
scenario.focus()
is also called before setting the active pane Id: otherwise the editor doesn't unblur the CodeMirror-textarea for some reason, leaving the active cursor to the wrong pane.Another semi-interesting tidbit is the following:
activePaneIdBeforeFlip
is not alwaysself.id
: when theFlipView
button has been clicked, the pane doesn't get the focus: this can actually be preferable, because this enables a fast way for flipping multiple views from the non-active view to the active one without losing the focus. Other way would be giving the pane the button belongs to focus when clicked, but this makes giving the focus to the correct pane after the switch even more confusing 😕.Note that this PR is against @swmitra's
BugFixesFor1.6Beta
-branch instead of the masterI'll make some unit tests for these next.
ping @swmitra