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

Clicking on the contents may not close the menu in the sharing page #7613

Closed
danxuliu opened this issue Dec 22, 2017 · 7 comments
Closed

Clicking on the contents may not close the menu in the sharing page #7613

danxuliu opened this issue Dec 22, 2017 · 7 comments
Labels
0. Needs triage Pending check for reproducibility or if it fits our roadmap bug design Design, UI, UX, etc. feature: sharing help wanted needs info

Comments

@danxuliu
Copy link
Member

danxuliu commented Dec 22, 2017

In the sharing page (go to the Files app, open the right sidebar for a file, open the Sharing tab, click on Share link and open that link in the browser) the share menu is open when clicking on the toggle at the right of the header, and once open it is closed when clicking anywhere outside the menu (in current master clicking again on the toggle does not close the menu, but there is a fix for that in #7598).

That is the theory, though. In practice, depending on the app contents, the menu may or may not be closed when clicking on them. The reason is that the menu is closed when a mouseup event is received by the document and that event originated on an element which is not the share menu. However, if the propagation of the mouseup event was prevented in any descendant element it would not bubble all the way up to the document, the handler would not be executed and the menu would not be closed. This happens, for example, when using the PDF Viewer app and opening a sharing page for a PDF file.

This could be solved by using a different technique to close the menu: when the menu is open a transparent div covering the whole page is created, and when a click is received on that div it is removed and the menu is closed. The z-index of the div should be higher than any other element in the page, except for the share menu (and the toggle, to support the changes from #7598).

There is a problem in that approach, though. Currently when clicking outside the share menu the click is handled by the element visually receiving it (and its ancestors), and this will also close the menu. However, if a div is covering the whole page then the div (and its ancestors, but that is not very relevant) will handle the click, but not the element visually receiving it. The result is that the menu will be closed, but nothing will happen with the element that the user thinks she is clicking on.

It would be possible to solve that too by generating a new click event and sending it to the element that should have been clicked (by using something like elementFromPoint once the div is removed)... but it feels like a hack more than a solution, and to be honest I do not know if that would work in every situation.

If there is a proper solution to that problem I am afraid that I do not know it. Ideas (and, better yet, patches ;-) ) welcome :-)

So, when the share menu is open, which one is the best option?:

  • Always handle clicks on the app contents, but depending on what is shown in them the menu is closed or not (current behaviour)
  • Never handle clicks on the app contents, but always close the menu (to be implemented)
  • Always handle clicks on the app contents, and always close the menu (to be implemented; hacky, may give headaches)
  • Always handle clicks on the app contents, and always close the menu (to be implemented; nice, clean and robust... and I do not know how to do it :-) )

@nextcloud/designers

@danxuliu danxuliu added this to the Nextcloud 13 milestone Dec 22, 2017
@pixelipo
Copy link
Contributor

I'd just like to note that a very recent bug report to the Deck app was to restore (or add an optionto enable) right sidebar that doesn't close when losing focus...

@danxuliu
Copy link
Member Author

danxuliu commented Jan 3, 2018

The issue applies too to the contacts and settings menu, as they use the same technique of listening to the mouseup event; they can not be closed, for example, when clicking on a PDF preview in the details view of the Files app.

I'd just like to note that a very recent bug report to the Deck app was to restore (or add an optionto enable) right sidebar that doesn't close when losing focus...

I agree that a sidebar should not be closed when losing focus, but this issue is about menus, not sidebars; unrelated behaviours in my opinion ;-)

@rullzer rullzer removed this from the Nextcloud 13 milestone Jan 11, 2018
@nextcloud-bot nextcloud-bot added the stale Ticket or PR with no recent activity label Jun 20, 2018
@skjnldsv skjnldsv added the 1. to develop Accepted and waiting to be taken care of label Jun 6, 2019
@stale stale bot removed the stale Ticket or PR with no recent activity label Jun 6, 2019
@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 1. to develop Accepted and waiting to be taken care of labels Jul 12, 2019
@skjnldsv
Copy link
Member

Fixed with #15719

@danxuliu
Copy link
Member Author

This it not fixed with #15719, because this issue is about the menu in the header of the public share page, not the menu in the Sharing tab of the Files app sidebar.

@danxuliu danxuliu reopened this Oct 29, 2019
@skjnldsv
Copy link
Member

Then I did not understood the issue :)
Could you clarify a bit? I think the wall of text you wrote is preventing any designer from daring to look at this one 😁

@jancborchardt
Copy link
Member

☝️ also, screenshots make things easy to understand. :)

@skjnldsv skjnldsv added 1. to develop Accepted and waiting to be taken care of and removed 3. to review Waiting for reviews labels Nov 4, 2019
@szaimen
Copy link
Contributor

szaimen commented Jan 9, 2023

Hi, please update to 24.0.8 or better 25.0.2 and report back if it fixes the issue. Thank you!

@szaimen szaimen added needs info and removed 1. to develop Accepted and waiting to be taken care of labels Jan 9, 2023
@szaimen szaimen added the 0. Needs triage Pending check for reproducibility or if it fits our roadmap label Jan 9, 2023
@szaimen szaimen closed this as completed Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0. Needs triage Pending check for reproducibility or if it fits our roadmap bug design Design, UI, UX, etc. feature: sharing help wanted needs info
Projects
None yet
Development

No branches or pull requests

7 participants