587261 - No edit context menu in textbox of tab group

Status: RESOLVED WONTFIX

(Firefox Graveyard :: Panorama, defect, P3)

Description

[Alice0775 White]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b4pre)
Gecko/20100813 Minefield/4.0b4pre ID:20100813041308
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b4pre)
Gecko/20100813 Minefield/4.0b4pre ID:20100813041308

I can not select text by mouse drag in textbox of Tab group.
The container of the tab group moves instead.

Reproducible: Always

Steps to Reproduce:
1. Start Minefield with new profile
2. Open TabView
3. Create tab group
4. Right click textbox on top of the Tab group

Actual Results:
 No edit context menu in textbox of Tab groop

Expected Results:
 Edit context menu should appear.

Comment 1

[Michael Yoshitaka Erlewine [:mitcho]]

See also bug 590251.

Comment 2

[Michael Yoshitaka Erlewine [:mitcho]]

Punting to future.

Comment 3

[Raymond Lee [:raymondlee]]

Attachment: v1

> +  _initContextMenu: function TabView__initContextMenu() {
> +    let contextMenu = document.getElementById("tab-view-context-menu");
> +
> +    [ "menu_undo", "menu_redo", "menuseparator", "menu_cut",
> +      "menu_copy", "menu_paste", "menu_delete", "menuseparator",
> +      "menu_selectAll" ].forEach(function(id) {
> +      let item;
> +      if (id == "menuseparator") {
> +        item = document.createElement("menuseparator");
> +      } else {
> +        let sourceItem = document.getElementById(id);
> +        item = sourceItem.cloneNode(false);
> +        item.id = id.replace("menu_", "tab-view-context-");
> +      }
> +      contextMenu.appendChild(item);
> +    });
> +  },

In order to prevent adding new entries to our properties file, this code just clones the appropriate items with label, accesskey and command for input boxes.

