Last Comment Bug 626326 - add-on bar should not show context menu when a descendent node was right-click target
: add-on bar should not show context menu when a descendent node was right-clic...
Status: RESOLVED FIXED
[addon bar]
:
Product: Add-on SDK
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: P2 normal (vote)
: ---
Assigned To: Dietrich Ayala (:dietrich)
:
:
Mentors:
Depends on:
Blocks: 634712
  Show dependency treegraph
 
Reported: 2011-01-16 22:40 PST by Dietrich Ayala (:dietrich)
Modified: 2013-12-27 14:31 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Pointer to pull request (359 bytes, text/html)
2011-02-06 09:24 PST, Dietrich Ayala (:dietrich)
adw: review+
Details
Pointer to pull request (359 bytes, text/html)
2011-03-01 09:03 PST, Dietrich Ayala (:dietrich)
no flags Details
Prevent toolbar context menu to be displayed on widgets (705 bytes, patch)
2012-02-08 03:56 PST, Alexandre Poirot [:ochameau]
dietrich: review+
dave.hunt: feedback+
Details | Diff | Splinter Review

Description Dietrich Ayala (:dietrich) 2011-01-16 22:40:36 PST
add-ons often have context menus. currently, if you right click an item in the add-on bar, both the add-on context menu and the add-on bar content menu are shown.
Comment 1 Dietrich Ayala (:dietrich) 2011-01-16 22:41:14 PST
spun off from https://bugzilla.mozilla.org/show_bug.cgi?id=624607#c2.
Comment 2 Dietrich Ayala (:dietrich) 2011-01-16 22:44:30 PST
Requesting blocking. Add-on context menus are common.
Comment 3 Cork 2011-01-16 22:47:03 PST
This is wfm, in linux with abp, noscript and firebug; windows only?
Comment 4 Siddharth Agarwal [:sid0] (inactive) 2011-01-25 06:38:23 PST
WFM with Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b10pre) Gecko/20110125 Firefox/4.0b10pre and ABP.
Comment 5 Dietrich Ayala (:dietrich) 2011-01-25 07:05:49 PST
Can we get a Mac check?
Comment 6 Siddharth Agarwal [:sid0] (inactive) 2011-01-25 08:04:29 PST
WFM with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b10pre) Gecko/20110125 Firefox/4.0b10pre and ABP.

.. though it's possible that ABP could be doing something special here with event.stopPropagation() etc.
Comment 7 Asa Dotzler [:asa] 2011-01-25 08:09:00 PST
If I'm reading this bug correctly it is not fixed. When I context click on the amazing Hard Blocker Counter extension button in the add-on bar I get the add-on menu and the toolbar context menu. See http://grab.by/8zz7 which is today's Windows nightly build.
Comment 8 Dietrich Ayala (:dietrich) 2011-01-25 08:21:10 PST
Hm, interesting. I think the problem is slightly different than I put in the summary originally.

Looks like Widgets from the Add-on SDK are responding to right-click as if it was a regular click. -> moving to Add-on SDK.
Comment 9 Asa Dotzler [:asa] 2011-02-04 14:17:05 PST
Myk, this seems like a pretty bad behavior for any add-on that relies on context clicks. Is there a way for me to nominate this for a nearby release?
Comment 10 Myk Melez [:myk] [@mykmelez] 2011-02-04 14:31:20 PST
Asa: Add-on SDK beta releases are time-based, so this wouldn't block one, but I'd certainly love to see this problem be fixed in the next beta, which ships later this month.  Setting the target milestone accordingly.

Dietrich: is your time still entirely taken up by Firefox 4 blocker bugs, or might you have a bit of time to look into this?
Comment 11 Dietrich Ayala (:dietrich) 2011-02-06 09:24:43 PST
Created attachment 510125 [details]
Pointer to pull request
Comment 12 Dietrich Ayala (:dietrich) 2011-02-06 09:27:09 PST
I don't know if disabling right-click in widget-space is the right thing to do long term, but it'll do for now.

The workaround for add-ons that need right-click support is to handle it in a content script and msg back to their privileged code.
Comment 13 Mike Beltzner [:beltzner, not reading bugmail] 2011-02-08 13:51:58 PST
--> Moving to Firefox to get rid of the blocking flag; this is an SDK bug.
Comment 14 Mike Beltzner [:beltzner, not reading bugmail] 2011-02-08 13:52:30 PST
--> Moving back.
Comment 15 Drew Willcoxon :adw 2011-02-28 10:25:25 PST
Comment on attachment 510125 [details]
Pointer to pull request

If I'm reading this bug right, there are two problems: 1) the widget "click" event is fired for right-clicks, but it should only be fired for left-clicks (comment 8), and 2) the add-on bar's context menu is shown when you right-click a widget (comment 7).

Is that right?  This patch fixes problem 1 but not problem 2, right?

