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

Bump @nextcloud/vue to 6.0.0-beta.4 and related #33516

Merged
merged 5 commits into from
Aug 25, 2022

Conversation

PVince81
Copy link
Member

@PVince81 PVince81 commented Aug 12, 2022

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.

% npm install "@nextcloud/calendar-availability-vue@^0.5.0-beta.1" "@nextcloud/vue@6.0.0-beta.4" "vue-template-compiler@^2.7.8" "vue@^2.7.8"
  • BUG: create link share, unshare it, then ALL of the action menus stop working until page refresh

@PVince81 PVince81 added the 2. developing Work in progress label Aug 12, 2022
@PVince81 PVince81 added this to the Nextcloud 25 milestone Aug 12, 2022
@PVince81 PVince81 self-assigned this Aug 12, 2022
@PVince81
Copy link
Member Author

Issue: Broken action menus

Steps to reproduce

  1. Open the sharing sidebar
  2. Create a share (link share or user share)
  3. Click the three dots behind the share can select "Unshare" or "Add another link"
  4. Click on any other actions menu (three dots anywhere on the page)

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.
So there's something fishy about "Unshare" and "Add another link", maybe because they force-close the dropdown somehow ?

@PVince81
Copy link
Member Author

it's likely a logic issue of sorts.
I checked the DOM and no popover appears any more on clicking, so it's not a CSS / scrolling / visibility issue

@PVince81
Copy link
Member Author

the menu item closes the actions popup by setting this.open = false: https://github.com/nextcloud/server/blob/update-nextcloud-vue-6.0.0-beta.2/apps/files_sharing/src/mixins/SharesMixin.js#L205

if I comment it out the dropdown still closes and the bug persists, so there's something else at play

@PVince81
Copy link
Member Author

even the "new" menu for creating folders doesn't open any more after that
AFAIK it's not even an actions menu

aha, and tabbing also doesn't work any more, so perhaps it's the focus trap that is not disabled properly ?! @vinicius73

@PVince81
Copy link
Member Author

PVince81 commented Aug 12, 2022

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:

  1. is the way that the sharing menus close themselves broken/illegal/unusual and should we fix it here ?
    or
  2. there's a more general issue to handle in nextcloud-vue to cover more closing cases ?

@raimund-schluessler
Copy link
Member

1. is the way that the sharing menus close themselves broken/illegal/unusual  and should we fix it here ?
   or

I don't know enough about the server code to answer this really, but Actions can be closed after one clicks an action e.g. ActionButton by setting the close-after-click prop to true, see https://github.com/nextcloud/nextcloud-vue/blob/master/src/mixins/actionText.js#L43-L49

@PVince81
Copy link
Member Author

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

@vinicius73
Copy link
Member

maybe that specific dropdown is bypassing this somehow, will need to dig into the sharing code to find out

Probably yes
I will send a PR removing focus-trap not only in afterHide, but also in before destroy

@PVince81 PVince81 mentioned this pull request Aug 12, 2022
@PVince81
Copy link
Member Author

PVince81 commented Aug 12, 2022

  • will wait for beta.3 at least for the focus trap fix

@skjnldsv skjnldsv mentioned this pull request Aug 18, 2022
@ChristophWurst
Copy link
Member

will wait for beta.3 at least for the focus trap fix

Then this PR will fix #33637

@PVince81
Copy link
Member Author

PVince81 commented Aug 22, 2022

  • damn, so now we'll need to rename all component names to prefix with Nc

@PVince81
Copy link
Member Author

PVince81 commented Aug 22, 2022

another challenge:

ERROR in ./node_modules/@nextcloud/calendar-availability-vue/lib/index.esm.js 1:0-75
Module not found: Error: Can't resolve '@nextcloud/vue/dist/Components/DatetimePicker' in '/srv/www/htdocs/server/node_modules/@nextcloud/calendar-availability-vue/lib'

@PVince81
Copy link
Member Author

weird, even with "@nextcloud/calendar-availability-vue": "^0.5.0-beta.0", the compilation still fails even after deleting node_modules and running npm ci:

ERROR in ./node_modules/@nextcloud/calendar-availability-vue/lib/index.esm.js 1:0-77
Module not found: Error: Can't resolve '@nextcloud/vue/dist/Components/DatetimePicker' in '/srv/www/htdocs/server/node_modules/@nextcloud/calendar-availability-vue/lib'

@raimund-schluessler
Copy link
Member

@PVince81
Copy link
Member Author

