987569 - [Camera][Madai] Add transition animations when swiping in the gallery/preview

Status: RESOLVED FIXED

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

Description

[Diego Marcos [:dmarcos]]

Attachment: gallery.MOV

Without a transition animations the swiping gesture feels cheap. Gestures work right when you feel like directly manipulating a physical object. The user needs immediate feedback during the whole duration of the gesture.

Compare this video to the attach one from our gallery:

https://www.youtube.com/watch?feature=player_detailpage&v=IhrbT9O6kW8#t=25

Comment 1

[Diego Marcos [:dmarcos]]

If we cannot offer transitions with good performance I think it's better to just provide buttons to go to the next and previous pictures

Comment 2

[Tiffanie Shakespeare [:tif]]

Rob and I also noticed some weirdness when swiping between photos/videos in preview. For us though, it seemed like the photo would partially load (~10%), stop for like a 1/4 second, and then continue loading. It felt like a website back in the 90's.

We also noticed that if the next item is a video, there is a little image in the upper right that makes me think of what one would see if the image wasn't there. You can actually see this in Diego's video attachment.

Comment 3

[Tiffanie Shakespeare [:tif]]

Should the items I mention above be files as a separate issue?

Comment 4

[Rob MacDonald [:robmac]]

(In reply to Diego Marcos [:dmarcos] from comment #0)
> Without a transition animations the swiping gesture feels cheap. Gestures
> work right when you feel like directly manipulating a physical object. The
> user needs immediate feedback during the whole duration of the gesture.

Totally agree. Is it possible to use the same transitions as Gallery? It makes sense to have consistency in this case.

Comment 5

[Tiffanie Shakespeare [:tif]]

Just found a bug for the weird thumbnail thing...

Bug 987610 - [Camera][Madai][UX] Preview - Default image icon is temporary displayed before rendering video

Comment 6

[Tiffanie Shakespeare [:tif]]

Huzzah! Found a bug for the other item as well. 

Bug 988594 - [Camera] Very slow to draw previews when swiping through Preview from Camera

Comment 7

[Justin D'Arcangelo [:justindarc]]

Tracking a bug to refactor to encapsulate the Gallery app's carousel into a reusable component in this bug:

Bug 984566 - [B2G][Media] Encapsulate Gallery carousel into media_carousel.js

Comment 8

[David Flanagan [:djf]]

Attachment: link to patch on github

This patch adds transitions when swiping between items in the preview gallery.  It also allows panning within a zoomed image without swiping (bug 989361)

It also unlocks the orientation while displaying the preview gallery so that the delete and share dialogs come out the right way. This actually simplifies a lot of code. And it makes the confirm/retake dialog for pick activities handle orientation correctly too. It does mean that we get the jerky orientation changes that other apps are subject to.  But at least things work right now.

So this patch is for this bug, but also 951014 and 989361. I don't know if we have a bug filed for the confirmation view that does not handle orientation, but it fixes that as well.

I'm pretty sure it currently breaks unit tests, so I'm asking for feedback instead of final review.

Also, I've occasionally seen a situation where when returning from preview gallery back to the camera, the camera does not get its correct orientation. I can't repeat this reliably, however, and have not been able to figure it out.  So I need help really hammering on this patch.

Tif: please see what you think about the swipe and the transition. There is probably room for improvement, but it is much much better than what we had.

I'll also investigate to see if we can do anything to soften the jerky orientation change in preview mode (like fade out and fade back in when we get a resize event).

Comment 9

[Wilson Page [:wilsonpage]]

Comment on attachment 8403671 [details] [review]
link to patch on github

Works very well, and the code look thorough :)

Unit-tests are my only concern. There are areas (especially views/preview-gallery.js) that have a lot of complexity that really needs coverage. This will be easier if functions are broken down into smaller testable pieces that do fewer things.

Happy to lend a hand if you need it.

Comment 10

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8403671 [details] [review]
link to patch on github

This is looking good so far. It looks like we're losing a lot of complexity with the orientation-related stuff which is really nice. I can also lend a hand with helping to add some test coverage as well if you need.

Comment 11

[Tiffanie Shakespeare [:tif]]

Comment on attachment 8403671 [details] [review]
link to patch on github

Overall this patch is a nice improvement and I'm giving it a + because I know time is tight. I spoke with David a bit on IRC about a couple of things. Those being:

1. The redraw when swiping between photos - David confirmed this is a Nexus 4 issue
2. When zooming in and panning, it's still easier than I would like to accidentally go to the next photo (I'm being super nitty here)
3. Rotating the device creates a brief redraw of the wrong orientation before switching orientations. But our dialogues now are in the proper orientation. David said he's going to see if he can at least hide this redraw.

