[RTL] [Camera] Preview swiping is not reversed

VERIFIED FIXED in Firefox OS v2.2

Status

P1
normal
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: tif, Assigned: dmarcos)

Tracking

unspecified
2.2 S10 (17apr)
x86
Mac OS X
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(blocking-b2g:2.2+, b2g-v2.2 verified, b2g-master wontfix)

Details

Attachments

(4 attachments)

(Reporter)

Description

4 years ago
Created attachment 8587665 [details]
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.
(Reporter)

Comment 1

4 years ago
I'm not sure which meta bug to attach this to - there isn't one for Camera.
blocking-b2g: --- → 2.2?
Flags: needinfo?(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.
Blocks: 1072002
Flags: needinfo?(doliver)
RTL triage: P1 -- camera and gallery need to agree on directionality.
Priority: -- → P1

Updated

4 years ago
blocking-b2g: 2.2? → 2.2+

Comment 4

4 years ago
Diego or Punam: Who can take this?

Thanks
Hema
Flags: needinfo?(pdahiya)
Flags: needinfo?(dmarcos)

Comment 5

4 years ago
Diego is taking this on, assigning to him.
Assignee: nobody → dmarcos
Flags: needinfo?(pdahiya)
Flags: needinfo?(dmarcos)
Created attachment 8591925 [details] [review]
[gaia] dmarcos:bug1150706 > mozilla-b2g:v2.2
(Assignee)

Comment 7

4 years ago
Created attachment 8591926 [details] [review]
Pull Request on v2.2
Attachment #8591926 - Flags: review?(jdarcangelo)
(Assignee)

Comment 8

4 years ago
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 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.
Attachment #8591926 - Flags: review?(jdarcangelo) → review+
(Assignee)

Comment 10

4 years ago
Pushed review changes + unit tests.
(Assignee)

Comment 11

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

Carry on r+ from Justin
Attachment #8591925 - Flags: review+
(Assignee)

Comment 12

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

Who should be approving the PR?
Flags: needinfo?(hkoka)
Attachment #8591925 - Flags: feedback?
(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.
Flags: needinfo?(hkoka)
(Assignee)

Comment 14

4 years ago
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
Attachment #8591925 - Flags: feedback? → approval-gaia-v2.2?(doliver)

Updated

4 years ago
Attachment #8591925 - Flags: approval-gaia-v2.2?(doliver) → approval-gaia-v2.2+

Updated

4 years ago
Keywords: checkin-needed

Updated

4 years ago
Keywords: checkin-needed
v2.2: https://github.com/mozilla-b2g/gaia/commit/d50b8a3919a7b4d8d289f150d3b9bed704ebafa9
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-b2g-v2.2: --- → fixed
status-b2g-master: --- → wontfix
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S10 (17apr)

Comment 16

4 years ago
Created attachment 8595239 [details]
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
Flags: needinfo?(tshakespeare)

Updated

4 years ago
QA Whiteboard: [MGSEI-Triage+]
(Reporter)

Comment 17

4 years ago
Checked out the v2.2 branch and it looks good! Thanks Sue!
Flags: needinfo?(tshakespeare)

Comment 18

4 years ago
Per comment 16 and comment 17, change status to VERIFIED,FIXED
Status: RESOLVED → VERIFIED
status-b2g-v2.2: fixed → verified

Comment 19

3 years ago
Test case has been added in moztrap:
https://moztrap.mozilla.org/manage/case/15921/
Flags: in-moztrap+
You need to log in before you can comment on or make changes to this bug.