Last Comment Bug 669207 - right click in the empty part of the tabbar doesn't produce menu
: right click in the empty part of the tabbar doesn't produce menu
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: SeaMonkey 2.1 Branch
: All Other
: -- normal with 1 vote (vote)
: seamonkey2.17
Assigned To: Philip Chee
:
Mentors:
: 672011 681456 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-04 13:19 PDT by Petko Alov
Modified: 2013-04-05 14:16 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1.0 Proposed fix. (9.19 KB, patch)
2012-12-26 08:15 PST, Philip Chee
neil: review-
Details | Diff | Review
Patch v1.1 Better fix. (8.49 KB, patch)
2012-12-28 05:24 PST, Philip Chee
neil: feedback+
Details | Diff | Review
Patch v1.2 fix nits. (8.36 KB, patch)
2013-01-01 00:31 PST, Philip Chee
neil: review+
Details | Diff | Review
Patch w1.0 Adjust whitespace. (1.44 KB, patch)
2013-01-19 05:09 PST, Philip Chee
philip.chee: review+
Details | Diff | Review

Description Petko Alov 2011-07-04 13:19:52 PDT
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 2011-07-05 03:12:53 PDT
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 3 Petko Alov 2011-07-05 05:41:28 PDT
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 4 Philip Chee 2011-07-16 08:54:57 PDT
*** Bug 672011 has been marked as a duplicate of this bug. ***
Comment 5 Philip Chee 2011-07-18 11:45:33 PDT
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 6 Philip Chee 2011-08-24 02:15:03 PDT
*** Bug 681456 has been marked as a duplicate of this bug. ***
Comment 7 Philip Chee 2011-08-24 03:02:05 PDT
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 2011-08-24 03:04:54 PDT
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 2012-12-26 08:15:29 PST
Created attachment 695761 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2012-12-26 14:54:03 PST
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 2012-12-27 00:19:30 PST
> 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 neil@parkwaycc.co.uk 2012-12-27 16:23:01 PST
(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 2012-12-28 05:24:45 PST
Created attachment 696283 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2012-12-30 14:55:43 PST
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 2013-01-01 00:31:49 PST
Created attachment 696869 [details] [diff] [review]
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 17 Philip Chee 2013-01-19 05:09:58 PST
Created attachment 704212 [details] [diff] [review]
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 2013-04-05 14:16:56 PDT
(In reply to Philip Chee from comment #17)

Great!

Note You need to log in before you can comment on or make changes to this bug.