989361 - [Camera] [Madai] [Preview] Conflict between photo panning and swipe to next/previous picture gestures

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Camera, defect)

Description

[Diego Marcos [:dmarcos]]

Attachment: paddingBug.MOV

When I zoom in an image and swipe my fingers to pan the gesture is interpreted as next/previous image gesture. See attached video.

Comment 1

[Hema Koka [:hema]]

Youngjun - please assign to hyuna or anyone else you can take this up from your team

Comment 2

[Youngjun Kim]

Dear Hyuna,

Please check it.

Comment 3

[David Flanagan [:djf]]

Related to 987569. I'll try to fix this as part of that bug.

Comment 4

[David Flanagan [:djf]]

Wow. That's pretty bad, isn't it.  An earlier version was velocity-sensitive, only switching left or right if the swipe was fast enough. Somewhere in the translation of the preview code we lost the velocity test, so we're switching images on any left or right pan.

I think this is pretty easy to fix, and instead of relying on velocity, I think we can do better and only switch images if the image is already panned all the way to the edge, which is how we do it in gallery.

Comment 5

[David Flanagan [:djf]]

Attachment: link to pull request

Wilson,

This bug changes the way we decide when to switch between images in preview mode. It relies on the return value of MediaFrame.prototype.pan() which tells how many pixels beyond the edge we are.

It also cleans up event names a bit and gets an event out of lib/panzoom.js

Comment 6

[Wilson Page [:wilsonpage]]

Comment on attachment 8402998 [details] [review]
link to pull request

Looks like a good patch, lots of simplification :)

I'm r-'ing because the patch breaks existing unit-tests and has not not added any additional unit-tests to prove the bug has been fixed.

Comment 7

[Amy Lee [:amylee] UX]

Comment on attachment 8402998 [details] [review]
link to pull request

Hi David, 

Can you rebase your patch? The camera app crashes each time I open it from the homescreen. I had the same issue with Wilson's patch but it was fixed once it was rebased. Thanks!

Comment 8

[David Flanagan [:djf]]

Amy,

This patch is actually superseded by the patch in 987569, so I'm just going to clear the review request here instead of rebasing this.

Comment 9

[David Flanagan [:djf]]

This is fixed by the patch that just landed in bug 987569.  This is a 1.4+ bug, so that patch in the other bug will need to be uplifted.

Comment 10

[Yeojin Chung [:YeojinC]]

New test case needs to be added. There is no existing test case.

Comment 11

[KTucker [:KTucker][Inactive 3/4/2016]]

Test case has been added in moztrap:

https://moztrap.mozilla.org/manage/case/14350/