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

RESOLVED FIXED in seamonkey2.17

Status

SeaMonkey
Tabbed Browser
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: Petko Alov, Assigned: Philip Chee)

Tracking

SeaMonkey 2.1 Branch
seamonkey2.17
All
Other

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

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

Comment 1

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

Comment 2

6 years ago
The relevant code is here:
http://hg.mozilla.org/comm-central/annotate/ae84f8b74c90/suite/browser/tabbrowser.xml#l970
(Reporter)

Comment 3

6 years ago
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).
Status: UNCONFIRMED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → INVALID
(Assignee)

Updated

6 years ago
Duplicate of this bug: 672011
(Assignee)

Comment 5

6 years ago
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.
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: INVALID → ---
(Assignee)

Updated

6 years ago
Status: REOPENED → NEW
(Assignee)

Updated

6 years ago
Duplicate of this bug: 681456
(Assignee)

Comment 7

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

Comment 8

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

Comment 9

5 years ago
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.
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
Attachment #695761 - Flags: review?(neil)
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?
Attachment #695761 - Flags: review?(neil) → review-
(Assignee)

Comment 11

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

Comment 13

5 years ago
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".
Attachment #695761 - Attachment is obsolete: true
Attachment #696283 - Flags: review?(neil)
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?]
Attachment #696283 - Flags: review?(neil) → feedback+
(Assignee)

Comment 15

5 years ago
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.
Attachment #696283 - Attachment is obsolete: true
Attachment #696869 - Flags: review?(neil)

Updated

5 years ago
Attachment #696869 - Flags: review?(neil) → review+
(Assignee)

Comment 16

5 years ago
Pushed comm-central changeset 470f14ee6470
http://hg.mozilla.org/comm-central/rev/470f14ee6470
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.17
(Assignee)

Comment 17

5 years ago
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
Attachment #704212 - Flags: review+
(Reporter)

Comment 18

4 years ago
(In reply to Philip Chee from comment #17)

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