(In reply to comment #12)
> I don't know if disabling right-click in widget-space is the right thing to do
> long term, but it'll do for now.

I think the widget "click" event should be for left-clicks only, so this patch is fine with me.  Eventually we should add a "RightClick" event and a nice API for attaching context menus to widgets similar to the context-menu API.

I made some inline comments on GitHub.  r+ with the changes mentioned there, but problem 2, if it is in fact happening, needs to be fixed too.
Comment 16 Dietrich Ayala (:dietrich) 2011-03-01 07:23:20 PST
Yep, will fix comment 7 in this bug too.
Comment 17 Dietrich Ayala (:dietrich) 2011-03-01 07:56:23 PST
(In reply to comment #16)
> Yep, will fix comment 7 in this bug too.

Too hasty! Current policy in Firefox is to show the host toolbar's context menu for child items that do not have a context menu of their own.
Comment 18 Dietrich Ayala (:dietrich) 2011-03-01 09:03:26 PST
Created attachment 515930 [details]
Pointer to pull request
Comment 19 Dietrich Ayala (:dietrich) 2011-03-01 09:06:34 PST
latest attachment fixes adw's comments, but not the add-on bar context menu bit.
Comment 20 Dietrich Ayala (:dietrich) 2011-03-09 00:42:14 PST
Hm, this is a right patch on the wrong bug.

The patch makes widgets stop treating right-clicks as left-clicks.

The patch does not resolve the issue in the summary, and comment #7.
Comment 21 Drew Willcoxon :adw 2011-03-14 14:03:00 PDT
Comment on attachment 515930 [details]
Pointer to pull request

This seems to be the same pull request I already r+'ed.
Comment 22 Dietrich Ayala (:dietrich) 2011-03-14 23:00:41 PDT
(In reply to comment #21)
> Comment on attachment 515930 [details]
> Pointer to pull request
> 
> This seems to be the same pull request I already r+'ed.

The pull request URI is always the same, even after additional commits, if that's what you mean.

However, this bug is not done. I need to look at how Hard Blocker Counter is providing a context menu, and if possible, not show the toolbar context menu when the add-on's context menu is shown.
Comment 23 Dietrich Ayala (:dietrich) 2011-03-16 10:09:28 PDT
I'm splitting this patch off into bug 642166, so we can land that, while fixing the toolbar context menu part here, without things getting more muddy.
Comment 24 Myk Melez [:myk] [@mykmelez] 2011-06-15 12:58:15 PDT
(automatic reprioritization of 1.0 bugs)
Comment 25 George 2011-07-16 12:54:12 PDT
With my add-on Canadian Weather, I display a panel on right-click. Works properly with Windows 7, but Ubuntu 11.04 will also display the toolbar context menu. Firefox 5.0
Comment 26 Wes Kocher (:KWierso) 2011-09-08 11:15:40 PDT
(Pushing all open bugs to the --- milestone for the new triage system)
Comment 27 Alexandre Poirot [:ochameau] 2012-02-08 03:56:12 PST
Created attachment 595371 [details] [diff] [review]
Prevent toolbar context menu to be displayed on widgets

Henrik pointed out on irc this old bug that sounds like a blocker for one or multiple QA addons to be converted from xul to jetpack!

I took some time to look into widget code, and here is a pontential -simple stupid- way to fix this: prevent customize context menu to be display on all widgets.

Henrik, can you confirm that it is the expected behavior? And that this fix works properly.
Comment 28 Dave Hunt (:davehunt) 2012-02-09 04:05:08 PST
Comment on attachment 595371 [details] [diff] [review]
Prevent toolbar context menu to be displayed on widgets

I can confirm that this patch resolved the issue of the toolbar context menu appearing when right clicking on a widget.
Comment 29 Alexandre Poirot [:ochameau] 2012-02-09 06:35:06 PST
Comment on attachment 595371 [details] [diff] [review]
Prevent toolbar context menu to be displayed on widgets

Hey dietrich, as you said that you were open to code review on jetpack,
I've found a really cool one for you: a one liner!

Do you think such XUL trick is OK?
"Setting `context` attribute to an empty value so that we won't have a context menu defined on a parent node"
Comment 30 Dietrich Ayala (:dietrich) 2012-02-09 09:23:16 PST
Comment on attachment 595371 [details] [diff] [review]
Prevent toolbar context menu to be displayed on widgets

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

yes, this is fine. thanks!
Comment 31 Henrik Skupin (:whimboo) 2012-02-09 22:05:36 PST
Comment on attachment 595371 [details] [diff] [review]
Prevent toolbar context menu to be displayed on widgets

>+  // Bug 626326: Prevent customize toolbar context menu to appear
>+  node.setAttribute("context", "");

Shouldn't we move down this line to the other node.setAttribute(...) calls? Just for clarity and readability.
Comment 32 [github robot] 2012-03-05 10:51:59 PST
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/9835974d4aa85ca392f14773851a4f8f35a84d8f
Bug 626326: add-on bar should not show context menu when a descendent node was right-click target r=@autonome
Comment 33 Alexandre Poirot [:ochameau] 2012-03-05 10:54:41 PST
We should include this fix in 1.6.
Is there a tag or anything we can add to bugs in order to do such request?
Comment 34 Alexandre Poirot [:ochameau] 2012-03-05 10:55:41 PST
(In reply to Henrik Skupin (:whimboo) from comment #31)
> Shouldn't we move down this line to the other node.setAttribute(...) calls?
> Just for clarity and readability.

Good catch! I've done this in the final patch.
Comment 35 [github robot] 2012-03-05 12:14:47 PST
Commit pushed to stabilization at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/bc99d37ef6c3c1e7f204f65376623307ff71298b
Bug 626326: add-on bar should not show context menu when a descendent node was right-click target r=@autonome
(cherry picked from commit 9835974d4aa85ca392f14773851a4f8f35a84d8f)
Comment 36 Wes Kocher (:KWierso) 2012-03-05 12:27:38 PST
:)

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