669207 - right click in the empty part of the tabbar doesn't produce menu

Status: RESOLVED FIXED

(SeaMonkey :: Tabbed Browser, defect)

Description

[Petko Alov]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:2.0.1) Gecko/20110608 Firefox/4.0.1 SeaMonkey/2.1
Build ID: 20110608031730

Steps to reproduce:

Right click on the empty part of the tabbar


Actual results:

Nothing


Expected results:

Pop-up menu should appear, like upon right click on an active tab (though with couple of options dimmed - close tab, reload tab) - this was the behavior in Seamonkey 2.0

Comment 1

[Philip Chee]

This is an intentional change as part of Bug 484968 (Make SeaMonkey tab bar scrollable to cope with tab overflow) See the last paragraph of Bug 484968 Comment 36.

Besides we have plans to make the tabbar a customizable toolbar, in which case we would certainly want the customize context menu rather than the tab context menu to appear.

I think this bug is INVALID => WORKING as designed. Unless you have a convincing use case for us.

Comment 2

[Philip Chee]

The relevant code is here:
http://hg.mozilla.org/comm-central/annotate/ae84f8b74c90/suite/browser/tabbrowser.xml#l970

Comment 3

[Petko Alov]

Well, no convincing case, except convenience of users who not feel like to open zillions of tabs :o)) - especially if a special 'tabbar' menu will develop soon(er or later).

Comment 5

[Philip Chee]

Re-Opening. I asked in the SeaMonkey user newsgroup and everyone who responded is favourable for bringing back the context menu for the empty part of the tabbar.

Comment 7

[Philip Chee]

Now that I've looked at it, it appears more complicated than I originally thought.

When I click on the empty part of the tab-bar the document.popupNode is the "tabs" element which includes the new-tab button, the close-tab button, and (if visible) the scroll arrows.

Comment 8

[Philip Chee]

Now that I've looked at it, it appears more complicated than I originally thought.

When I click on the empty part of the tab-bar the document.popupNode is the "tabs" element which includes the new-tab button, the close-tab button, and (if visible) the scroll arrows.

Comment 9

[Philip Chee]

Attachment: Patch v1.0 Proposed fix.

I tried fiddling with mousethough=always/never but could never get this to work so I decided instead to use a sledge-hammer to address this issue.

Comment 10

[[e-mail]]

Comment on attachment 695761 [details] [diff] [review]
Patch v1.0 Proposed fix.

>+            if (this.mContextTab.localName == "tab") {
>+              let disabled = this.tabs.length == 1;
>+              let menuItems = aPopupMenu.getElementsByAttribute("tbattr", "tabbrowser-multiple");
>+              for (let i = 0; i < menuItems.length; i++)
>+                menuItems[i].setAttribute("disabled", disabled);
>+            }
>+            else if (this.mContextTab.localName == "spacer") {
>+              let menuItems = aPopupMenu.getElementsByAttribute("tbattr2", "tabbrowser-spacer");
>+              for (let i = 0; i < menuItems.length; i++)
>+                menuItems[i].setAttribute("disabled", true);
>+            }
>+            else
>               return false;
I think this code goes wrong for Bookmark This Group of Tabs and Reload All Tabs because it doesn't change their disabled state and they could have been left disabled from a previous right-click when there was only one tab open.
It might be better to use getElementsByAttribute("tbattr", "*") and then enable or disable menuitems depending on the value of the attribute, the number of tabs and the localName.

>+              case "contextmenu":
>+                // openPopup() does not set the document.popupNode property so
>+                // we need to do this ourselves.
>+                document.popupNode = this.mTabsRight;
>+                var tabbrowser = document.getBindingParent(this);
>+                var tabContextMenu = document.getAnonymousElementByAttribute(tabbrowser, "anonid", "tabContextMenu");
>+                tabContextMenu.openPopup(null, "", aEvent.clientX, aEvent.clientY, true);
>+                break;
Instead of this ugly and possibly incorrect code (null and "" parameters?), is it possible to prevent the context menu appearing over unwanted elements and their children by adding an empty context="" attribute on them instead?

Comment 11

[Philip Chee]

> I think this code goes wrong for Bookmark This Group of Tabs and Reload All Tabs 
> because it doesn't change their disabled state and they could have been left 
> disabled from a previous right-click when there was only one tab open.

I thought I took care of that when I removed all the disabled attributes here:

+            var menuItems = aPopupMenu.childNodes;
+            for (let i = 0; i < menuItems.length; i++)
+              menuItems[i].removeAttribute("disabled");

> It might be better to use getElementsByAttribute("tbattr", "*") and then enable 
> or disable menuitems depending on the value of the attribute, the number of tabs 
> and the localName.

I'll see if I can do this.

> Instead of this ugly and possibly incorrect code (null and "" parameters?), is 

At first I used the spacer as the anchor node however this doesn't match the
position of the menu when over a tab. In the case a tab, the popup is positioned
relative to the mouse pointer. Unfortunately openPopup() doesn't recognize the
"after_pointer" position so instead I used an unanchored popup as described in:

<https://developer.mozilla.org/en-US/docs/XUL/PopupGuide/OpenClose#Unanchored_Popups_with_openPopup>

"This way, a popup can be positioned relative to the window, rather than relative
to a specific node. For example, you might wish to open a popup at the current
mouse position when the mouse is clicked."

> it possible to prevent the context menu appearing over unwanted elements and 
> their children by adding an empty context="" attribute on them instead?

Not sure what you mean by this. Currently the right-click event is consumed by
either a <xul:tab> or the <xul:tabs> element. The event never reaches "unwanted
elements and their children"

Comment 12

[[e-mail]]

