586212 - Don't carry out the command when clicking on a disabled splitmenu

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[Christian Ascheberg]

User-Agent:       Mozilla/5.0 (Windows NT 6.0; rv:2.0b4pre) Gecko/20100810 Minefield/4.0b4pre
Build Identifier: Mozilla/5.0 (Windows NT 6.0; rv:2.0b4pre) Gecko/20100810 Minefield/4.0b4pre

In javascript popups, "New Tab" in the root menu is not disabled, while in the
submenu it is disabled.
Clicking "New Tab" shows a blank page, but you can not switch tabs. You will have to close the popup.
(It is only possible to actually open a new tab, if browser.tabs.closeWindowWithLastTab is true [default])

Reproducible: Always

Steps to Reproduce:
1. Open website above (for example)
2. Click "Webradio" to open popup
3. Click the Firefox button
4. "New Tab" in the root menu is enabled, in the submenu it is disabled.
Actual Results:  
If you click it, a blank tab opens (if browser.tabs.closeWindowWithLastTab is true)
The tab is not displayed, you can not switch tabs, can not change URL.

Expected Results:  
"New Tab" in the menu root should be disabled.
Or maybe it should say "New Window".

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Is this a regression from the checkin of bug 571782?

Comment 2

[u88484]

(In reply to comment #1)
> Is this a regression from the checkin of bug 571782?
No, in a sense this is new (not a regression in a working feature per se) since before bug 571782 landed "New Tab" wasn't available in the Firefox button.  Also, a tabbar/menu bar has never (or hasn't in forever) been shown in a popup window so this case has just never been brought to light.  I couldn't get keyboard shortcut CTRL+T to work so I'm assuming this case might have been taken care of before so maybe bug 571782 just didn't account for this.

Comment 3

[u88484]

Sorry, meant to CC Dao on my last comment.  Dao, what do you think should happen here?

Comment 4

[Christian Ascheberg]

In Firefox 3.6.8 the given webpage is showing a menu bar in the popup, and it is also possible to open new tabs in it, the tabbar will be displayed then.
So I think it is a rather new behaviour to disallow opening new tabs in popups.

Comment 5

[Johnathan Nightingale [:johnath]]

Doesn't block, but I'd love to see a patch that made things consistent, here.

Comment 6

[Christian Ascheberg]

(In reply to comment #5)
> Doesn't block, but I'd love to see a patch that made things consistent, here.

In the current build, both menu items are enabled in popups now. (I do not know what caused this.) So it is consistent, though it does not make much sense this way, as it is not possible to open new tabs in popups.

Comment 7

[Christian Ascheberg]

(In reply to comment #6)
> In the current build, both menu items are enabled in popups now.

I was wrong, sorry, this is only the case when browser.tabs.closeWindowWithLastTab = false (see bug 577382). Though, in that case, it is not actually possible to open tabs (see description).

Comment 8

[Christian Ascheberg]

I have tested this again using the current latest-trunk version, and some things have changed. This bug is now independent from browser.tabs.closeWindowWithLastTab pref.

To test this, the popup must have the attributes menubar=1 and toolbar=0.
In that popup, the minefield button will be displayed. Clicking it will show the root menu with the item "New Tab" greyed out. The belonging submenu will not be shown on hover.

Clicking the greyed out "New Tab" item does still open a new tab in the popup though! These errors occur:

Error: newBrowser is undefined
Source File: chrome://browser/content/tabbrowser.xml
Line: 867

Error: content is null
Source File: chrome://browser/content/browser.js
Line: 5515

Trying to close the tab (seen as white content only) using CTRL+W shows this alert:

ASSERT: Giving up waiting for the tab closing animation to finish (bug 608589)
Stack Trace: 
0:([object XULElement],[object XULElement],-5)

and this error:

Error: this.selectedTab is null
Source File: chrome://browser/content/tabbrowser.xml
Line: 1679

Then closing the popup:

Error: aWindow.content is null
Source File: resource://gre/components/nsSessionStore.js
Line: 845

Comment 9

[Christian Ascheberg]

Attachment: Testcase

Comment 10

[Dão Gottwald [:dao]]

Attachment: patch

fixes the remaining issue from comment 8

Comment 11

[Dão Gottwald [:dao]]

morphing

Comment 12

[Justin Dolske [:Dolske]]

Comment on attachment 512267 [details] [diff] [review]
patch

Maybe worth a 1-liner comment?

Also, we should disable the hover effect for the menu arrow part of the splitmenu when it's disabled... Followup?

Comment 13

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/39982a2ba344

Comment 14

[Dão Gottwald [:dao]]

(In reply to comment #12)
> Also, we should disable the hover effect for the menu arrow part of the
> splitmenu when it's disabled... Followup?

It's actually the other way around, the hover effect is missing for the menuitem part. Filed bug 638431.

Comment 15

[Teodosia Pop]

Verified using 
Mozilla/5.0 (Windows NT 6.1; rv:2.0) Gecko/20100101 Firefox/4.0 
and 
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0) Gecko/20100101 Firefox/4.0.