> -    iQ("#searchbutton").mousedown(function() {
> +    iQ("#searchbutton").click(function() {
>       self.initiatedBy = "buttonclick";

Changed it to click because right click activates the search mode

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 559081 [details] [diff] [review]
v1

Review of attachment 559081 [details] [diff] [review]:
-----------------------------------------------------------------

Nice! I just tried your patch and the one problem is that every context menu item is always enabled. In an untouched empty input field "undo", "redo", "copy", "delete all", etc. don't make sense.

How about creating a separate file "contextMenu.js" and declaring a ContextMenu object there? That object should care about initializing and creating the context menu. We should not simply clone those menu nodes but get the string values and actions ourselves. The ContextMenu itself is then responsible for enabling/disabling the corresponding menu items depending of the state of the currently selected input field. I think you should also provide a context menu for the search field while you're at it.

Comment 5

[Raymond Lee [:raymondlee]]

(In reply to Tim Taubert [:ttaubert] (on vacation Sep 10-25) from comment #4)
> Nice! I just tried your patch and the one problem is that every context menu
> item is always enabled. In an untouched empty input field "undo", "redo",
> "copy", "delete all", etc. don't make sense.
> 
It works fine for me on Mac. The reason why I clone those menu items are because the menu bar (edit menu with copy, paste, etc) is still visible on Mac in Panorama mode.  Therefore, it makes sense to clone them and let the command observer/notifier to enable and disable those menu items.

OK, might be we can modify code in updateEditUIVisibility(). Set gEditUIVisible to true when the context menu for text box is showing/open in Panorama. This should avoid to write the same code again for Panorama and the edit menu in the menu bar can be opened in Panorama mode would act exactly the same as context menu in Panorama. 
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#3751
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/editMenuOverlay.js#40

> How about creating a separate file "contextMenu.js" and declaring a
> ContextMenu object there? That object should care about initializing and
> creating the context menu. We should not simply clone those menu nodes but
> get the string values and actions ourselves. The ContextMenu itself is then
> responsible for enabling/disabling the corresponding menu items depending of
> the state of the currently selected input field. I think you should also
> provide a context menu for the search field while you're at it.

Comment 6

[Raymond Lee [:raymondlee]]

Attachment: v2

* Updated updateEditUIVisibility() to support the context menu in Panorama.
* Created contextMenu.js to handle context menu in Panorama

Comment 7

[Raymond Lee [:raymondlee]]

Comment on attachment 559465 [details] [diff] [review]
v2

BTW, this should also fix bug 605092

Comment 8

[Raymond Lee [:raymondlee]]

Tim: could you review this patch please?  Thanks!

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 559465 [details] [diff] [review]
v2

Review of attachment 559465 [details] [diff] [review]:
-----------------------------------------------------------------

Nice work! r=me with the little issues fixed. We'd still need review from a browser peer, though.

::: browser/base/content/browser-tabview.js
@@ +283,5 @@
>      return groupItem ? groupItem.getTitle() : "";
>    },
>  
>    // ----------
> +  _updateTabContextMenu: function TabView__updateTabContextMenu(tab, popup) {

browser_tabview_bug587990.js still relies on the old method name.

::: browser/base/content/browser.js
@@ +3751,5 @@
>  function updateEditUIVisibility()
>  {
> +  let tabviewContextMenuPopupState = "closed";
> +  if (document.getElementById("tab-view-context-menu"))
> +    tabviewContextMenuPopupState = document.getElementById("tab-view-context-menu").state;

I think we shouldn't call document.getElementById("tab-view-context-menu") twice. Please save that result in a variable.

::: browser/base/content/tabview/contextMenu.js
@@ +44,5 @@
> +  // ----------
> +  // Function: init
> +  // Initializes this object.
> +  init: function ContextMenu_init() {
> +    this._menu = gWindow.document.getElementById("tab-view-context-menu");

We're using gWindow.document a lot in this class. Maybe you could save this to "this._doc" or something that would save us a property lookup per access.

@@ +46,5 @@
> +  // Initializes this object.
> +  init: function ContextMenu_init() {
> +    this._menu = gWindow.document.getElementById("tab-view-context-menu");
> +    this._initClipboardItems()
> +  },

Please add a ContextMenu.uninit() method that deletes "this._menu" and make sure this method is called on Panorama shutdown.

@@ +56,5 @@
> +    let self = this;
> +
> +    [ "menu_undo", "menu_redo", "menuseparator", "menu_cut",
> +      "menu_copy", "menu_paste", "menu_delete", "menuseparator",
> +      "menu_selectAll" ].forEach(function(id) {

Please move this list to the top and declare it as a property with a nice name :)

@@ +77,5 @@
> +  //  boolean indicates whether the context menu should be displayed or not.
> +  shouldShowContextMenu: function ContextMenu_shouldShowContextMenu() {
> +    // for group name and search input boxes
> +    if (gWindow.document.popupNode.getAttribute("class") == "name" ||
> +        gWindow.document.popupNode.id == "searchbox") {

Nit: please create a local variable popupNode to save some property lookups and shorten the if-condition.

Comment 10

[Raymond Lee [:raymondlee]]

Attachment: v3

(In reply to Tim Taubert [:ttaubert] from comment #9)
> Nice work! r=me with the little issues fixed. We'd still need review from a
> browser peer, though.
> 
> ::: browser/base/content/browser-tabview.js
> @@ +283,5 @@
> >      return groupItem ? groupItem.getTitle() : "";
> >    },
> >  
> >    // ----------
> > +  _updateTabContextMenu: function TabView__updateTabContextMenu(tab, popup) {
> 
> browser_tabview_bug587990.js still relies on the old method name.

Fixed

> 
> ::: browser/base/content/browser.js
> @@ +3751,5 @@
> >  function updateEditUIVisibility()
> >  {
> > +  let tabviewContextMenuPopupState = "closed";
> > +  if (document.getElementById("tab-view-context-menu"))
> > +    tabviewContextMenuPopupState = document.getElementById("tab-view-context-menu").state;
> 
> I think we shouldn't call document.getElementById("tab-view-context-menu")
> twice. Please save that result in a variable.
> 

Fixed

> ::: browser/base/content/tabview/contextMenu.js
> @@ +44,5 @@
> > +  // ----------
> > +  // Function: init
> > +  // Initializes this object.
> > +  init: function ContextMenu_init() {
> > +    this._menu = gWindow.document.getElementById("tab-view-context-menu");
> 
> We're using gWindow.document a lot in this class. Maybe you could save this
> to "this._doc" or something that would save us a property lookup per access.
> 

Fixed

> @@ +46,5 @@
> > +  // Initializes this object.
> > +  init: function ContextMenu_init() {
> > +    this._menu = gWindow.document.getElementById("tab-view-context-menu");
> > +    this._initClipboardItems()
> > +  },
> 
> Please add a ContextMenu.uninit() method that deletes "this._menu" and make
> sure this method is called on Panorama shutdown.
> 

Added

> @@ +56,5 @@
> > +    let self = this;
> > +
> > +    [ "menu_undo", "menu_redo", "menuseparator", "menu_cut",
> > +      "menu_copy", "menu_paste", "menu_delete", "menuseparator",
> > +      "menu_selectAll" ].forEach(function(id) {
> 
> Please move this list to the top and declare it as a property with a nice
> name :)
> 

Moved

> @@ +77,5 @@
> > +  //  boolean indicates whether the context menu should be displayed or not.
> > +  shouldShowContextMenu: function ContextMenu_shouldShowContextMenu() {
> > +    // for group name and search input boxes
> > +    if (gWindow.document.popupNode.getAttribute("class") == "name" ||
> > +        gWindow.document.popupNode.id == "searchbox") {
> 
> Nit: please create a local variable popupNode to save some property lookups
> and shorten the if-condition.

Fixed

Comment 11

[Raymond Lee [:raymondlee]]

Attachment: v4

Updated patch after the path change of Panorama module

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 570186 [details] [diff] [review]
v4

I don't think we want updateEditUIVisibility to know about panorama, especially given bug 836758. If this can't be implemented completely within panorama, I guess we need some new extension hooks.

Comment 14

[Justin Dolske [:Dolske]]

Panorama has been removed from Firefox 45, currently in Beta and scheduled for release on March 7th. As such, I'm closing all existing Panorama bugs.

If you are still using Panorama, you will see a deprecation message in Firefox 44, and when 45 is released your tab group data will be migrated to bookmarks, with a folder for each group. There are also a few addons offering similar functionality.

See https://support.mozilla.org/en-US/kb/tab-groups-removal for more info.

We're removing Panorama because it has extremely low usage (about 0.01% of users), and has a large number of bugs and usability issues. The cost of fixing all those issues is far too high to justify, and so we'll instead be focusing our time and energy on improving other parts of Firefox.