bwinton / TabCenter

Firefox add-on for arranging tabs vertically
https://testpilot.firefox.com/experiments/tab-center
210 stars 55 forks source link

Right click menu not working #583

Closed Cowpatz closed 7 years ago

Cowpatz commented 8 years ago

Right clicking on a tool bar books mark to select "pen in a new tab" not working. In fact all of the right click menu is not working for firefox tabs since installing this. Menu appears greyed out. Using Win 7 64 bit.

tab centre

tomerof commented 8 years ago

untitled-2 i have the same problem... windows 10 pro

SoftVision-PaulOiegas commented 7 years ago

I can confirm this on my end too. here are the exact steps to reproduce:

  1. Start Firefox with a new clean profile.
  2. Right click on toolbar and enable "Bookmarks Toolbar"
  3. Navigate to https://testpilot.firefox.com and install Test Pilot.
  4. Click on Tab Center experiment card and after redirected, choose to "Enable" it.
  5. After enabled, right click on the default Firefox "Getting Started" bookmark.

[Expected results]:

[Actual results]:

ericawright commented 7 years ago

Interesting, this seems to only be reproducible when installed through test pilot, not by using a custom xpi.

Keith94 commented 7 years ago

Related: #490

fredfred41 commented 7 years ago

exact same problem here..

ericawright, how do you install a custom xpi?

ericawright commented 7 years ago

@fredfred41 if you'd like to take a look at the work on here before we release it you can by building your own XPI. Steps:

jasondrew commented 7 years ago

Exact same problem for me too, Firefox 49.0.1 on Windows 10. The context menu (right-click menu) on items in the bookmarks toolbar is grayed out: all the expected options appear, but are disabled. If I access the same items through the main Bookmarks menu, they work OK. It's pretty annoying.

LarnuUK commented 7 years ago

Still ongoing, Windows 7 Professional SP1, Firefox 49.0.1.

ericawright commented 7 years ago

thanks to @brennie for pointing out this is possibly related to this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=625778

(And this provides a possibility for testing, as I could not reproduce in an environment where I could easily change the code, but now I can! 🎉 )

kkot commented 7 years ago

@ericawright Shouldn't it have some higher prio ? For me it makes using this extension very difficult because I often add bookmarks and now I cannot rename them easily.

ericawright commented 7 years ago

@kkot I would say this is probably one of the highest priority bugs we have at the moment. It is simply difficult to fix 😄

fredfred41 commented 7 years ago

@ericawright , thanks, but I dont know how to do what you writed :

After cloning this repository, in the terminal, run npm install and npm install jpm then: jpm xpi to generate an installable XPI file locally once you have the XPI file, you can drag and drop it onto nightly to install.

ericawright commented 7 years ago

@fredfred41 to make it easier for you you can find the xpi file here: https://www.dropbox.com/s/ne5i2wu62a0j4hn/tabcentertest1%40mozilla.com-1.26.0.xpi?dl=0 That contains the most current code as of today. If you open the file with Nightly or Dev Edition it will work (it will not work with regular Firefox).

We should be releasing a new version soon though, and if you start using the custom xpi you will not get future updates.

fredfred41 commented 7 years ago

@ericawright thanks, I will try it, would you please tell us when the new version would be available please. Si when it will, I could manually uninstall the custom xpi.

Off topic, I tried "repair" fonction and I lost all my opened tabs (not synchronisés with Sync of course...) Do you have a way to retrieve them? Thanks

bwinton commented 7 years ago

@fredfred41 Check out Mozilla Support for answers to questions like that…

rmader commented 7 years ago

I also have this issue on two distinctive (not connected) installations, both Firefox 50.1.0, Fedora 25 (up-to-date)

SoftVision-PaulOiegas commented 7 years ago

ericawright commented on Sep 6, 2016 Interesting, this seems to only be reproducible when installed through test pilot, not by using a custom xpi.

@ericawright This issue is bugging me also a lot, so I've put some time in it to figure out the exact behavior. It seams that there has nothing to do with Test Pilot install since I've managed to reproduce it with a custom build made today from the repo. Instead it seams to be directly related to the bookmarks toolbar and Tab Center. See the "Notes" section in my scenario for additional details. Here is the scenario to reproduce this:

[Prerequisites]:

[Steps to reproduce]:

  1. Open latest Nightly build with the profile from prerequisites.
  2. Right click the area under the awesomebar and choose to enable "Bookmarks Toolbar".
  3. Drag the xpi into the browser and install it.
  4. Right click the default bookmarks from the toolbar and observe the options from context menu.

[Expected results]: All options are available and can be selected for the bookmark.

[Actual results]: All options are grayed out and unusable.

[Notes]:

Hope this helps in figuring out the exact cause. If you need any details or want me to help you with testing, let me know.

arai-a commented 7 years ago

looks like rearranging XUL elements results in releasing this._viewElt.controllers, that had a controller for Places:

https://dxr.mozilla.org/mozilla-central/rev/698de2db1b16a5ef3c6a39f0f72885e69aee4022/browser/components/places/content/browserPlacesViews.js#17

this._viewElt.controllers.appendController(this._controller);

maybe released here: https://dxr.mozilla.org/mozilla-central/rev/698de2db1b16a5ef3c6a39f0f72885e69aee4022/dom/xul/nsXULElement.cpp#880

    NS_IF_RELEASE(slots->mControllers);

or here: https://dxr.mozilla.org/mozilla-central/rev/698de2db1b16a5ef3c6a39f0f72885e69aee4022/dom/base/FragmentOrElement.cpp#635 NS_IF_RELEASE(mControllers); or somewhere similar to that.

and controllers is regenerated after that, when gets accessed:

https://dxr.mozilla.org/mozilla-central/rev/698de2db1b16a5ef3c6a39f0f72885e69aee4022/dom/xul/nsXULElement.cpp#1454-1455

   rv = NS_NewXULControllers(nullptr, NS_GET_IID(nsIControllers),
                             reinterpret_cast<void**>(&slots->mControllers));

and regenerated controllers doesn't have the controller appended before, and returns null here:

https://dxr.mozilla.org/mozilla-central/rev/698de2db1b16a5ef3c6a39f0f72885e69aee4022/browser/components/places/content/controller.js#1721

 return view.controllers.getControllerForCommand(aCommand);

so placesController below becomes null and all places commands are set to disabled:

https://dxr.mozilla.org/mozilla-central/rev/698de2db1b16a5ef3c6a39f0f72885e69aee4022/browser/components/places/content/controller.js#1685-1688

var placesController = doGetPlacesControllerForCommand("placesCmd_open"); function updatePlacesCommand(aCommand) { goSetCommandEnabled(aCommand, placesController && placesController.isCommandEnabled(aCommand));

the controller is released at this line in rearrangeXUL in verticaltabs.js: https://github.com/bwinton/TabCenter/blob/48f4b66f67762210023ca1e937d1f6e7f27e206f/verticaltabs.js#L603

contentbox.insertBefore(top, contentbox.firstChild);
arai-a commented 7 years ago

forgot to mention that top there is #navigator-toolbox and it contains #PlacesToolbar that is this._viewElt. above.

Inoki commented 7 years ago

Still present in 51.0.1 (32-bit).

petsuter commented 7 years ago

🎉 When can I get this fix in test pilot?

ericawright commented 7 years ago

We'll try to get it out ASAP

SoftVision-PaulOiegas commented 7 years ago

Me and @SoftVision-CiprianMuresan tested the build with the fix for a few days now and we can confirm that seems to be a good fix. Life saver ! 👍 🎉