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

The viewer app can no longer be closed after a double click, the lightbox remains black #893

Closed
mariomaurer opened this issue May 11, 2021 · 8 comments · Fixed by #1247
Closed
Labels
0. Needs triage Pending approval or rejection. This issue is pending approval. bug Something isn't working

Comments

@mariomaurer
Copy link

Describe the bug
if you double click an image then the viewer app opens twice and you can only close the first lightbox

To Reproduce
Steps to reproduce the behavior:

  1. Go to App Files
  2. Double Click on a Picture
  3. The Lightbox opens
  4. Close the Lightbox in the upper right corner with the close button
  5. The first lightbox closes and the app stays black

Expected behavior
There should be a double click prevention when opening the viewer app.
It should be redirected back to the file browser after closing it

Screenshots
Bildschirmfoto 2021-05-11 um 10 07 34
Bildschirmfoto 2021-05-11 um 10 07 23
Bildschirmfoto 2021-05-11 um 10 07 10

Desktop (please complete the following information):

  • OS: Windows/MacOsX
  • Browser Chrome/Firefox
  • Chrome Version 90.0.4430.93
  • Firefox Version 86.0 (64-Bit)

Browser log
cloud.gablerband.at-1620721383850.log

@MichaelEPope
Copy link

Trying to figure this one out as it's a pretty big deal for the company I work for.

First time trying to contribute to NextCloud. So far, adding a timer variable that blocks double opens in Viewer.js isn't fixing things, but I'm probably doing things wrong. All of my edits are in the Chrome Web Tools too, since I'm having a hard time editing it in the actual web server itself. Gonna try a few more things and look in a few more places.

I want to see if I can get this one fixed at least partly in the next two weeks.

@skjnldsv
Copy link
Member

@MichaelEPope this issue is unfortunately not a priority for us at the moment. :/
Considering you are using Nextcloud for your company, you could get assistance and issue prioritising with a Nextcloud subscription.

@MichaelEPope
Copy link

MichaelEPope commented Apr 14, 2022

@skjnldsv No worries. :) I was planning on trying to figure it out myself in the next two weeks - it would be really entitled for me to expect you guys to fix something when you are not getting paid to fix it. If I can get a pull request that works though, can you guys merge it?

About subscriptions, I might get a subscription in the future... but I'm just introducing NextCloud to the company, so it would be pretty hard me to sell them about it right now.

@skjnldsv
Copy link
Member

If I can get a pull request that works though, can you guys merge it?

Definitely! I will also assist you my best if I have time for it :)
While we are also a company, this is and will always remain an open source project ✌️

@MichaelEPope
Copy link

MichaelEPope commented Apr 14, 2022

EDIT: The code I wrote earlier actually had an issue, probably due to the cache... so it didn't end up working. However, I did find out what the problem is. The problem is when an item is clicked while the Viewer is loading up an item.

I put a 1 second delay on opening a file in the viewer in File Action Handler and just kept resetting a timeout every time it was called, and it blocked the double click issue.... unless the person clicked after that 1 second precisely in the gap between when the file started loading in the viewer and when it finished loading in the viewer.

So if we fix that issue, the entire thing is solved. A simple solution might actually be to make the black background of the viewer not pass clicks through... that might fix it.

@MichaelEPope
Copy link

MichaelEPope commented Apr 15, 2022

Figured out part of it. When a file is double clicked, the openFile() method (as well as the File Handler) is only called once, but the file is opened twice. When that happens, the first file automatically closes, and calls close() and cleanup(). However, the second file is still on the screen (sometimes without the black background). When you close that file, close() is called, but cleanup() is not.

So there's two ways to fix this. One, if I can figure out what's causing the file to open a second time, I can either block it, or pass it to openFile() and do some handling there to block one of the copies from opening. The other way to fix this is to just deal with the issue and write some code that makes sure the black background still appears, and then call cleanup() inside of the close() method.

Gonna take some more time to look things over, but this is how things are looking now.

