Clicking on Search input in menu panel causes panel to close

RESOLVED FIXED in Firefox 28

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mconley, Assigned: Gijs)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 28
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:M?][Australis:P1])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
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.

Updated

5 years ago
Duplicate of this bug: 880071
(Assignee)

Updated

5 years ago
Assignee: nobody → jaws
(Assignee)

Comment 2

5 years ago
Stealing this.
Assignee: jaws → gijskruitbosch+bugs
(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!
(Assignee)

Comment 4

5 years ago
Created attachment 762418 [details] [diff] [review]
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?
Attachment #762418 - Flags: review?(jaws)
(Assignee)

Comment 5

5 years ago
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.
(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 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.
(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.
(Assignee)

Comment 9

5 years ago
Comment on attachment 762418 [details] [diff] [review]
Hacky Patch

Need to look into this, more clearing review request for now.
Attachment #762418 - Flags: review?(jaws)
(Reporter)

Comment 10

5 years ago
These bugs didn't make it into the UR Build that went out in bug 879846. Clearing the [User Research Build+] flag.
Whiteboard: [Australis:M7][User Research Build+] → [Australis:M7]
(Reporter)

Comment 11

5 years ago
Removing the items from M7 that do not block us from landing on m-c.
Whiteboard: [Australis:M7] → [Australis:M?]
(Assignee)

Comment 12

5 years ago
Created attachment 765288 [details] [diff] [review]
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...
Attachment #762418 - Attachment is obsolete: true
Attachment #765288 - Flags: review?(jaws)
(Assignee)

Comment 13

5 years ago
Comment on attachment 765288 [details] [diff] [review]
Patch

And then I noticed this needs work for subview items. Oops. :-\
Attachment #765288 - Attachment is obsolete: true
Attachment #765288 - Flags: review?(jaws)
(Assignee)

Comment 14

5 years ago
Created attachment 765312 [details] [diff] [review]
Patch
Attachment #765312 - Flags: review?(jaws)
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?
Attachment #765312 - Flags: review?(jaws) → feedback+
P1 since search in popup UI is a pretty common convention that I expect people/addons will want to be able to use.
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P1]
(Assignee)

Comment 17

5 years ago
Created attachment 766841 [details] [diff] [review]
Patch v1.1

Good points indeed. Fixed to use handleEvent and fixed that newline.
Attachment #765312 - Attachment is obsolete: true
Attachment #766841 - Flags: review?(jaws)
Comment on attachment 766841 [details] [diff] [review]
Patch v1.1

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

LGTM!
Attachment #766841 - Flags: review?(jaws) → review+
(Assignee)

Comment 19

5 years ago
Pushed: https://hg.mozilla.org/projects/ux/rev/1e1547dbba7e
Status: NEW → ASSIGNED
Whiteboard: [Australis:M?][Australis:P1] → [Australis:M?][Australis:P1][fixed-in-ux]
(Assignee)

Updated

5 years ago
Duplicate of this bug: 878988

Comment 21

5 years ago
WFM in UX 25.0a1 (2013-08-02). Awesome!
(Assignee)

Comment 22

5 years ago
https://hg.mozilla.org/mozilla-central/rev/1e1547dbba7e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M?][Australis:P1][fixed-in-ux] → [Australis:M?][Australis:P1]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.