Ensure active element is preserved when opening context menu

[martin-fleck-at]

What it does

Fixes #10851

This change ensures that the active element from the document is preserved when opening the menu. If there is already another element that is preserved, this fix simply does nothing as the previous preserved element is kept.

Please note that the top menu is not affected by this since the aboutToShow is called through the DynamicMenuBarWidget.

How to test

1. Start Theia (I used the Electron app but it might occur also in the Browser app)
2. ensure that browser menus are used (window.titleBarStyle: custom)
3. Select any file and open the context menu and hit 'Copy'
4. Open the context menu and hit 'Paste'
5. See that the file is copied as expected

Review checklist

• As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• As a reviewer, I agree to behave in accordance with the review guidelines

[vince-fugnitto]

@martin-fleck-at do you have any additional details on how to reproduce the problem, the behavior as you described works fine for me on master. I thought perhaps that no file selection should be present when performing paste, which does not work on master, but the pull-request also does not fix it.

If there is no file selection however the paste does not work, but it is also true with this pull-request.

[martin-fleck-at]

@vince-fugnitto Interesting. I just tested this as shown in the video. I am using the current master, building with yarn && yarn build and then starting the Launch Electron Backend launch config from the debug perspective. I am using Ubuntu with Chrome as a default browser. Simply selecting a file, hitting 'Copy' from the context menu and then hitting 'Paste' with right click on the same file does not work for me without the fix. I just double checked but there is nothing else I am doing.

This PR does not aim to fix the empty selection Paste (if that is even an issue), so that is fine.

[vince-fugnitto]

@martin-fleck-at this is master for me, I opened the dev tools to show the sources of the method in question.
Perhaps I am not reproducing it properly:

copy-paste-explorer.mp4

[martin-fleck-at]

@vince-fugnitto Seems like I was not 100% correct about the necessary steps to reproduce this bug BUT thanks to your video I know see it. The bug happens when you use a custom title bar (Preferences > Window: Title Bar Style). In that case, the context menu seems to be rendered slightly differently.

[vince-fugnitto]

@martin-fleck-at thank you for the follow-up I was able to reproduce finally 👍
I confirm that the changes work correctly thanks to your updates.

[colin-grant-work]

There is a note in the code about this phenomenon. It seems we should at least remove the comment, if we are now calling preserveFocusedElement for context menus, but it may warrant further investigation, since this bug may suggest that something about the command registry is not behaving as expected.

theia/packages/core/src/browser/menu/browser-menu-plugin.ts

Lines 259 to 260 in b8df294

	// Hint: this is not called from the context menu use-case, but is not required.
	// For the context menu the command registry state is calculated by the factory before `open`.

[martin-fleck-at]

@colin-grant-work I did not remove the comment because I do not actually trigger the aboutToShow method but instead only call the preserveFocusedElement. The comment was exactly the reason I did not want to do it since, as you mentioned, there seems to be something special going on. The preserveFocusedElement, however, is pretty safe since it does not override any existing preserved element and thus should not cause much harm (famous last words ;-)).

[colin-grant-work]

@martin-fleck-at, the comment above aboutToShow is about why preserveFocusedElement is called there and not in the context menu context. Since we're now calling it in both contexts, the comment is no longer accurate (it is incorrect to say 'is not called from the context menu use-case'), so it would be better to remove the comment.

[martin-fleck-at]

@colin-grant-work I see, I interpreted it as: The aboutToShow is not called in the context menu use case (since it is called from DynamicMenuBarWidget) as the command registry state is calculated by the factory before open (and therefore we do not need to clear items and re-populate the sub-menus). However, I removed the comment as you suggested, thank you!

[colin-grant-work]

LGTM. 👍