yeah, I missed a thing... nextcloud/calendar-availability-vue#26

@PVince81 PVince81 changed the title Bump @nextcloud/vue to 6.0.0-beta.2 and related Bump @nextcloud/vue to 6.0.0-beta.3 and related Aug 22, 2022
@PVince81
Copy link
Member Author

rebasing, re-updating, recompiling, etc etc etc... stay tuned

@PVince81 PVince81 force-pushed the update-nextcloud-vue-6.0.0-beta.2 branch from 793bbb9 to f100b95 Compare August 22, 2022 15:42
@PVince81
Copy link
Member Author

okay, pushed again. I haven't tested anything, we should retest again!

@PVince81 PVince81 force-pushed the update-nextcloud-vue-6.0.0-beta.2 branch from f100b95 to 1732716 Compare August 23, 2022 12:33
@PVince81
Copy link
Member Author

I fixed some XPaths, there might be more...

@PVince81 PVince81 force-pushed the update-nextcloud-vue-6.0.0-beta.2 branch from 9905889 to 6bf6e0b Compare August 24, 2022 15:12
@PVince81
Copy link
Member Author

rebased to solve the conflict

@PVince81
Copy link
Member Author

@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

@PVince81
Copy link
Member Author

I've raised #33675 for the password dialog specifically.
I think this one will also appear on other pages so we should port it to Vue anyway.
@Pytal can you take care ? (I took the liberty to assign you there, thanks!)

@PVince81
Copy link
Member Author

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.

@PVince81 PVince81 added 4. to release Ready to be released and/or waiting for tests to finish accessibility and removed 2. developing Work in progress labels Aug 24, 2022
@PVince81
Copy link
Member Author

seems I made it worse :-(

 AppNavigationContext::iOpenTheSection()
      Recent section item in App Navigation could not be found after 100 seconds (NoSuchElementException)

@PVince81 PVince81 force-pushed the update-nextcloud-vue-6.0.0-beta.2 branch from 6bf6e0b to 36e14bf Compare August 24, 2022 15:56
@PVince81
Copy link
Member Author

ok, I made the test selector more generic and it worked for the "Recent" section, let's hope it works for everything now

@PVince81
Copy link
Member Author

more stuff to fix...

  • create menu button
# FileListContext::iCreateANewFolderNamed()
      │ Create menu button in file list could not be clicked
      │ Exception message: element not interactable
      │   (Session info: chrome=90.0.4430.85)
      │   (Driver info: chromedriver=90.0.4430.24 (4c6d850f087da467d926e8eddb76550aed655991-refs/branch-heads/4430@{#429}),platform=Linux 5.4.0-121-generic x86_64)
      │ Trying again
  • selectors for share permissions


# FilesAppSharingContext::iSetTheShareWithAsNotCreatable()
--


      Share with 
user0 menu trigger in the details view in Files app could not be found 
after 20 seconds (NoSuchElementException)
  • section selector still buggy
AppNavigationContext::iOpenTheSectionOf()
      invalid selector: Unable to locate an element with the xpath expression (//html//*[@id="app-navigation" or contains(@class, 'app-navigation')])[1]//li[normalize-space() = 'Administration']/following-sibling::li/*[contains(normalize-space(), 'Basic settings'])/.. because of the following error:
      SyntaxError: Failed to execute 'evaluate' on 'Document': The string '(//html//*[@id="app-navigation" or contains(@class, 'app-navigation')])[1]//li[normalize-space() = 'Administration']/following-sibling::li/*[contains(normalize-space(), 'Basic settings'])/..' is not a valid XPath expression.
  • and many more...

@PVince81 PVince81 force-pushed the update-nextcloud-vue-6.0.0-beta.2 branch 2 times, most recently from 5e4ba53 to 98d489f Compare August 24, 2022 22:06
@PVince81
Copy link
Member Author

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>
@PVince81 PVince81 force-pushed the update-nextcloud-vue-6.0.0-beta.2 branch from 98d489f to ede9ac2 Compare August 25, 2022 07:55
@PVince81
Copy link
Member Author

more tests fixed ⛰️

@PVince81
Copy link
Member Author

failing tests unrelated, the acceptance one passes for me so it must be one of the random ones...

merging

@PVince81 PVince81 merged commit 2911dff into master Aug 25, 2022
@PVince81 PVince81 deleted the update-nextcloud-vue-6.0.0-beta.2 branch August 25, 2022 11:29
@blizzz blizzz mentioned this pull request Aug 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish accessibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants