Fix popover menu not closing in user settings #7668
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A popover menu is displayed when its element has the "open" CSS class. That class is added when clicking on the menu toggle, but it was removed only when clicking again on the toggle, so the popover menu in user settings could be closed only by clicking again on the menu toggle. Now the "open" CSS class is removed too when clicking on the body, either directly or through event propagation. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
danxuliu
added
3. to review
"click" events are handled by several elements in user settings, and some of them (like the edit icon in the user name) stop the propagation of the event. Due to this the event never reaches the document and thus the menu was not closed in those cases. "click" events are always preceded by "mouseup" events (as "click" events are generated when "mousedown" and "mouseup" events occur in the same element), so now the menu is closed when a "mouseup" is received in the document. The described problem would happen too if an element stopped the propagation of the "mouseup" event; currently no element does that in the user settings, so now the menu is always closed as expected. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
danxuliu
added
2. developing
|
I have realized that listening to Please do not review yet, and sorry for the noise :-) |
Codecov Report
@@ Coverage Diff @@
## master #7668 +/- ##
===========================================
+ Coverage 51.19% 51.2% +<.01%
- Complexity 24888 24893 +5
===========================================
Files 1601 1602 +1
Lines 94775 94803 +28
Branches 1371 1372 +1
===========================================
+ Hits 48522 48543 +21
- Misses 46253 46260 +7
|
danxuliu
added
3. to review
|
Done; this pull request is ready for review! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes (partially; the position part is not covered here) #7617
This pull request fixes a regression introduced in commit 7fb3292
Before that commit the popover menu in user settings was shown or not based on whether the popover menu element was descendant of an active row or not. The
activeCSS class was added to the row when clicking on the menu toggle and removed when clicking again on the menu toggle or on the body, either directly or through event propagation.The aforementioned commit unified its behaviour with the guidelines, which is showing it when the popover menu element has the
openCSS class. However, theopenCSS class was removed only when clicking again on the menu toggle and not when clicking on the body, so once open it was not closed until the menu toggle was clicked again. This is fixed by removing theopenCSS class too when clicking on the body.Note that this pull request restores the same technique used to close the menu when it was based on the active row, and that technique has the same problems described in #7613 (for example, if clicking on the button to edit the user name the popover menu is not closed, because the event propagation is stopped and it never reaches the body element).This pull request restores the same technique used to close the menu when it was based on the active row, although now
mouseupis used instead ofclick(like for other menus in the server). Although the technique has the same problems described in #7613 the propagation ofmouseupevents is not stopped in the user settings elements, so the problems in this case are just theoretical and do not appear in practice.