I'm going to put a patch in for the cleanup, even without the black box for a temporary fix. I'll look into other things later though.

@MichaelEPope
Copy link

Hey @skjnldsv I created a pull request. However, I think I didn't properly put the 'sign off' text in the initial commit (although I put it in the pull request). I also... didn't do any refactoring for tests. Is there a place you'd like me to refactor to trigger this?

#1214

Thanks.

MichaelEPope pushed a commit to MichaelEPope/nextcloud-viewer that referenced this issue Apr 21, 2022
…extcloud#893, where if you double click an item that opens in the viewer, it opens in a way that the viewer can't close.  It works for the Viewer for Images, Videos, and PDFs for sure.  I didn't test audio, but it probably works for it as well.  The main cause of the issue is that when a file is double clicked, the file is opened twice.  However, something weird occurs when this happens.  openFile() is only called once, while close() and cleanup() are immediately called.  What I'm guessing happens is that the original file loads, and is closed somehow, followed by the second file opening.  I'm not sure which of those files calls openFile() and which doesn't.  One result of this though is that when the second file is closed, close() is called, but cleanup() is not.  This patch ensures that it is called, which resolves the issue.  There are still some cases where the double clicking can cause the black background of the viewer to not appear (to get this to happen, you have to double click, but put some spaces between the clicks that are a part of the double click, it's difficult to get the timing right, so try it a few times).  But this at least solves the one issue.  This is not really meant to be a permanent workaround probably.  There are better things to do then run cleanup(), but I can't set up my development environment well enough to test it.  But this is suitable for now.

Signed-off-by: Michael Pope <michael.pope.email@gmail.com>
@MichaelEPope
Copy link

