prevent focus loss when chat code block toolbar dropdown More... closes

[meganrogge]

fixes https://github.com/microsoft/vscode-copilot/issues/9362

Fixing this as a priority today because will be out most of next week and this should've been fixed awhile ago. I thought it was invalid because had only tried on mac, where we use the native menu. It's an issue on windows and linux, where we use the custom context menu.

To test the fix on mac, I modified these lines so the custom context menu was used:

vscode/src/vs/workbench/services/contextmenu/electron-sandbox/contextmenuService.ts

Line 51 in f954362

if (!isMacintosh && !hasNativeTitlebar(configurationService)) {

Before:

demo2.mov

After:

demo.mov

[meganrogge]

@bpasero, do you have any insights here?

[bpasero]

This does not seem like the right approach. We have other instances of dropdowns that only show on hover and the issue does not apply there, notice how the "..." remains visible even when the menu shows:

I would suggest to review how other "..." entries are doing it, here for example the "..." in the "Repositories" view.

[meganrogge]

@bpasero , you are right. This works for all of the other consumers - just not the chat code block toolbar's dropdown.

I found that commenting out this line (with no other changes) fixes it cc @roblourens

vscode/src/vs/workbench/contrib/chat/browser/codeBlockPart.css

Lines 11 to 12 in ff02fe4

	.interactive-result-code-block .interactive-result-code-block-toolbar {
	display: none;

[meganrogge]

demo.mov

addressed in just the chat toolbar

[bpasero]

This looks nice, but can it be done without need for setTimeout still?

[meganrogge]

@bpasero that is necessary to fix this as I describe in the comment.

[bpasero]

But why do the other cases where we have this working not need to use setTimeout and can we try to mimic how it works there? 🤔

[meganrogge]

The other cases do not hide the toolbar by default with display none, that removes it from the dom #244990 (comment)

[roblourens]

I would expect notebook cell toolbars to have the same problem (if you set "notebook.cellToolbarVisibility": "hover"). Also, would using visibility or another method work instead of using display: none to hide the toolbar?

[meganrogge]

@roblourens I tried using visibility and opacity and got the same result

[meganrogge]

edit: opacity works - I was not applying it to all of the necessary places when I tested that before.

[roblourens]

Megan and I talked about this today, the new change with opacity is simpler but I'll look at this tomorrow, I'd like to understand more about why it needs that and doesn't work with fixing the rule but just using display.

[meganrogge]

AFAICT using display: none fully removes an element from the layout and accessibility tree, so when it’s made visible again, the browser needs a full frame to reflow and repaint before the element can receive focus—otherwise, .focus() may fail because the element isn’t yet interactable. In contrast, opacity: 0 keeps the element in the layout and accessibility tree, just visually hidden, so it can be focused immediately without waiting for a frame.

[roblourens]

We have this flag, renderDropdownAsChildElement, AKA menuAsChild which renders the context menu as a child of the button. That makes focus easier to manage because you can use :focus-within but I think it breaks in certain places, that's why it's off by default. It seems like it works fine to set it here but I asked Steven for the history to confirm it's ok here. Here's a PR that does this: #245271

For the current state, here's my interpretation

• The context menu hides, we get to this handler. The first thing it does is call onHide

  vscode/src/vs/platform/contextview/browser/contextMenuHandler.ts

  Line 132 in 966a672

  delegate.onHide?.(!!didCancel);

• That's what triggers this event to toggle our force-visibility:

  vscode/src/vs/workbench/contrib/chat/browser/codeBlockPart.ts

  Line 252 in 966a672

  toolbarElement.classList.toggle('force-visibility', e);

• But that runs immediately before this code, which returns focus to the toolbar:

  vscode/src/vs/platform/contextview/browser/contextMenuHandler.ts

  Line 143 in 966a672

  this.focusToReturn?.focus();

• So we remove our class, the toolbar hides, we can't focus it. Putting a setTimeout around our code to remove force-visibility would be another safe way to fix it.

I think my PR is much simpler, but, there is a separate issue here which the opacity fix could also help with. I swear there's an old issue but I can't find it and I'm sure I never will. The issue is that the codeblock toolbar comes after the editor in the tab order, so that you tab from the editor to the toolbar. If we'd like it to be before the codeblock, we can probably move it in the DOM (I don't know why it's like that, maybe to make it show above), then the opacity trick lets me tab directly onto the toolbar before it's visible. Also the renderDropdownAsChildElement trick and the opacity trick could be combined for the benefits of both.

Anyway that was interesting, I'll let you decide how to go forward @meganrogge

[meganrogge]

@roblourens thanks for investigating and explaining. Given using opacity fixes the focus problem and another issue, seems like we should go with this fix?

[roblourens]

Sure, to be clear, it doesn't fix the other issue on its own, there would be another change to make there

[roblourens]

Since you're about to be out, and I have all of this stuff paged into memory now, how about I take over and look at that other tabbing issue? I assume that from an accessibility standpoint it would make sense, if I'm tabbing through the response, to first tab to the codeblock toolbar, then second tab into the codeblock itself right?

[meganrogge]

I would think users would be annoyed with tabbing into a toolbar before tabbing into the code block bc they need to review the code block (ie tab into it) before deciding to take an action, if any.

[meganrogge]

But yes, if you could pls take this over, that would be great. If you don't have time, maybe we should just fix the linked issue for now?

[roblourens]

bc they need to review the code block (ie tab into it) before deciding to take an action

Oh ok then maybe we did it this way because we actually prefer it this way. In that case, it also seems like it could be confusing to shift+tab backwards into a codeblock, and focus the toolbar before the codeblock (which we get by using opacity). But also skipping the toolbar entirely when tabbing backwards seems confusing too (current state). But I can just take it over. I'll probably merge both my PR and your opacity change.