Last Comment Bug 829651 - middle-click on widget shows erroneously the attached panel
: middle-click on widget shows erroneously the attached panel
Status: RESOLVED FIXED
:
Product: Add-on SDK
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: P2 normal (vote)
: ---
Assigned To: Alexandre Poirot [:ochameau]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-11 09:43 PST by Marc Chevrier
Modified: 2013-01-21 10:37 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patched version of sdk/widget.js (from 1.13b1) solving this problem (see line 847) (28.92 KB, application/octet-stream)
2013-01-11 09:43 PST, Marc Chevrier
no flags Details
git patch on master branch (3.10 KB, patch)
2013-01-19 01:56 PST, Marc Chevrier
no flags Details | Diff | Splinter Review
Pull request 735 (165 bytes, text/html)
2013-01-21 06:38 PST, Alexandre Poirot [:ochameau]
evold: review+
Details

Description Marc Chevrier 2013-01-11 09:43:12 PST
Created attachment 701173 [details]
Patched version of sdk/widget.js (from 1.13b1) solving this problem (see line 847)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0
Build ID: 20130104151925

Steps to reproduce:

Create a widget and attach a panel to it through panel property during widget creation.
The widget also includes an icon displayed in the add-on bar.
Click with middle-button of the mouse on the widget.


Actual results:

The panel is displayed


Expected results:

The panel is not displayed. Expect that only left button of the mouse can display the panel.
Comment 1 Wes Kocher (:KWierso) 2013-01-17 11:20:22 PST
Comment on attachment 701173 [details]
Patched version of sdk/widget.js (from 1.13b1) solving this problem (see line 847)

Alex, any chance you could take a look at this and help get this patch in shape for landing?
Comment 2 Alexandre Poirot [:ochameau] 2013-01-17 15:34:15 PST
Comment on attachment 701173 [details]
Patched version of sdk/widget.js (from 1.13b1) solving this problem (see line 847)

Marc, Would you be able to provide a patch intead of a copy of widget.js ?
(or even better, a pull request?)

I looked briefly at the widget.js file you uploaded,
I think it would be even better to do:
  if (e.type == "click" && e.button != 0)
    return;
So that we explicitely accept only left click and nothing else.
Comment 3 Marc Chevrier 2013-01-19 01:56:44 PST
Created attachment 704189 [details] [diff] [review]
git patch on master branch
Comment 4 Marc Chevrier 2013-01-19 02:00:36 PST
Patch on master branch submitted for review.
I hope I proceeded correctly because this is my first contribution to add-on SDK! :)

Let me know if something is wrong.

By the way, how is it possible to contribute through pull request because the add-on sdk repository is read-only for me.
Comment 5 Wes Kocher (:KWierso) 2013-01-19 03:08:30 PST
(In reply to Marc Chevrier from comment #4)
> By the way, how is it possible to contribute through pull request because
> the add-on sdk repository is read-only for me.

You would need to click the "Fork" button on the Github repository, which would create a "forked" copy of the SDK within your own Github account. You can then commit changes to your forked copy, and then issue a pull request from there to the official repository by clicking the "Pull Request" button up near the top of the page. Once you fill out the form on the page to make sure your request only includes the changes you want for this bug and add some comments if needed, you can submit the pull request.

At that point, messages should be sent out from Github to a few people (or at least me), and we can take it from there. To make sure things get tracked properly though, if you know the bug number associated to that pull request, it'd be helpful to at least comment in that bug with a link to your pull request.

For a more wordy explanation (with pictures), https://help.github.com/articles/using-pull-requests :)
Comment 6 Marc Chevrier 2013-01-20 09:46:53 PST
Thanks for the explanations. I will proceed this way for my next patch (if any ;))
Comment 7 Alexandre Poirot [:ochameau] 2013-01-21 06:38:32 PST
Created attachment 704533 [details]
Pull request 735

Marc, I've updated your patch in order to make all widget tests pass.
Various tests were failing because we were using following pattern to simulate click:
  var evt = document.createEvent('HTMLEvents');
  evt.initEvent('click', true, true );
But that end up creating a MouseEvent with an undefined button attribute...
So I've replaced all of them with the following pattern, which is using DOM event constructors (see bug 709127):
  var evt = new MouseEvent('click', {button: 0});

Thanks again for your contribution.
Comment 8 Marc Chevrier 2013-01-21 07:05:05 PST
Thanks to fix my errors.
The curious point is that I only add a new test so erroneous event management was already here !?

I promise to take more time next time to be sure all is OK.

Thanks for your help and patience...
Comment 9 Alexandre Poirot [:ochameau] 2013-01-21 08:25:27 PST
No worry.

The issue was that we no longer accept undefined button, which wasn't the case before your widget.js patch.
Comment 10 [github robot] 2013-01-21 09:21:25 PST
Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/74a16eed171635904e1721a15acb694393803063
Bug 829651 - middle-click on widget shows erroneously the attached panel

https://github.com/mozilla/addon-sdk/commit/dec5f74ae5dda838e01759f1af48d65ef39437cf
Merge pull request #735 from ochameau/middle-click@829651

Fix Bug 829651 - middle-click on widget shows erroneously the attached panel r=@erikvold
Comment 11 Jeff Griffiths (:canuckistani) (:⚡︎) 2013-01-21 10:07:08 PST
Marc: thanks so much for you contribution! 

If you feel liek tackling another bug in the future, feel free to drop by #jetpack on irc.mozilla.org or the jetpack mailing list ( https://groups.google.com/forum/#!forum/mozilla-labs-jetpack ) if you want tips on how to submit a pull request via github, or any other process-related questions.
Comment 12 Jeff Griffiths (:canuckistani) (:⚡︎) 2013-01-21 10:37:39 PST
ps: https://wiki.mozilla.org/WeeklyUpdates/2013-01-21#Friends_of_the_Tree_Friends_of_the_Tree

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