I don't think any of these are show stoppers and given our deadlines I'm going to review +.  Hopefully David's next patch update can resolve #3.

Comment 12

[Amy Lee [:amylee] UX]

Comment on attachment 8403671 [details] [review]
link to patch on github

I would change the black background to match the grey in the letter boxing. Thanks!

Comment 13

[David Flanagan [:djf]]

The ugly double-redraw on orientation change seems to affect other apps as well, and I think it is a regression in the 1.5 system app.  See bug 993019.  

Setting document.body.style.opacity to 0 on the resize event and then setting the opacity back again after a few hundred ms hides the problem, but I'm not going to do now because I hope we can just get the underlying bug fixed instead.

Comment 14

[David Flanagan [:djf]]

Comment on attachment 8403671 [details] [review]
link to patch on github

Justin,

Here's an updated version of the patch.  It addresses the comments you and wilson left on github.

It fixes the background color that Amy asked for.  

It makes it harder to accidentally switch between images when panning within a zoomed image (Tif asked for this fix) This is done by requiring a longer swipe or a faster swipe.

It does not address the double redraw on orientation change issue because I think that is a system app bug.

And it updates the unit tests and adds a few new ones.

I'm also asking Amy for ui-review again to check the background color change, but given that we have Tif's review+ already, I think we can land this. Justin: if it looks good to you, and if I'm not online, feel free to land this so you can rebase your own patch on top of it.

Comment 15

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8403671 [details] [review]
link to patch on github

David: Conditional R+. I had one comment on the `resize` event handler. I see where you're adding/removing the event listener on the `open` and `close` methods, which is good, but I still see the original one that doesn't appear to ever get removed. I'm going to hold off on landing this patch until you get a chance to take a look at that. I'll also hold off on rebasing and landing 989429 for now. Otherwise, looks good!

Comment 16

[David Flanagan [:djf]]

Thanks, Justin,

I misread Wilson's original comment and thought it was referring to the place in preview_gallery where I wasn't removing the resize event handler. Since I'd already fixed it there I didn't pay enough attention to notice that he was talking about the confirm view.

I've added code to remove the handler in that file as well. Note that the code will never actually be invoked because the confirm dialog view never actually gets destroyed.  But if we ever change things so that it does, the handler should be removed.

I also added another test to verify that the preview gallery is properly adding and removing the resize handler it needs.

Comment 17

[David Flanagan [:djf]]

Landed on master: https://github.com/davidflanagan/gaia/commit/f46815ac901fb1168328ce4266792b9312c4bc69

Comment 18

[David Flanagan [:djf]]

The patch that just landed also fixes two 1.4+ bugs. This bug is not 1.4+, however, so although the patch needs to be uplifted, it will not be automatically uplifted until we get the flag set.

Hema, would you set 1.4+ on this bug please?

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for v1.4 uplift.

Comment 20

[David Flanagan [:djf]]

I put the wrong link for the commit to master: https://github.com/mozilla-b2g/gaia/commit/fb29a8ea2d79c56cf2dda54ce2056e7e53644243

Working now on resolving the merge conflict

Comment 21

[David Flanagan [:djf]]

Uplifted to v1.4: https://github.com/mozilla-b2g/gaia/commit/11d161210f63cc080dacfde75f1d17e2447762ab

The merge conflict was a bizarrely trivial one, and I don't understand why it didn't resolve automatically.

Comment 22

[Yeojin Chung [:YeojinC]]

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

Comment 23

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

Test case has been created in moztrap:

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