Resubmitted the pull request as ( #1218 ) because I couldn't get it to pass DCO by editing it inside the Github UI. Sorry about that. You can ignore #1214 and #1217 .

MichaelEPope pushed a commit to MichaelEPope/nextcloud-viewer that referenced this issue Apr 21, 2022
…extcloud#893, where if you double click an item that opens in the viewer, it opens in a way that the viewer can't close.  It works for the Viewer for Images, Videos, and PDFs for sure.  I didn't test audio, but it probably works for it as well.  The main cause of the issue is that when a file is double clicked, the file is opened twice.  However, something weird occurs when this happens.  openFile() is only called once, while close() and cleanup() are immediately called.  What I'm guessing happens is that the original file loads, and is closed somehow, followed by the second file opening.  I'm not sure which of those files calls openFile() and which doesn't.  One result of this though is that when the second file is closed, close() is called, but cleanup() is not.  This patch ensures that it is called, which resolves the issue.  There are still some cases where the double clicking can cause the black background of the viewer to not appear (to get this to happen, you have to double click, but put some spaces between the clicks that are a part of the double click, it's difficult to get the timing right, so try it a few times).  But this at least solves the one issue.  This is not really meant to be a permanent workaround probably.  There are better things to do then run cleanup(), but I can't set up my development environment well enough to test it.  But this is suitable for now.

Signed-off-by: Michael Pope <michael.pope.email@gmail.com>
artonge added a commit that referenced this issue May 16, 2022
First click: open the viewer
Second click: close the viewer

But the second click does not correctly close the Viewer, as the method used to cancel the requests does not work.

This PR use modern API to cancel the requests. The requests are now correctly aborted, and the `openFile` method exit because the request call throws an exception.

Fix #893

https://axios-http.com/docs/cancellation
https://developer.mozilla.org/en-US/docs/Web/API/AbortController

Signed-off-by: Louis Chemineau <louis@chmn.me>
artonge added a commit that referenced this issue May 16, 2022
First click: open the viewer
Second click: close the viewer

But the second click does not correctly close the Viewer, as the method used to cancel the requests does not work.

This PR use modern API to cancel the requests. The requests are now correctly aborted, and the `openFile` method exit because the request call throws an exception.

Fix #893

https://axios-http.com/docs/cancellation
https://developer.mozilla.org/en-US/docs/Web/API/AbortController

Signed-off-by: Louis Chemineau <louis@chmn.me>
nextcloud-command pushed a commit that referenced this issue May 16, 2022
First click: open the viewer
Second click: close the viewer

But the second click does not correctly close the Viewer, as the method used to cancel the requests does not work.

This PR use modern API to cancel the requests. The requests are now correctly aborted, and the `openFile` method exit because the request call throws an exception.

Fix #893

https://axios-http.com/docs/cancellation
https://developer.mozilla.org/en-US/docs/Web/API/AbortController

Signed-off-by: Louis Chemineau <louis@chmn.me>
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
backportbot-nextcloud bot pushed a commit that referenced this issue May 17, 2022
First click: open the viewer
Second click: close the viewer

But the second click does not correctly close the Viewer, as the method used to cancel the requests does not work.

This PR use modern API to cancel the requests. The requests are now correctly aborted, and the `openFile` method exit because the request call throws an exception.

Fix #893

https://axios-http.com/docs/cancellation
https://developer.mozilla.org/en-US/docs/Web/API/AbortController

Signed-off-by: Louis Chemineau <louis@chmn.me>
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
backportbot-nextcloud bot pushed a commit that referenced this issue May 17, 2022
First click: open the viewer
Second click: close the viewer

But the second click does not correctly close the Viewer, as the method used to cancel the requests does not work.

This PR use modern API to cancel the requests. The requests are now correctly aborted, and the `openFile` method exit because the request call throws an exception.

Fix #893

https://axios-http.com/docs/cancellation
https://developer.mozilla.org/en-US/docs/Web/API/AbortController

Signed-off-by: Louis Chemineau <louis@chmn.me>
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
backportbot-nextcloud bot pushed a commit that referenced this issue May 17, 2022
First click: open the viewer
Second click: close the viewer

But the second click does not correctly close the Viewer, as the method used to cancel the requests does not work.

This PR use modern API to cancel the requests. The requests are now correctly aborted, and the `openFile` method exit because the request call throws an exception.

Fix #893

https://axios-http.com/docs/cancellation
https://developer.mozilla.org/en-US/docs/Web/API/AbortController

Signed-off-by: Louis Chemineau <louis@chmn.me>
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
nextcloud-command pushed a commit that referenced this issue May 17, 2022
First click: open the viewer
Second click: close the viewer

But the second click does not correctly close the Viewer, as the method used to cancel the requests does not work.

This PR use modern API to cancel the requests. The requests are now correctly aborted, and the `openFile` method exit because the request call throws an exception.

Fix #893

https://axios-http.com/docs/cancellation
https://developer.mozilla.org/en-US/docs/Web/API/AbortController

Signed-off-by: Louis Chemineau <louis@chmn.me>
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
nextcloud-command pushed a commit that referenced this issue May 17, 2022
First click: open the viewer
Second click: close the viewer

But the second click does not correctly close the Viewer, as the method used to cancel the requests does not work.

This PR use modern API to cancel the requests. The requests are now correctly aborted, and the `openFile` method exit because the request call throws an exception.

Fix #893

https://axios-http.com/docs/cancellation
https://developer.mozilla.org/en-US/docs/Web/API/AbortController

Signed-off-by: Louis Chemineau <louis@chmn.me>
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
nextcloud-command pushed a commit that referenced this issue May 17, 2022
First click: open the viewer
Second click: close the viewer

But the second click does not correctly close the Viewer, as the method used to cancel the requests does not work.

This PR use modern API to cancel the requests. The requests are now correctly aborted, and the `openFile` method exit because the request call throws an exception.

Fix #893

https://axios-http.com/docs/cancellation
https://developer.mozilla.org/en-US/docs/Web/API/AbortController

Signed-off-by: Louis Chemineau <louis@chmn.me>
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@szaimen szaimen unpinned this issue Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0. Needs triage Pending approval or rejection. This issue is pending approval. bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants