Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Insiders] Search viewlet click/dblclick behavior changed -- not in a good way #24717

Closed
eamodio opened this issue Apr 13, 2017 · 9 comments
Closed
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug search Search widget and operation issues verified Verification succeeded

Comments

@eamodio
Copy link
Contributor

eamodio commented Apr 13, 2017

  • VSCode Version: Code - Insiders 1.12.0-insider (d1b315f, 2017-04-13T06:05:22.216Z)
  • OS Version: Darwin x64 16.5.0

FYI I have "workbench.editor.revealIfOpen": true set

Steps to Reproduce:

  1. Open the search viewlet and search for something with results
  2. Close all editors or make sure the editor that will open isn't already open
  3. Click on a result -- 👍 it will open the file in a preview tab in the column of the active editor

  1. Open the search viewlet and search for something with results
  2. Make sure the editor that will open is already open, but the active editor is in another column
  3. Click on a result -- 🐛 it will switch to the opened editor tab, but won't focus it -- the original active editor will still be active and focused

  1. Open the search viewlet and search for something with results
  2. Close all editors or make sure the editor that will open isn't already open
  3. Double click on a result -- 🐛 it will open the editor in a preview tab, next to the active one, but then will open another preview tab in the column to the right of the active one -- so you end up with 2 preview tabs for the file. This will keep going if you keep double-clicking until the 3 columns all have the file opened in them. And they are all preview tabs

A single click used to activate/focus the editor if it was already open or open a preview tab if not. While a double click used to both activate/focus and pin the editor if it was already open or open a pinned tab if not.

My guess is it has to do with the changes in this commit a079ef1

/cc @sandy081

@roblourens
Copy link
Member

I fixed the doubleclick issue, but I'm not sure about the first issue. I see it activate the editor with the result file, but focus stays in the search results, which is what I expect - I might want to click on a result, then use the arrow keys to navigate in the search results. Did it ever work differently?

@eamodio
Copy link
Contributor Author

eamodio commented Apr 14, 2017

@roblourens looking at this more (for the single click issue) I only had "workbench.editor.revealIfOpen": true set in the insiders, so that must have been the difference.

I do still find it a little odd, that if you have 2 columns:

| Tab A | Tab B | ----------------- | Tab C |

If B is the active editor, clicking on a search item that opens A will switch to Tab A as well as change the active editor to A -- extensions will get the event and the tab will be highlighted

If A is the selected tab in column 1 andC is the active editor, clicking on a search item that opens A will not change the active editor to A (C will remain the active editor and stay highlighted -- though the active line highlight will switch to A) -- extensions will not get the event and the tab will not be highlighted

If B is the selected tab in column 1 andC is the active editor, clicking on a search item that opens A will switch to Tab A but not change the active editor to A (C will remain the active editor and stay highlighted -- though the active line highlight will switch to A) -- extensions will not get the event and the tab will not be highlighted

In all cases the focus still stays in the search viewlet. 👍

IMO it would be best if all of these cases behaved the same -- switch to the correct tab, and make it the active editor. Switching the active editor in all cases would help, again imo, with knowing which tab I should actually be looking in -- the active line highlight is much more subtle than the tab highlight (and I assume it could even be turned off)

@roblourens
Copy link
Member

Ok, I see the problem. That does seem inconsistent. Here's a gif of the first issue -

If A is the selected tab in column 1 andC is the active editor, clicking on a search item that opens A will not change the active editor to A

apr-14-2017 10-31-56

The match in completionModel.test.ts is revealed, but you can see that typescriptServices is still the active editor, from the color of the tab title.

@sandy081 I don't know whether this is a regression, but do you know whether we can set completionModel.test.ts to be the active editor in this case? We set revealIfVisible when opening the editor, but that doesn't make it the active editor. I don't see another option or API for this.

@eamodio
Copy link
Contributor Author

eamodio commented Apr 14, 2017

FWIW -- with "workbench.editor.revealIfOpen": true set, I see the same behavior in 1.11.2 but don't know about it earlier than that.

@sandy081
Copy link
Member

@roblourens thanks for fixing the double click issue.. It was caused due to the refactoring.

Regarding focussing active editor, it is not a regression from Search functionality but agree behaviour is not correct and have to make the focus editor active.

@bpasero Please see the GIF from @roblourens in above comment. May I know, why is not editor group with completionModel.test.ts is not made active when opening it? Here are the options we pass to open editor with file match in the above case with single click.

this.editorService.openEditor({
			resource,
			options: {
				preserveFocus: true,
				pinned: false
				selection,
				revealIfVisible: true
			}
		}, false) 

@sandy081 sandy081 added search Search widget and operation issues bug Issue identified by VS Code Team member as probable bug labels Apr 18, 2017
@sandy081 sandy081 added this to the April 2017 milestone Apr 18, 2017
@bpasero
Copy link
Member

bpasero commented Apr 18, 2017

@sandy081 we only set it active when focus moves to it (so preserveFocus: true would not trigger it), you can use IEditorGroupService.activateGroup to force the group to be active.

@sandy081 sandy081 modified the milestones: May 2017, April 2017 Apr 27, 2017
@sandy081 sandy081 modified the milestones: June 2017, May 2017 May 30, 2017
@sandy081 sandy081 modified the milestones: Backlog, June 2017 Jun 26, 2017
@roblourens roblourens modified the milestones: Backlog, September 2018 Sep 12, 2018
@bpasero
Copy link
Member

bpasero commented Sep 12, 2018

@eamodio @roblourens

Open the search viewlet and search for something with results
Make sure the editor that will open is already open, but the active editor is in another column
Click on a result -- 🐛 it will switch to the opened editor tab, but won't focus it -- the original active
editor will still be active and focused

Please stay away from changing that behaviour. IF you click into a viewlet with the mouse. DO NOT move focus away, otherwise you will BREAK keyboard accessibility (+ any UX guideline that exists).

If the issue is about making the editor active or not, then yes, it should become active, but not focused.

@eamodio
Copy link
Contributor Author

eamodio commented Sep 12, 2018

Yeah, I shouldn't have said focused. For me it is about it being active.

@roblourens
Copy link
Member

Yeah the only change is to activate the group. I can still

  • Click a search result
  • See the new group activated
  • Focus is still in the search view, arrow keys move the selected search result, etc.

@mjbvz mjbvz added the verified Verification succeeded label Sep 26, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators Oct 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug search Search widget and operation issues verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

5 participants