Closed Bug 1473209 Opened 2 years ago Closed 2 years ago

Make clicking the meatball menu icon close the menu if it is already opened

Categories

(DevTools :: General, defect, P1)

defect

Tracking

(firefox63 fixed)

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: birtles, Assigned: mantaroh)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

See bug 1461522 where I made it do this to match the behavior of the Hamburger menu and Bookmarks sidebar menu. It turns out that behavior only exists on Linux and probably shouldn't behave like that anyway.
Priority: -- → P1
Duplicate of this bug: 1476528
This is the same as Bug 1457066.

On Mac/Linux, We need to specify the 'consumeoutsideclicks' of XULPanel to true in this case like the menu.js[1]. We specify this value to false in the HTMLTooltips.js at the moment, so the onclick event of the meatball button will be fired again on macOS and Linux.[2]

[1] https://searchfox.org/mozilla-central/rev/ad36eff63e208b37bc9441b91b7cea7291d82890/devtools/client/framework/menu.js#101 
[2] https://searchfox.org/mozilla-central/rev/ad36eff63e208b37bc9441b91b7cea7291d82890/devtools/client/shared/components/menu/MenuButton.js#173
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #2)
> This is the same as Bug 1457066.
> 
> On Mac/Linux, We need to specify the 'consumeoutsideclicks' of XULPanel to
> true in this case like the menu.js[1]. We specify this value to false in the
> HTMLTooltips.js at the moment, so the onclick event of the meatball button
> will be fired again on macOS and Linux.[2]
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> ad36eff63e208b37bc9441b91b7cea7291d82890/devtools/client/framework/menu.
> js#101 
> [2]
> https://searchfox.org/mozilla-central/rev/
> ad36eff63e208b37bc9441b91b7cea7291d82890/devtools/client/shared/components/
> menu/MenuButton.js#173

If we don't use the consumeoutsideclicks of XUL Panel, we can't distinguish that the current click event is related to hiding the meatball menu popup or not since popup menu has closed already when receiving the click event of meatball button.

As far as I can see, the photonized app menu button waits for the transitionend of own animation, then clear the 'pointer-events' property after finishing the animation.[1] We can add the same fade in/out transition and observe the transition end. (Perhaps, the bug 1474757 is related to this bug?)

[1] https://searchfox.org/mozilla-central/rev/d160ac574a5798010eda4e33e91ee628aa268a33/browser/components/places/content/menu.xml#584-623

If we need to prevent this phenomenon as soon as possible, we can add the time which disabling the meatball button. (e.g., add the 100ms timer, then we prevent to fire the click event of this button until this timer has finished) However, this way is temporary-fix.
Some other possibilities:

* Try to annotate the event itself (the HTMLTooltip receives the event during the capture phase, but then its the same event we'll end up getting on the button itself) to indicate that this same event closed the popup so we can ignore it in the button.
* Get HTMLTooltip to call a callback when it does autoclose when we then route to the MenuButton to say, "Ignore the next click at these coordinates" (or some other information to identify the event).
* Pass the button dimension / elements to HTMLTooltip and get it to include it its _isInTooltipContainer calculation.

I'd probably try the first option first since it will be cleanest if it works. However, I don't know if it's possible to annotate these events and have those expando properties preserved through event propagation.
(In reply to Brian Birtles (:birtles) from comment #4)
> Some other possibilities:
> 
> * Try to annotate the event itself (the HTMLTooltip receives the event
> during the capture phase, but then its the same event we'll end up getting
> on the button itself) to indicate that this same event closed the popup so
> we can ignore it in the button.
> * Get HTMLTooltip to call a callback when it does autoclose when we then
> route to the MenuButton to say, "Ignore the next click at these coordinates"
> (or some other information to identify the event).
> * Pass the button dimension / elements to HTMLTooltip and get it to include
> it its _isInTooltipContainer calculation.
> 
> I'd probably try the first option first since it will be cleanest if it
> works. However, I don't know if it's possible to annotate these events and
> have those expando properties preserved through event propagation.

Thanks brian,

I'll try to the way annotating to the event.
(One other thought that occurred to me that might work if the others don't, is to get the click for the MenuButton in the capture phase and then either stop it and handle the closing ourselves, or, probably better, record if we were open so we can ignore the event when we get the event in the bubbling phase. In React I believe you can use onClickCapture to bind an event handler to the capture phase.)
Actually, HTMLTooltip will not call the HTMLTooltip.hide() when click the meatball button. Generally, XUL popup menu will hide when clicking outside of popup. However, if we specify noautohide to the panel, the popup will not hide automatically. (This autohide attribute is removed on Bug 1310957.)

So we can't distinguish that target click event is related to popup close or not, because, the popup menu has closed automatically when receiving the click button event.

I think that we can choose the following two pattern:

1) listen to click event after XUL popup menu is hidden automatically, and annotate that popup is closed automatically to this event.

In this case, we need to consider to switch window. If a user changed the active window, XUL popup menu will be hidden automatically. As a result of it, a user can't display popup menu due to this event handler even if click the meatball button. So we might need to remove this event handler in this case (like using the timer.)

WIP: https://hg.mozilla.org/try/rev/7a56e8ad17fe5e16ebaf16f6677843b9e43d9b94

2) Use autohide attribute to tooltip panel.

We can use the 'autohide' attribute to this panel. As mentioned by Julian on bug 1310957, XUL panel will leave even if we changed the window. In this case, we should listen to blur of the window and hide the popup if the window lost the focus.
In this case, we don't change the current onClick event handler of MenuButton and HTMLTooltips.

WIP: https://hg.mozilla.org/try/rev/542c842468571f21d33065bf8d403c2b6a5e4e20
Assignee: nobody → mantaroh
Status: NEW → ASSIGNED
Comment on attachment 8995381 [details]
Bug 1473209 - Ignore the click event until popup is hidden. .

https://reviewboard.mozilla.org/r/259844/#review266878

This works great. Thank you!

::: commit-message-54c01:13
(Diff revision 1)
> +is false.)
> +
> +As a result of it, the popup menu will be reopened. In order to prevent this,
> +this patch will disable the MenuButton until popup is hidden by using
> +the popuphidden event. This popuphidden event will be fired before the click
> +event of MenuButton, then the click event of MenuButton will be fire after

