Closed Bug 669207 Opened 11 years ago Closed 9 years ago

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

Categories

(SeaMonkey :: Tabbed Browser, defect)

SeaMonkey 2.1 Branch
All
Other
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.17

People

(Reporter: petko, Assigned: philip.chee)

References

Details

Attachments

(2 files, 2 obsolete files)

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
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.
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
Closed: 11 years ago
Resolution: --- → INVALID
Duplicate of this bug: 672011
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 → ---
Status: REOPENED → NEW
Duplicate of this bug: 681456
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.
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.
Attached patch Patch v1.0 Proposed fix. (obsolete) — Splinter Review
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-
> 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.
Attached patch Patch v1.1 Better fix. (obsolete) — Splinter Review
> 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+
>>+                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)
Attachment #696869 - Flags: review?(neil) → review+
Pushed comm-central changeset 470f14ee6470
http://hg.mozilla.org/comm-central/rev/470f14ee6470
Status: ASSIGNED → RESOLVED
Closed: 11 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.17
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+
(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.