-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Bump @nextcloud/vue to 6.0.0-beta.4 and related #33516
Conversation
Issue: Broken action menusSteps to reproduce
Expected:Menus still working. Actual:Menus not opening any more, no errors in console. Notes:When opening any other menu and picking an action, like rename files, it doesn't break the other menus. The other sharing actions that change the checkbox don't cause trouble. |
it's likely a logic issue of sorts. |
the menu item closes the actions popup by setting if I comment it out the dropdown still closes and the bug persists, so there's something else at play |
even the "new" menu for creating folders doesn't open any more after that aha, and tabbing also doesn't work any more, so perhaps it's the focus trap that is not disabled properly ?! @vinicius73 |
I've compared with master where we are on nextcloud-vue 5.4.0 and the problem doesn't appear, probably because we don't have the focus trap so:
|
I don't know enough about the server code to answer this really, but |
the focus trap is supposed to deactivated when the popover's "isShown" property becomes false: https://github.com/nextcloud/nextcloud-vue/blob/master/src/components/Popover/Popover.vue#L138 maybe that specific dropdown is bypassing this somehow, will need to dig into the sharing code to find out |
Probably yes |
|
Then this PR will fix #33637 |
|
another challenge:
ERROR in ./node_modules/@nextcloud/calendar-availability-vue/lib/index.esm.js 1:0-75 |
weird, even with
|
yeah, I missed a thing... nextcloud/calendar-availability-vue#26 |
rebasing, re-updating, recompiling, etc etc etc... stay tuned |
793bbb9
to
f100b95
Compare
okay, pushed again. I haven't tested anything, we should retest again! |
f100b95
to
1732716
Compare
I fixed some XPaths, there might be more... |
9905889
to
6bf6e0b
Compare
rebased to solve the conflict |
@ChristophWurst I found it: it seems the password dialog is an old style oc-dialog, which is why the focus trap is not moving: https://github.com/nextcloud/server/blob/update-nextcloud-vue-6.0.0-beta.2/core/src/OC/password-confirmation.js#L90 we'd need to port that dialog to Vue then to make it work, or have a way to manually pause the focus trap |
this means that if we have other flows where two dialogs appear and the first one is a nextcloud-vue Dialog and the second one an old style dialog, it might fail as well. @juliushaertl I suggest you test the file picker in the text app as this might be exactly this kind of scenario. |
seems I made it worse :-(
|
6bf6e0b
to
36e14bf
Compare
ok, I made the test selector more generic and it worked for the "Recent" section, let's hope it works for everything now |
more stuff to fix...
|
5e4ba53
to
98d489f
Compare
more selectors fixed... |
Update @nextcloud/vue to 6.0.0-beta.3 Update vue and vue-template-compiler to 2.7.8 Update calendar-availability-vue to 0.5.0-beta.1 to fix conflicts. Signed-off-by: Vincent Petry <vincent@nextcloud.com>
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
98d489f
to
ede9ac2
Compare
more tests fixed ⛰️ |
failing tests unrelated, the acceptance one passes for me so it must be one of the random ones... merging |
Update @nextcloud/vue to 6.0.0-beta.3Update vue and vue-template-compiler to 2.7.8
Update calendar-availability-vue to 0.5.0-beta.1 to fix conflicts.