Nit: s/be fire/be fired/

::: commit-message-54c01:14
(Diff revision 1)
> +popuphidden immediately. So the term of disabling the MenuButton should wait
> +for next click event, this patch will wait for this event by using the timer.
> +(time is zero.)

Let's drop this last sentence (beginning, "So the term...") since it is explained in the onHidden comment.

::: devtools/client/framework/test/browser_toolbox_options_panel_toggle.js:47
(Diff revision 1)
>    await onReady;
>  }
>  
>  async function clickSettingsMenu(toolbox) {
>    const onPopupShown = () => {
> -    toolbox.doc.removeEventListener("popuphidden", onPopupShown);
> +    toolbox.doc.removeEventListener("popupshown", onPopupShown);

Nice find.

::: devtools/client/shared/components/menu/MenuButton.js:171
(Diff revision 1)
> +    // Enable the button when XUL popup is hidden.
> +    // If the button is clicked when the popup is displayed, the popup menu will
> +    // be closed and notify popuphidden event, then the widget will deal with
> +    // a mouse event. So we need to skip this button event in order to prevent
> +    // to reopen the popup menu

Let's add a space before this comment.

Then let me reword it so that it's a little easier to follow:

    // While the menu is open, if we click _anywhere_ outside the menu, it will
    // automatically close. This is performed by the XUL wrapper before we get
    // any chance to see any event. To avoid immediately re-opening the menu
    // when we process the subsequent click event on this button, we set
    // 'pointer-events: none' on the button while the menu is open.
    //
    // After the menu is closed we need to remove the pointer-events style (so
    // the button works again) but we don't want to do it immediately since the
    // "popuphidden" event which triggers this callback might be dispatched
    // before the "click" event that we want to ignore.  As a result, we queue
    // up a task using setTimeout() to run after the "click" event.

::: devtools/client/shared/components/menu/MenuButton.js:176
(Diff revision 1)
> +    // Enable the button when XUL popup is hidden.
> +    // If the button is clicked when the popup is displayed, the popup menu will
> +    // be closed and notify popuphidden event, then the widget will deal with
> +    // a mouse event. So we need to skip this button event in order to prevent
> +    // to reopen the popup menu
> +    if (!Services.prefs.getBoolPref("ui.popup.disable_autohide", false)) {

I'm not sure if we actually need this pref check here.

The menu will behave a bit funny if you do:

1. Enable the disable_autohide pref
2. Open a menu
3. Disable the disable_autohide pref
4. Close the menu

But that will happen whether or not we have this pref check so I think we can probably remove it.

::: devtools/client/shared/components/menu/MenuButton.js:193
(Diff revision 1)
> +        // If the popup menu will be shown, disable this button in order to
> +        // prevent reopening the popup menu.

This comment is great. I would add one extra sentence like so:

        // If the popup menu will be shown, disable this button in order to
        // prevent reopening the popup menu. See extended comment in onHidden().
        // above.
        //
        // Also, we should _not_ set 'pointer-events: none' if
        // ui.popup.disable_autohide pref is in effect since, in that case,
        // there's no redundant hiding behavior and we actually want clicking
        // the button to close the menu.

::: devtools/client/shared/components/menu/MenuButton.js:195
(Diff revision 1)
> +        if (!Services.prefs.getBoolPref("ui.popup.disable_autohide", false) &&
> +            !this.state.expanded) {

Let's switch the order of this two conditions to check this.state.expanded first since it is cheaper and doing so will match the comment better.
Attachment #8995381 - Flags: review?(bbirtles) → review+
Comment on attachment 8995381 [details]
Bug 1473209 - Ignore the click event until popup is hidden. .

https://reviewboard.mozilla.org/r/259844/#review266878

Thank you for review!

> I'm not sure if we actually need this pref check here.
> 
> The menu will behave a bit funny if you do:
> 
> 1. Enable the disable_autohide pref
> 2. Open a menu
> 3. Disable the disable_autohide pref
> 4. Close the menu
> 
> But that will happen whether or not we have this pref check so I think we can probably remove it.

Ah,, yes!
We should remove this preference check.
Thanks for working on this Mantaroh! For information, we have similar issues with our "event details" tooltip, and I think I will try to apply the same strategy as what you did here and see if it helps :)
Yeah, Mantaroh and I discussed several different possible approaches for this.[1] Apparently the pointer-events approach is used elsewhere in Firefox but there removing the style is keyed off transitionend which seems a bit flaky.

[1] e.g. Can we annotate the event in the capture phase somehow and ignore it? No, it turns out the menu is hidden before then so we can't tell if it was open or not. Can we just set state when opening the window and ignore the next click on the button? No, the window can be hidden by switching windows, not just by clicking. Can we catch mousedown first and set state? No, it turns out the auto-hiding happens before even mousedown. Can we just set consumeoutsideclicks? No, otherwise you need to click twice to close the menu then select a Tab. etc. etc.
See Also: → 1349416
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a1c1b1637015
Ignore the click event until popup is hidden. r=birtles.
https://hg.mozilla.org/mozilla-central/rev/a1c1b1637015
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.