[CLOSED] Fix Flipview focus issues

[core-ai-bot]

Issue by petetnt
Thursday Jan 07, 2016 at 18:27 GMT
Originally opened as adobe/brackets#12060

---

This PR fixes the following FlipView focus issues:

Steps:

1. Launch brackets and make sure the working set is empty.
2. Single click on any file to have it view only.
3. Enable split view. At this stage the view of the file is present in pane 1 lets assume.
4. Flip view from pane 1 to pane 2.

Issue 1( Must-fix):

The focus remains on pane 1 which is expected but the editor in the destination pane still shows > the cursor. Ideally editor in the second pane should loose focus as the focus in in first pane.

Issue 2( Must-fix):

If you now click on the flipped editor in the second pane, focus switch happens to second pane as expected. But now if you try to click on the first pane focus is never retained on first pane and > immediately reverts to second pane. So at this point there is no way you can open a document in the first pane unless there is something in the working set of first pane and you click there to regain focus.

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 is false, which makes the focus end up in the new panel after pressing the FlipView button; setting it true 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 the retainFocusAfterFlip 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 always self.id: when the FlipView 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 master

I'll make some unit tests for these next.

ping@swmitra

---

petetnt included the following code: https://github.com/adobe/brackets/pull/12060/commits

[core-ai-bot]

Comment by swmitra
Thursday Jan 07, 2016 at 19:08 GMT

---

@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.

[core-ai-bot]

Comment by petetnt
Thursday Jan 07, 2016 at 19:15 GMT

---

Actually I am totally okay with removing the preference too: it makes the fix much more simpler too. Pushed it in as petetnt/brackets@8bc2845, if there's some requests later to get the functionality back we can always get the changes from petetnt/brackets@41e0263

[core-ai-bot]

Comment by swmitra
Friday Jan 08, 2016 at 08:38 GMT

---

@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.

1. Now click on index.html
   

2. Flip index.html view to pane 2
   

3. While pane 2 is in focus , click on main.css to have a view in pane 2
   

4. Now flip main.css view from pane 2 to pane 1
   

Notice the working set behavior. Index.html is lost from pane 2 working set.

[core-ai-bot]

Comment by swmitra
Friday Jan 08, 2016 at 08:43 GMT

---

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 ?

ping@nethip@abose

[core-ai-bot]

Comment by petetnt
Friday Jan 08, 2016 at 08:47 GMT

---

@swmitra Oh, I seem to have understood the comment from adobe/brackets#12060 (comment) wrong: when retainFocusAfterFlip was false, the focus would follow the flipped item, not stay on the flipped view. This behaviour can be flipped back though by essentially applying the change from petetnt/brackets@8bc2845 otherway around:

$headerFlipViewBtn.on("click.pane", function (e) {
            var currentFile = self.getCurrentlyViewedFile();
            var otherPaneId = self.id === FIRST_PANE ? SECOND_PANE : FIRST_PANE;
            var otherPane = MainViewManager._getPane(otherPaneId);

            // Currently active pane is not necessarily self.id as just clicking the button does not
            // give focus to the pane. This way it is possible to flip multiple panes to the active one
            // without losing focus.
            var activePaneIdBeforeFlip = MainViewManager.getActivePaneId();

            MainViewManager._moveView(self.id, otherPaneId, currentFile).always(function () {
                CommandManager.execute(Commands.FILE_OPEN, {fullPath: currentFile.fullPath,
                                                            paneId: otherPaneId}).always(function () {
                    otherPane.trigger("viewListChange");
                    self.trigger("viewListChange");

                    // Defer the focusing until other focus events have occurred.
                    setTimeout(function () {                        
                        // Focus has most likely changed: give it back to the original pane.
                        var activePaneBeforeFlip = MainViewManager._getPane(activePaneIdBeforeFlip);
                        activePaneBeforeFlip.focus();
                        self._lastFocusedElement = activePaneBeforeFlip.$el[0];
                        MainViewManager.setActivePaneId(activePaneIdBeforeFlip);
                    }, 1);
                });
            });
        });

[core-ai-bot]

Comment by petetnt
Friday Jan 08, 2016 at 08:54 GMT

---

I'll look into that MRU-issue later today...

Regarding adobe/brackets#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:

• When dragging and dropping file from one view to another, the inline-widgets from the working set the item was dragged to are kept open
• When flipped, the inline widgets are kept open from the working set the item was flipped from

Not entirely sure ATM why that is the case

[core-ai-bot]

Comment by swmitra
Friday Jan 08, 2016 at 09:01 GMT

---

thanks a lot@petetnt . I will try to look into the same doc flip case in more details.