(In reply to Philip Chee from comment #11)
> > I think this code goes wrong for Bookmark This Group of Tabs and Reload All Tabs 
> > because it doesn't change their disabled state and they could have been left 
> > disabled from a previous right-click when there was only one tab open.
> 
> I thought I took care of that when I removed all the disabled attributes
> here:
> 
> +            var menuItems = aPopupMenu.childNodes;
> +            for (let i = 0; i < menuItems.length; i++)
> +              menuItems[i].removeAttribute("disabled");

OK, so in that case they're still wrong, but for a different reason: they still depend on having multiple tabs open, even when you're not clicking on a tab. Also:
> +              let disabled = this.tabs.length == 1;
> +              let menuItems = aPopupMenu.getElementsByAttribute("tbattr", "tabbrowser-multiple");
> +              for (let i = 0; i < menuItems.length; i++)
> +                menuItems[i].setAttribute("disabled", disabled);
needs to be tweaked to not bother enabling the items again.

> Unfortunately openPopup() doesn't recognize the "after_pointer" position

Ah, that's disappointing...

> > it possible to prevent the context menu appearing over unwanted elements and 
> > their children by adding an empty context="" attribute on them instead?
> 
> Not sure what you mean by this. Currently the right-click event is consumed by
> either a <xul:tab> or the <xul:tabs> element. The event never reaches "unwanted
> elements and their children"

Reaches? It bubbles up to the <tabs> element from them. The reason you never see it in document.popupNode is because that always uses the event's target, not its originalTarget.

Comment 13

[Philip Chee]

Attachment: Patch v1.1 Better fix.

> It might be better to use getElementsByAttribute("tbattr", "*") and then enable 
> or disable menuitems depending on the value of the attribute, the number of tabs 
> and the localName.
Fixed.

> Instead of this ugly and possibly incorrect code (null and "" parameters?), is
> it possible to prevent the context menu appearing over unwanted elements and
> their children by adding an empty context="" attribute on them instead?
Fixed using context="" where necessary.

Remaining issues: For the context menu on the tabs element, I've left "Reload all tabs" as enabled even if there is only one tab open since there is no option for "Reload remining tab".

Comment 14

[[e-mail]]

Comment on attachment 696283 [details] [diff] [review]
Patch v1.1 Better fix.

Yes, this looks much better.

>+                menuitem.hidden = this.mPrefs.getIntPref("browser.tabs.max_tabs_undo") <= 0 &&
>+                                      this.mPrefs.getIntPref("browser.sessionstore.max_tabs_undo") <= 0;
Nit: incorrect indentation

>+              else if ((tbattr.contains("tabbrowser-bookmarks") && isSingle) ||
>+                       (isTab && tbattr.contains("tabbrowser-multiple") && isSingle) ||
>+                       (!isTab && tbattr.contains("tabbrowser-tabs")))
>+                menuitem.setAttribute("disabled", true);
>+              else
>+                menuitem.removeAttribute("disabled");
I think this bit is more complicated than it needs to be.

menuitem.setAttribute("disabled", tbattr.contains("tabbrowser-multiple") && !isMultiple || tbattr.contains("tabbrowser-tab") && !isTab))

e.g. Close Other Tabs tbattr contains multiple and tab, so is disabled unless both isMultiple and isTab
e.g. Close Tab tbattr contains tab, so is disabled unless isTab
e.g. Bookmark tbattr contains multiple, so is disabled unless isMultiple
(Would work for New Tab too, since tbattr contains neither so is always enabled)
[Note suggested naming tweaks!]

>+             return true;
[Hmm, if we always allow the popup, then we don't need to return anything?]

Comment 15

[Philip Chee]

Attachment: Patch v1.2 fix nits.

>>+                menuitem.hidden = this.mPrefs.getIntPref("browser.tabs.max_tabs_undo") <= 0 &&
>>+                                      this.mPrefs.getIntPref("browser.sessionstore.max_tabs_undo") <= 0;
> Nit: incorrect indentation
Fixed.

>>+              else if ((tbattr.contains("tabbrowser-bookmarks") && isSingle) ||
>>+                       (isTab && tbattr.contains("tabbrowser-multiple") && isSingle) ||
>>+                       (!isTab && tbattr.contains("tabbrowser-tabs")))
>>+                menuitem.setAttribute("disabled", true);
>>+              else
>>+                menuitem.removeAttribute("disabled");
> I think this bit is more complicated than it needs to be.
> 
> menuitem.setAttribute("disabled", tbattr.contains("tabbrowser-multiple") && !isMultiple || tbattr.contains("tabbrowser-tab") && !isTab))
Fixed.

> e.g. Close Other Tabs tbattr contains multiple and tab, so is disabled unless both isMultiple and isTab
> e.g. Close Tab tbattr contains tab, so is disabled unless isTab
> e.g. Bookmark tbattr contains multiple, so is disabled unless isMultiple
> (Would work for New Tab too, since tbattr contains neither so is always enabled)
> [Note suggested naming tweaks!]
Naming tweaks fixed. Now using isMultiple, and "tabbrowser-tab"

>>+             return true;
> [Hmm, if we always allow the popup, then we don't need to return anything?]
Removed return totally.

Comment 16

[Philip Chee]

Pushed comm-central changeset 470f14ee6470
http://hg.mozilla.org/comm-central/rev/470f14ee6470

Comment 17

[Philip Chee]

Attachment: Patch w1.0 Adjust whitespace.

I forgot to address the whitespace issue. Checking in a supplementary patch. Carrying forward r+

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/6e7c673c3157

Comment 18

[Petko Alov]

(In reply to Philip Chee from comment #17)

Great!