879985 - Clicking on Search input in menu panel causes panel to close

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

Currently, the search input doesn't really work in the menu panel. I can drag it in there, but if I click on it to enter some text, the panel closes.

Comment 2

[:Gijs (he/him)]

Stealing this.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gijs Kruitbosch from comment #2)
> Stealing this.

Gijs, I'm pretty sure this is because the .currentTarget doesn't have `noautoclose`. Hope that helps!

Comment 4

[:Gijs (he/him)]

Attachment: Hacky Patch

(In reply to Jared Wein [:jaws] from comment #3)
> (In reply to :Gijs Kruitbosch from comment #2)
> > Stealing this.
> 
> Gijs, I'm pretty sure this is because the .currentTarget doesn't have
> `noautoclose`. Hope that helps!

That's only so in the overflow panel. It also doesn't work in the normal menupanel, which doesn't check currentTarget. I poked at this a bunch and found some inconsistencies. The modifications to CustomizableUI don't make it perfect, but it's a little better. I only decided to remove the checks for the provider later on. Without those, the overflow panel could use identical checks to the menu panel (id est, only apply these handlers to toolbar buttons, but also do so if they're not a/the direct child of the overflow panel / toolbar).

Unfortunately, with these customizableUI changes, the panel doesn't close at all, not even after doing a search, which I think it should. That then turned out to be harder to fix than I thought it ought to be. Shouldn't we change the customizableui-area attribute for overflowed widgets?

I tried to make our auto-close stuff smarter so it could deal with these cases, by only caring about clicks on buttons, but events get eaten so we never get the enter keypress, which means that didn't help. Still, not happy about having to hack the search widget to behave properly in the menupanel, so if you have a better idea, we can give it a shot. Maybe capturing event listeners?

Comment 5

[:Gijs (he/him)]

Comment on attachment 762418 [details] [diff] [review]
Hacky Patch

>-                   flex="100" persist="width" removable="true" noautoclose="true">
>+                   flex="100" persist="width" removable="true">

And this is why I shouldn't write patches at 2.30am. Ignore this hunk, please, it shouldn't have made it into this attachment.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gijs Kruitbosch from comment #4)
> I tried to make our auto-close stuff smarter so it could deal with these
> cases, by only caring about clicks on buttons, but events get eaten so we
> never get the enter keypress, which means that didn't help. Still, not happy
> about having to hack the search widget to behave properly in the menupanel,
> so if you have a better idea, we can give it a shot. Maybe capturing event
> listeners?

Can you do a capturing event listener that sets up a setTimeout which checks to see if the event was cancelled? If the event isn't cancelled, then we can hide the panel.

We do something similar for handling clicks on the video controls to toggle playback. See https://hg.mozilla.org/mozilla-central/rev/3f4c460bb95e.

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 762418 [details] [diff] [review]
Hacky Patch

>+            while (panelNode && panelNode.localName != "panel")
>+              panelNode = panelNode.parentNode;
>+            if (panelNode && panelNode.hidePopup) {
>+              panelNode.hidePopup();
>+            }

inconsistent brace style between while and if. prevailing style seems to be to omit the braces.

Comment 8

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] from comment #6)
> (In reply to :Gijs Kruitbosch from comment #4)
> > I tried to make our auto-close stuff smarter so it could deal with these
> > cases, by only caring about clicks on buttons, but events get eaten so we
> > never get the enter keypress, which means that didn't help. Still, not happy
> > about having to hack the search widget to behave properly in the menupanel,
> > so if you have a better idea, we can give it a shot. Maybe capturing event
> > listeners?
> 
> Can you do a capturing event listener that sets up a setTimeout which checks
> to see if the event was cancelled? If the event isn't cancelled, then we can
> hide the panel.

This sounds like a use case for nsIEventListenerService::addSystemEventListener.

Comment 9

[:Gijs (he/him)]

Comment on attachment 762418 [details] [diff] [review]
Hacky Patch

Need to look into this, more clearing review request for now.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

These bugs didn't make it into the UR Build that went out in bug 879846. Clearing the [User Research Build+] flag.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Removing the items from M7 that do not block us from landing on m-c.

Comment 12

[:Gijs (he/him)]

Attachment: Patch

Dao, thanks a lot for the system event listener hint! That's made our "make buttons close the panel" code a *lot* cleaner.

Unfortunately, to deal with the search box I still has logic on top of just defaultPrevented. In my testing, this now works for the search engine dropdown, autocomplete, and clicking the little search icon. I also hope it's general enough to essentially work for other text-area or menupopup based widgets that people would put in there.

A separate bug I noticed is that it seems like opening the customization dialog loses the search input. This is not caused by my fix and rather by XBL bindings being reinitialized, but we probably should be fixing that. Might be best to fix it by fixing bug 248955 / bug 565740 ? Although we could probably workaround by having the search box keep an attribute around with its value or something like that...

Comment 13

[:Gijs (he/him)]

Comment on attachment 765288 [details] [diff] [review]
Patch

And then I noticed this needs work for subview items. Oops. :-\

Comment 14

[:Gijs (he/him)]

Attachment: Patch

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 765312 [details] [diff] [review]
Patch

Review of attachment 765312 [details] [diff] [review]:
-----------------------------------------------------------------

Overall looks good, but I'm curious about why we can't use handleEvent like I've noted below.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +899,5 @@
> +  },
> +
> +  hidePanelForNode: function(aNode) {
> +    let panel = aNode;
> +    while (panel && panel.localName != "panel") panel = panel.parentNode;

nit, place |panel = panel.parentNode;| on its own line.

@@ +919,5 @@
> +    } else { // mouse events:
> +      if (aEvent.defaultPrevented || aEvent.button != 0) {
> +        return;
> +      }
> +      // Can't use |this| because we're an event listener...

Why can't we use handleEvent?

Comment 16

[Justin Dolske [:Dolske]]

P1 since search in popup UI is a pretty common convention that I expect people/addons will want to be able to use.

Comment 17

[:Gijs (he/him)]

Attachment: Patch v1.1

Good points indeed. Fixed to use handleEvent and fixed that newline.

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 766841 [details] [diff] [review]
Patch v1.1

Review of attachment 766841 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM!

Comment 19

[:Gijs (he/him)]

Pushed: https://hg.mozilla.org/projects/ux/rev/1e1547dbba7e

Comment 21

[Florian Bender]

WFM in UX 25.0a1 (2013-08-02). Awesome!

Comment 22

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/1e1547dbba7e