948213 - Menu Panel stays open when clicking recently closed tabs items in the history subview

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[custom.firefox.lady [:tawn]]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20131204004002

Steps to reproduce:

1. Click Australis Menu (hamburger) button
2. Click History
3. Click one of the History links (above Restore all Tabs)
4. Note that the History menu stays open
5. Click another History link (above Restore all Tabs)


Actual results:

Step 3 works fine. Step 5 either has no effect or opens incorrect history entry.


Expected results:

The proper link should always be opened. Either the menu should close after step 3 or the menu needs to be live updated so history items are associated with the correct url.

Comment 1

[Sudheesh Singanamalla (:ShellHacker)]

Attachment: Drop down pushed up.png

Comment 2

[Sudheesh Singanamalla (:ShellHacker)]

Links in ,History seems to be working fine on Nightly 28.0a1 (2013-12-08)

Confirmed : "History drop down menu doesn't close after clicking on a link"

New bug noticed : The menu drop down is pushed upwards as soon as `History` tab is clicked.
(See attachment)

Comment 3

[:Gijs (he/him)]

(In reply to Sudheesh Singanamalla (:ShellHacker) from comment #2)
> New bug noticed : The menu drop down is pushed upwards as soon as `History`
> tab is clicked.
> (See attachment)

This is bug 941196.

Comment 4

[custom.firefox.lady [:tawn]]

(In reply to Sudheesh Singanamalla (:ShellHacker) from comment #2)
> Links in ,History seems to be working fine on Nightly 28.0a1 (2013-12-08)

I am still seeing the 'wrong url opened' bug on 29.0a1 (2013-12-10). Can you reproduce that issue with these steps?

Create new profile
Open https://www.mozilla.org/en-US/contribute/ ('Get Involved' from 'Recently Bookmarked')
Click new tab button
Open https://www.mozilla.org/en-US/firefox/customize/ ('Customize Firefox' from 'Recently Bookmarked')
Click new tab button
Close 'Get Involved' tab
Close 'Customize Firefox' tab
Click Australis Menu (PanelUI) button
Click History
Click 'Mozilla Firefox Web Browser-Customize' from the recently closed tabs area (it opens in a tab)
Note that the History menu stays open
Click 'Get Involved' from the recently closed tabs area (nothing happens; menu still open)
Click 'Customize' from the recently closed tabs area ('Get Involved' opens)

Regardless, fixing the "Menu Panel stays open..." (as the title of this bug was changed to) issue, should solve the problem.

Comment 5

[Jim Jeffery not reading bug-mail 1/2/11]

Confirming, setting to NEW  I see the same behavior on win7 x64 with the latest hourly m-c win32 build Nightly

Comment 7

[alex_mayorga]

Coming from bug 948687, I'd like the list to be live and not close specially for the "recently closed tabs" section as in my use case I cherry pick from that list after a tab closing rampage.

If the menu closed that would meant 3x clicks for my use case.

This should tie into bug 928843 too, right?

Comment 8

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

Sorry for the summary change. In fact we should be closing the menu panel here to match the same behavior as the Menu Bar's History Recently-Closed-Tabs list.

Comment 10

[alex_mayorga]

Judging by the new duplicate, seems to me like the consensus is that the list should update itself instead of close itself.

Could this be reconsidered, please?

Comment 11

[obrufau]

Yes, I think it would be very useful to update the list instead of closing it.

Because of functionality: if someone would like to close it after restoring a tab, only a click outside the menu will close it. But if the list is closed automatically and someone wants to restore some tabs, he will get mad.

And because of coherence: the Developer list isn't closed neither.

Comment 12

[:Gijs (he/him)]

(In reply to obrufau from comment #11)
> Yes, I think it would be very useful to update the list instead of closing
> it.
> 
> Because of functionality: if someone would like to close it after restoring
> a tab, only a click outside the menu will close it. But if the list is
> closed automatically and someone wants to restore some tabs, he will get mad.

This is only helpful if you want to restore more than one (but not all) tab(s). I posit that that is not the most common case.

> And because of coherence: the Developer list isn't closed neither.

That's a bug - but I can't reproduce it at all. Can you file a separate bug? Which developer item are you clicking that doesn't close the menu?

Comment 13

[obrufau]

I don't think that's a bug neither :)

By the way, clicking the following commands doesn't close the Developer list: Toggle Tools, Web Console, Inspector, Debugger, Style Editor, Profiler, Network, Developer Toolbar, App Manager, Responsive Design View, Get More Tools, Work Offline.

Only these close it: Browser Console, Scratchpad, Page Source. And it makes sense, because they open a new window, so the old one loses focus and then the menu is closed.

Note: If the developer tools (Toggle Tools, Web Console, Inspector, Debugger, Style Editor, Profiler, Network) were not docked in the window, they will open in a new window. Therefore, the Developer menu will close too.

But I think that this current behavior is very coherent and intuitive, and it shouldn't change. Only the malfunctions (like History not updating) should be fixed.

Comment 14

[:Gijs (he/him)]

(In reply to obrufau from comment #13)
> I don't think that's a bug neither :)
> 
> By the way, clicking the following commands doesn't close the Developer
> list: Toggle Tools, Web Console, Inspector, Debugger, Style Editor,
> Profiler, Network, Developer Toolbar, App Manager, Responsive Design View,
> Get More Tools, Work Offline.

I tried 5 of these, and all of them close the panel for me (yes, even when the devtools are docked). Have you tried on a clean profile? It *is* a bug if the panel isn't closing. You activated a menu item, it'll close the menu. It doesn't make sense that the menu would let you toggle between different developer tools after opening the first - you could just do that in the toolbox. Similarly, if the menu stayed open for e.g. the bookmarks view, that'd be weird, because if you click a bookmark, presumably that's the one you want, and we open it, and at that point the menu is just in the way of things. In this respect, it's no different from traditional menus.

Comment 15

[:Gijs (he/him)]

Attachment: fix recently closed tabs not closing panel in Australis panel subview,

Comment 16

[obrufau]

You're right, with a clean profile the panel closes when clicking a developer item.

Comment 17

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

Comment on attachment 8367957 [details] [diff] [review]
fix recently closed tabs not closing panel in Australis panel subview,

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

I had been looking at this and didn't realize that the fix would be so subtle. nice.

Comment 18

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/2bff84532177

Comment 19

[:Gijs (he/him)]

So this made the history view consistent with the developer and help subviews. That's fine.

Thinking about it more, however...

addPanelCloseListeners is called for the temporary panels we create in PanelUI.js (in case the buttons are in the toolbar), the overflow panel, and the main menu panel. However, even though the panel close listeners are on the main panel, they don't seem to get events for the subviews... this is going to bite everybody making subviews (see also: some of the last comments in bug 964944). Jared, when you were looking at this, did you figure out why that handler isn't dealing with stuff for us here?

Comment 20

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

No I didn't figure out why.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2bff84532177

Comment 22

[Jim Jeffery not reading bug-mail 1/2/11]

This appears to be not fixed. 

Tested on win7 x64 m-c win32 build.
Tested on Tinderbox hourly build cset:
https://hg.mozilla.org/mozilla-central/rev/940a7d00c096
1. click on Menu/Hamburger 
2. click on History Icon
3. Pick any link in 'Recent closed tabs' list
4. Left or mid-click on link, link opens but the History Panel does not close, still being displayed. 

Am I missing something here in reference to what this bug was about ?

Comment 23

[:Gijs (he/him)]

(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #22)
> This appears to be not fixed. 
> 
> Tested on win7 x64 m-c win32 build.
> Tested on Tinderbox hourly build cset:
> https://hg.mozilla.org/mozilla-central/rev/940a7d00c096
> 1. click on Menu/Hamburger 
> 2. click on History Icon
> 3. Pick any link in 'Recent closed tabs' list
> 4. Left or mid-click on link, link opens but the History Panel does not
> close, still being displayed. 
> 
> Am I missing something here in reference to what this bug was about ?

On a clean profile? Because it works for me...

Comment 24

[Jim Jeffery not reading bug-mail 1/2/11]

@ Gijs, 

Yes, just tested on a fresh Profile with no history what-so-ever, addons, etc...

History Panel will not close when clicking a 'recent closed tab' history item.

Comment 25

[:Gijs (he/him)]

I should wear a dunce cap for like a week for committing what I did. Sigh.

'win' doesn't exist in that context. Why this worked in my testing, no idea. Either way, this probably also causes bug 966125.

Comment 26

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #25)
> I should wear a dunce cap for like a week for committing what I did. Sigh.
> 
> 'win' doesn't exist in that context. Why this worked in my testing, no idea.
> Either way, this probably also causes bug 966125.

Backed out:

remote:   https://hg.mozilla.org/integration/fx-team/rev/60bff07e1074

Comment 27

[:Gijs (he/him)]

Attachment: Australis menu panel stays open when opening items in subviews,

How about this? We really shouldn't leave subviews with an inferior closing mechanism. Add-on authors have been running into this as well.

Comment 28

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

Comment on attachment 8368723 [details] [diff] [review]
Australis menu panel stays open when opening items in subviews,

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

This looks good.

Comment 29

[:Gijs (he/him)]

Attachment: Australis menu panel stays open when opening items in subviews,

Mike, can you doublecheck this change from a customize mode transition perspective? I'm moving the ensureReady check because otherwise the close handler stuff can't find the panel anymore (because we move the panel into the panelholder). This is only important if we open customize mode without opening the panel first, otherwise this shouldn't really have any effect, I think?

Comment 30

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

Comment on attachment 8368753 [details] [diff] [review]
Australis menu panel stays open when opening items in subviews,

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

I'd say that while this might add a little bit of jank at the beginning of the transition, it's jank that would have showed up at the *end* of the transition before during the switch from customize-entering to customize-entered, so I guess it's an even trade.

I'd also say that this case (customize mode without opening the panel) is likely a rare one, so this trade is likely fine.

Comment 31

[:Gijs (he/him)]

Alright, hope it works better the second time around. :-)

remote:   https://hg.mozilla.org/integration/fx-team/rev/7ea80c5d5a1e

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7ea80c5d5a1e

Comment 33

[Andrei Vaida [:avaida]]

Issue verified fixed on the latest Aurora (Build ID: 20140310004003) and latest Nightly (Build ID: 20140310030201), using:
- Windows 7 64-bit [1],
- Mac OS X 10.9.1 [2],
- Ubuntu 13.10 64-bit [3].

Note: each of the restored tabs is opened as the 1st tab (before the currently opened tab), I'm not sure if this is the intended behavior, please do confirm.

1. Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0
2. Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:29.0) Gecko/20100101 Firefox/29.0
3. Mozilla/5.0 (X11; Linux x86_64; rv:29.0) Gecko/20100101 Firefox/29.0