1150706 - [RTL] [Camera] Preview swiping is not reversed

Status: VERIFIED FIXED

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

Description

[Tiffanie Shakespeare [:tif]]

Attachment: RTL preview.pdf

STR:
1. Switch phone to Arabic
2. Open Camera and take a few photos
3. Tap on icon to go to Preview.
4. Swipe right to go to new photo

Actual:
Additional photos are found to the right and are reached by swiping left. The same interaction as in a non-RTL language.

Expected:
Changing languages should flip the interaction and additional photos are found to the left. This correct behaviour is seen in Gallery.

Why should this block?
Gallery implements the correct behaviour. Camera preview and Gallery need to have matching interactions in order to avoid some massive user confusion.

Comment 1

[Tiffanie Shakespeare [:tif]]

I'm not sure which meta bug to attach this to - there isn't one for Camera.

Comment 2

[Dylan Oliver [:doliver]]

(In reply to Tiffanie Shakespeare from comment #1)
> I'm not sure which meta bug to attach this to - there isn't one for Camera.

It's bug 1072002.

Comment 3

[Dylan Oliver [:doliver]]

RTL triage: P1 -- camera and gallery need to agree on directionality.

Comment 4

[Hema Koka [:hema]]

Diego or Punam: Who can take this?

Thanks
Hema

Comment 5

[Hema Koka [:hema]]

Diego is taking this on, assigning to him.

Comment 6

[GitHub Autolander Bot]

Attachment: [gaia] dmarcos:bug1150706 > mozilla-b2g:v2.2

Comment 7

[Diego Marcos [:dmarcos]]

Attachment: Pull Request on v2.2

Comment 8

[Diego Marcos [:dmarcos]]

Submitted PR for 2.2. On master I would like to use Justin's carrousel that already implements RTL orientation. It's ready to land but it got postponed

Comment 9

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8591926 [details] [review]
Pull Request on v2.2

One minor comment about simplifying the left/right swap in PreviewGalleryController.prototype.handleSwipe. If you agree, go ahead and change it, but I'm fine with it either way.

Comment 10

[Diego Marcos [:dmarcos]]

Pushed review changes + unit tests.

Comment 11

[Diego Marcos [:dmarcos]]

Comment on attachment 8591925 [details] [review]
[gaia] dmarcos:bug1150706 > mozilla-b2g:v2.2

Carry on r+ from Justin

Comment 12

[Diego Marcos [:dmarcos]]

Comment on attachment 8591925 [details] [review]
[gaia] dmarcos:bug1150706 > mozilla-b2g:v2.2

Who should be approving the PR?

Comment 13

[Dylan Oliver [:doliver]]

(In reply to Diego Marcos [:dmarcos] from comment #12)
> 
> Who should be approving the PR?

Please set the approval‑gaia‑v2.2 flag on the PR and provide the info requested in the form.

Comment 14

[Diego Marcos [:dmarcos]]

Comment on attachment 8591925 [details] [review]
[gaia] dmarcos:bug1150706 > mozilla-b2g:v2.2

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
Not RTL mode was implemented for the gallery preview carrousel
[User impact] if declined:
The carrousel in the gallery preview won't behave as expected in RTL mode
[Testing completed]:
Unit tests included
[Risk to taking this patch] (and alternatives if risky):
Low. The patch only implements small UI tweaks to reverse the swiping direction of the gallery preview on RTL mode.
[String changes made]:
None

Comment 15

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/d50b8a3919a7b4d8d289f150d3b9bed704ebafa9

Comment 16

[Sue]

Attachment: video.mp4

Hi Tiffanie,
I have verified this issue on latest build of Flame 2.2 with the same steps in comment 0. And the behaviour is same as that in Gallery. Could you help to confirm this issue is fixed or not? Thanks!
See attachment:video.MP4

Device: Flame 2.2 
Build ID               20150420002502
Gaia Revision          c15a2b6d3a783813959c2b3bffd2a131f4270b9e
Gaia Date              2015-04-17 17:49:32
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/cc02ee38b252
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150420.041634
Firmware Date          Mon Apr 20 04:16:45 EDT 2015
Bootloader             L1TC000118D0

Comment 17

[Tiffanie Shakespeare [:tif]]

Checked out the v2.2 branch and it looks good! Thanks Sue!

Comment 18

[Sue]

Per comment 16 and comment 17, change status to VERIFIED,FIXED

Comment 19

[Sue]

Test case has been added in moztrap:
https://moztrap.mozilla.org/manage/case/15921/