Remove mouse events in Gesture Detector



Firefox OS
4 years ago
4 years ago


(Reporter: janjongboom, Assigned: ramd, Mentored)


Firefox Tracking Flags

(Not tracked)


(Whiteboard: [good first bug])


(1 attachment, 2 obsolete attachments)

46 bytes, text/x-github-pull-request
: review+
Details | Review | Splinter Review
In gesture_detector.js we currently have code to handle mouse events, but we already convert mouse->touch in the touch helper in gecko. This code can probably go.


4 years ago
Whiteboard: [good first bug][mentor=janjongboom]

Comment 1

4 years ago
Hi Jan, Can I get assigned to this bug?
And I have a doubt, do we need to remove js file and its link in all other files because it contains mouse events only.
Hi Vaishnav, I've assigned you to this bug.

The file 'shared/js/gesture_detector.js' contains some code regarding mouse-events (mousedown, mousemove, mouseup). That code can be removed. The gesture_detector.js file is the only file you'll need to touch.

Please ping me on (#gaia or #b2g, @janjongboom) if you need assistance. Or post a new comment here and set 'Need more information from' to me.
Assignee: nobody → vaishnav.rd
Flags: needinfo?(vaishnav.rd)

Comment 3

4 years ago
Thanks Jan, let me dive deeper into this file now.
Flags: needinfo?(vaishnav.rd)

Comment 5

4 years ago
Created attachment 8350967 [details] [review]
pull request.txt

Pull request :
Attachment #8350967 - Flags: review?(janjongboom)
Sorry that I didn't take a look at this PR yet, it's due to the holidays. It's first thing on my list on January 2nd.
Comment on attachment 8350967 [details] [review]
pull request.txt

Thanks :Ram, almost there.

* Line 69: this.listeningForMouseEvents = true, should be removed, not commented out
* Line 65: this.options.mousePanThreshold = and 66, should be removed
* Line 165: GD.MOUSE_PAN_THRESHOLD = 15;, same
* Line 193: 'mousedown' should be removed from array
* function mouseCoordinates(e) { can be removed
* On top of the file, the description contains a lot of references to mouse. Can we remove that?

There is also a unit test in gallery that currently fails due to these changes, but I'll fix those before landing.

Can you make these changes, and re-push? Thanks!
Attachment #8350967 - Flags: review?(janjongboom)

Comment 8

4 years ago
Created attachment 8355176 [details] [review]

Thanks, I have fixed above things also, kindly have a look into new pull request.
Attachment #8350967 - Attachment is obsolete: true
Attachment #8355176 - Flags: review?(janjongboom)
Thanks Ram, I fixed the tests; so let's see if the build passes.
Comment on attachment 8355176 [details] [review]

r=janjongboom. :Ram, thanks for your contribution!

Landed in
Attachment #8355176 - Flags: review?(janjongboom) → review+

Comment 12

4 years ago
Do I need to do anything else also ?
Flags: needinfo?(janjongboom)
I'll take a look at why the tests busted.

Comment 14

4 years ago
Jan, Do you have any update on this?
Whiteboard: [good first bug][mentor=janjongboom] → [good first bug]
Rebased the code and now running all the tests.
Flags: needinfo?(janjongboom)
Created attachment 8469154 [details] [review]
Rebased PR
Attachment #8355176 - Attachment is obsolete: true
Attachment #8469154 - Flags: review+
Try is green. Merged again

Sorry that it took so long for me to get back to this...
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.