Use inject/provide to define popover/dropdown container

[vinicius73]

It helps to define the container of all popovers in the component tree at the same time.

Solving part of

• Recursive calls of focusTrap() in NcActions component #3154
• Slow menu opening since nc 7 #3300
• MenuBar throws errors on master (focus-trap too much recursion) nextcloud/text#2842
• [Bug]: Crash when share a file inside viewer app nextcloud/server#34404

<template>
<div :id="randomID">
    <!-- any popover (NcActions, NcEmojiPicker, ...)  will use the randomID as container by default -->
</div>
</template>

<script>
import { defineComponent } from 'vue'
import { NC_KEY_POPOVER_CONTAINER } from '@nextcloud/vue'

const randomHash = () => (Math.ceil((Math.random() * 100000) + 1000)).toString(16)

export default defineComponent({
  data() {
    return {
       randomID: `custom-container-${randomHash()}`,
    }
  },
  
  provide () {
    return {
       [NC_KEY_POPOVER_CONTAINER]: `#${this.randomID}`,
    }
  }
})
</script>

[raimund-schluessler]

@vinicius73 Could you please explain why changing the default container from body to some random div solves the issues mentioned?
And it requires manual work from every app developer, because otherwise, it falls back to body anyways. This does not seem ideal.

[vinicius73]

@raimund-schluessler this problem appears when we have more than one instance of focus trap active ate the same time (as mentioned #3154 and focus-trap/focus-trap#428)

In text it happens when text is open in the viwer (a NcModal) and an option from menu bar is open (nextcloud/text#2842)

The solution was moving the container of NcMenu (who uses NcPopover under the hood) to MenuBar element (nextcloud/text#3066).

If the focus trap is created inside a children element of other focus trap, we do not have this error.

We've faced the same problem in server (#3300)
If any viewer is open, interact with sidebar (specially in share options) will trigger the same error.

The viewer app already handles part of this problem

https://github.com/nextcloud/viewer/blob/2b5ed86ec105f65b8e3990373885f59dc9772749/src/views/Viewer.vue#L906-L913

Catching the sidebar event, and update its focus trap instance.

So, using a common container solve the problem without hard changes in next-cloud components.

---

This PR does not change any actual behavior of NcPopover.

In some cases, we have a lot of NcActions or similar at the same context.

This change makes the app developers able to handle this problem (if necessary) without hard changes, until we work in a more robust solution (a singleton instance of `focus trap focus-trap/focus-trap#428)

I've already prepared a PR for server

diff --git a/apps/files_sharing/src/views/SharingTab.provider.js b/apps/files_sharing/src/views/SharingTab.provider.js
new file mode 100644
index 00000000000..38fe212e57b
--- /dev/null
+++ b/apps/files_sharing/src/views/SharingTab.provider.js
@@ -0,0 +1,12 @@
+export const SHARING_TAB_ID = Symbol('menu::id')
+
+export const useSharingTabIDMixin = {
+	inject: {
+		$sharingTabID: { from: SHARING_TAB_ID, default: null },
+	},
+	computed: {
+		sharingTabIDSelector() {
+			return `#${this.$sharingTabID}`
+		},
+	},
+}
diff --git a/apps/files_sharing/src/views/SharingTab.vue b/apps/files_sharing/src/views/SharingTab.vue
index d1416a6c0d1..a5eea691ee4 100644
--- a/apps/files_sharing/src/views/SharingTab.vue
+++ b/apps/files_sharing/src/views/SharingTab.vue
@@ -21,7 +21,7 @@
   -->
 
 <template>
-	<div :class="{ 'icon-loading': loading }">
+	<div :id="randomID" :class="{ 'icon-loading': loading }">
 		<!-- error message -->
 		<div v-if="error" class="emptycontent" :class="{ emptyContentWithSections: sections.length > 0 }">
 			<div class="icon icon-error" />
@@ -89,6 +89,7 @@
 import { CollectionList } from 'nextcloud-vue-collections'
 import { generateOcsUrl } from '@nextcloud/router'
 import NcAvatar from '@nextcloud/vue/dist/Components/NcAvatar'
+import { NC_KEY_POPOVER_CONTAINER } from '@nextcloud/vue/dist/providers.js'
 import axios from '@nextcloud/axios'
 import { loadState } from '@nextcloud/initial-state'
 
@@ -120,8 +121,16 @@ export default {
 
 	mixins: [ShareTypes],
 
+	provide() {
+		return {
+			[NC_KEY_POPOVER_CONTAINER]: `#${this.randomID}`
+		}
+	},
+
 	data() {
 		return {
+			randomID: `SharingTab-${(Math.ceil((Math.random() * 900000) + 100)).toString(16)}`,
+
 			config: new Config(),
 
 			error: '',

[raimund-schluessler]

I see, but I am still a bit puzzled, since they had a common container already, it was just not a provided div, but simply body, no?. So what I don't get is, why switching away from body solves this.

Edit: I guess, I don't understand the solution completely yet 🙈

[max-nextcloud]

I see, but I am still a bit puzzled, since they had a common container already, it was just not a provided div, but simply body, no?. So what I don't get is, why switching away from body solves this.

My understanding is that if you have two focus traps sitting next to each other (for example as siblings) in the DOM they will compete for the focus. The focus can only be in one of the traps.
But if one focus trap is the parent of the other the focus can be in the child and therefore in both focus traps - so they are 'happy'.

If dropdown menus are attached to the body they are outside of any possible other focus trap (in particular the modal) and we have the competing situation. Attaching them to an element inside the current focus trap will make sure the focus can be in both traps at the same time.

So the problem could be solved by changing the container of all popovers inside a modal. However this can be tedious if there are multiple menus. This commit allows changing the default container for all popovers in the modal at once.

[raimund-schluessler]

Ah, ok, I think now I got it. Thanks. I somehow was under the impression that it's about multiple NcActions fighting about the focus, for which it wouldn't help to give them a common container. But it's in fact about a NcModal and multiple NcActions within and giving the actions a container within the modal solves the issue.

So this PR just adds a convenience function for an app-specific solution that would be possible already (e.g. by touching every NcActions in a modal), right?

That's fine with me. But since it still requires devs to adjust their code, a note about this in the documentation for every component using the focus-trap (i.e. NcActions, NcModal, etc.) would be great.

[ChristophWurst]

👍 🐘

[skjnldsv]

Sorry, I think this whole approach is far too messy and complex. Maintaining this in the long run will be too cumbersome.
If multiple traps are fighting between each others, we should fix the issue at the root cause.

Here is my suggestion:

1. We create a service that handle all the registered focus traps
2. When a new focus trap is registered and activated, it pauses all the others. Example given:
   1. You open a Settings modal (like talk or mail)
   2. You open an Actions menu inside this modal
   3. Those two items will have thei rown focus traps
   4. The one that should have the priority should be the last activated, as they are (presumably) always initiated by a user action
   5. When the Actions menu gets closed, the previous trap can resume its activity
3. This should maybe require us to create a @nextcloud/focus-trap, where the first to register one initialize the manager.
   Storing all data in window._nc_focus_trap_service or something. The same way we do with the event bus. This is how we properly share a service across various independent services/apps.