1115178 - [RTL][Camera] When setting a preview photo to wallpaper, the photo does not fit the screen

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Wallpaper, defect, P2)

Description

[pete siphantong [:Petes]]

Attachment: 2014-12-23-14-51-57.png

Description:
When setting a preview photo to wallpaper, the photo does not fit the screen


Repro Steps:
1) Update a Flame to 20141223010202
2) Go to Camera > take a photo
3) Tap the preview > tap share button
4) Tap wallpaper


Actual:
photo does not fit the screen


Expected:
photo displays correctly 


Environmental Variables:
Device: Flame 2.2 (319mb)(Kitkat Base)(Full Flash)
Build ID: 20141223010202
Gaia: c2da2bafd4e809317e2ca70c9bf5c11136a32818
Gecko: 0532f2509f3f
Gonk: a814b2e2dfdda7140cb3a357617dc4fbb1435e76
Version: 37.0a1 (2.2)
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0


Repro frequency:100%
See attached: screenshot

Comment 1

[Ahmed Nefzaoui [:Nefzaoui]]

Just checked, this is not occurring anymore.

Comment 2

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

This issue still seems to be occurring with 10/10 attpemts on todays build. Reopening this for now.

Environmental Variables:
Device: Flame 2.2 (319mb)(Kitkat Base)(Full Flash)
Build ID: 20141230010205
Gaia: 322ef5116a5827a30c9a3cd9b842449a9c66a5b3
Gecko: 67872ce17918
Gonk: a814b2e2dfdda7140cb3a357617dc4fbb1435e76
Version: 37.0a1 (2.2)
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Comment 3

[Ahmed Nefzaoui [:Nefzaoui]]

This just works fine here,
No longer occurring.

Comment 4

[Joshua Mitchell (Inactive)]

This issue is occurring again in both 2.2 and 3.0 following the STR from comment 0


Device: Flame 3.0 (KK - Nightly - Full Flash)
Build ID: 20150218010226
Gaia: 82f286f10a41aab84a0796c89fbefe67b179994b
Gecko: 9696d1c4b3ba
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 38.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0


Device: Flame 2.2 (KK - Nightly - Full Flash)
Build ID: 20150217002515
Gaia: ea64caf6d4ab03fc4472eca9f41f20d651d55fa9
Gecko: 78d823f7be4c
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0a2 (2.2)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Comment 5

[Punam Dahiya [:pdahiya]]

Issue reported is specific to wallpaper app share activity in RTL mode and can be replicated by
a) sharing preview image as wallpaper from camera app 
b) sharing image as wallpaper from gallery app

Updating component to wallpaper and taking bug to investigate and submit fix.

Comment 6

[Punam Dahiya [:pdahiya]]

Assigning myself. Ahmed, Please let me know if you are looking into this bug. Thanks!

Comment 7

[GitHub Autolander Bot]

Attachment: [gaia] punamdahiya:Bug1115178 > mozilla-b2g:master

Comment 8

[Punam Dahiya [:pdahiya]]

Comment on attachment 8570156 [details] [review]
[gaia] punamdahiya:Bug1115178 > mozilla-b2g:master

Hi David

Please review attached patch that fixes shift in image while sharing as wallpaper in RTL mode. 

I have tested and for wallpaper share activity allowing parent div preview container to have same size as previewImage div fixes this issue.

Another approach is to check for RTL mode and handle translate set on previewImage on L119 and L145  but prefer simpler css fix.

https://github.com/mozilla-b2g/gaia/blob/master/apps/wallpaper/js/share.js#L119

Thanks
Punam

Comment 9

[David Flanagan [:djf]]

Comment on attachment 8570156 [details] [review]
[gaia] punamdahiya:Bug1115178 > mozilla-b2g:master

r- because I don't understand why this patch works, and because it seems too brittle. Someone could easily add the width:100% back in not realizing that it would break rtl.

Here's how I'd fix it, and if this looks good to you, then you can assume you have my r+ to land this patch.  Instead of removing the width setting, leave that in, and add the following to the #preview styles:

  /*
   * The image positioning code in share.js assumes the preview image
   * is positioned in the upper-left corner
   */
  direction: ltr;

This forces the the image to be laid out ltr in its container so that the transforms we apply are correctly computed.  Since the image is the only child of the container, we don't have to worry about messing up the direction of anything else.

Comment 10

[Alissa（leave from Mozilla）]

Test case has been added in moztrap:

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

Comment 11

[Punam Dahiya [:pdahiya]]

(In reply to David Flanagan [:djf] from comment #9)
> Comment on attachment 8570156 [details] [review]
> [gaia] punamdahiya:Bug1115178 > mozilla-b2g:master
> 
> r- because I don't understand why this patch works, and because it seems too
> brittle. Someone could easily add the width:100% back in not realizing that
> it would break rtl.
> 
> Here's how I'd fix it, and if this looks good to you, then you can assume
> you have my r+ to land this patch.  Instead of removing the width setting,
> leave that in, and add the following to the #preview styles:
> 
>   /*
>    * The image positioning code in share.js assumes the preview image
>    * is positioned in the upper-left corner
>    */
>   direction: ltr;
> 
> This forces the the image to be laid out ltr in its container so that the
> transforms we apply are correctly computed.  Since the image is the only
> child of the container, we don't have to worry about messing up the
> direction of anything else.

Agreed this is better fix , updated css with review feedback. I guess I need to set review flag for the benefit of autolander.

Comment 12

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/e02ec55de52d2f410c44b76a1ffc3d9713eb2ea1

Comment 13

[Punam Dahiya [:pdahiya]]

Comment on attachment 8570156 [details] [review]
[gaia] punamdahiya:Bug1115178 > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):Not a regression
[User impact] if declined: In RTL mode from camera preview and gallery app, when an image is shared as wallpaper, the image will not fit the screen and display shifted.

[Testing completed]: On master
[Risk to taking this patch] (and alternatives if risky):very low
[String changes made]:none

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Pi Wei Cheng [:piwei] (inactive)]

This issue is verified fixed on Flame 3.0 and on Flame 2.2. While setting a wallpaper either via Camera Preview or via Gallery, the confirmation page now displays the wallpaper correctly in RTL (Arabic).

Device: Flame 3.0 Master (full flash 319MB)
BuildID: 20150304010324
Gaia: 3fc0ac309f5fb0c1fe82c12223b955a4efce27e6
Gecko: c5b90c003be8
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 39.0a1 (3.0 Master)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0

Device: Flame 2.2 (full flash 319MB)
BuildID: 20150304002529
Gaia: 8b4b3e4b7e7c308764f71542437fd60625ac6b75
Gecko: 2cb52b7cda5a
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0