Closed Bug 1107714 Opened 5 years ago Closed 5 years ago

[Gallery] When setting a wallpaper in landscape, the wallpaper will expand from its normal size in the homescreen

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

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

VERIFIED FIXED
2.2 S4 (23jan)
blocking-b2g 2.2+
Tracking Status
b2g-v2.1 --- unaffected
b2g-v2.2 --- verified
b2g-master --- verified

People

(Reporter: psiphantong, Assigned: pdahiya)

References

()

Details

(Keywords: regression, Whiteboard: [2.2-flame-reduced-run])

Attachments

(2 files, 3 obsolete files)

Attached file ww.txt
Description:
When setting a wallpaper in landscape, the wallpaper will expand from its normal size in the homescreen

Setup Steps:
1) Setup Steps: Have a wallpaper saved from http://wallpaperswide.com/batman-desktop-wallpapers.html (1920x1080) 
  
Repro Steps:
1) Update a Flame device to BuildID: 20141203040207
2) Go to Gallery app
3) Select the wallpaper > save as wallpaper > press the home button
4) Go back to gallery > press back button > rotate phone to landscape
5) Tap the same image > set to wallpaper > tap home button


Actual:
the wallpaper will expand from its normal size in the homescreen
 
Expected: 
wallpaper display correctly 

  
Flame 2.2

Device: Flame 2.2 Master (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141203040207
Gaia: 725685831f5336cf007e36d9a812aad689604695
Gecko: 2c9781c3e9b5
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 37.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0


  
Repro frequency: 3/3, 100%
Link to failed test case: https://moztrap.mozilla.org/manage/case/8789/
See attached: video, logcat, https://www.youtube.com/watch?v=DGINmLrIlX0
This issue does not reproduce on the Flame 2.1, the wallpaper will expand from its normal size in the homescreen

Flame 2.1 Full

Device: Flame 2.1 (319mb)(KitKat)(Full Flash)
Build ID: 20141203001205
Gaia: dbaf3e31c9ba9c3436e074381744f2971e15c7bf
Gecko: ebce587d2194
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 34.0 (2.1)
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(dharris)
Nominating 2.2? since this is a regression and a bad user experience.
blocking-b2g: --- → 2.2?
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
QA Contact: ckreinbring
Flags: needinfo?(dharris)
This window cannot be found because the repro rate decreases greatly the farther back into builds we go.  The last build I reproduced this issue was reproduced after 74 attempts.  This build was also as far back as 2.1 so it is likely that the branch checks are invalid and possible that this isn't actually a regression.  As low as the repro rate for this issue is however, would make it very difficult to determine.

Environmental Variables:
Device: Flame 2.1
BuildID: 20141209014645
Gaia: 89421df25ca321f2f4c152dd6e2146cf18b00f06
Gecko: 74fd011aacbf
Version: 34.0 (2.1) 
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
Flags: needinfo?(jmitchell)
The above environmental variables were a bad copy paste, here are the correct ones.

Environmental Variables:
Device: Flame 2.1
BuildID: 20140830092616
Gaia: c05ee27dd1f39e0f1cceb8bc7706e20f297cd9df
Gecko: 983cf2175495
Version: 34.0a1 (2.1) 
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
Flags: needinfo?(jmitchell)
QA Contact: ckreinbring
Punam,

Could you check what is going on? 

Petes:
Is this happening for any wallpaper? Or this particular one?

Thanks
Hema
Flags: needinfo?(psiphantong)
Flags: needinfo?(pdahiya)
Hi Hema, I think its likely to occur on downloaded wallpaper but I had this issue occur on other images on the Gallery app.
Flags: needinfo?(psiphantong)
I am able to replicate the issue with image taken using camera on Flame device with latest master build usinf steps in #comment 0. Looking into the issue
Flags: needinfo?(pdahiya)
Assignee: nobody → pdahiya
On debugging, in landscape mode global variable screenWidth and screenHeight are getting set incorrectly when share activity loads.

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

Initializing screenWidth and screenHeight inside startShare sets screen.Width and screen.Height from wallpaper portrait orientation. This set correct canvas.width and canvas.height and fixes the maximizing of wallpaper seen while setting wallpaper.

https://github.com/mozilla-b2g/gaia/blob/master/apps/wallpaper/js/share.js#L141
Attached file Patch with fix of Bug 1107714 (obsolete) —
Hi David
Please review attached patch that sets screenWidth and screenHeight inside startShare once the wallpaper share activity fully loads. 

I also updated window.innerWidth and window.innerHeight to screen.width and screen.height. While testing though intermittent I noticed window variables not getting correct width and height for portrait share activity when coming from landscape gallery window. This might be an underlying gecko issue, I will create a bug for this. Thanks!
Attachment #8544225 - Flags: review?(dflanagan)
Comment on attachment 8544225 [details] [review]
Patch with fix of Bug 1107714

r- because I'm not convinced that the change in when we query the screen dimensions will always be correct. See github for a couple of suggested alternatives. Also, I think that using screen dimensions instead of window dimensions in other places may be wrong.  (When there is a virtual home button displayed, those dimensions will differ, I think.)
Attachment #8544225 - Flags: review?(dflanagan) → review-
Blocking Reason: Based on comment 7 easily reproducible with images taken from camera as well. Making this a blocker for poor user experience.

Thanks
Hema
blocking-b2g: 2.2? → 2.2+
(In reply to David Flanagan [OOO 1/5-1/12] [:djf] from comment #10)
> Comment on attachment 8544225 [details] [review]
> Patch with fix of Bug 1107714
> 
> r- because I'm not convinced that the change in when we query the screen
> dimensions will always be correct. See github for a couple of suggested
> alternatives. Also, I think that using screen dimensions instead of window
> dimensions in other places may be wrong.  (When there is a virtual home
> button displayed, those dimensions will differ, I think.)

Thanks David for feedback. On debugging further, I have narrowed down the root cause to set wallpaper button getting clicked twice which calls scaleImage method twice, resulting in maximized image. 
https://github.com/mozilla-b2g/gaia/blob/master/apps/wallpaper/js/share.js#L119

I will update patch with your suggestion to take the longer dimension as the height and the shorter as the width while setting screenWidth and screenHeight and disable setButton after getting clicked to prevent hammering.
Comment on attachment 8544225 [details] [review]
Patch with fix of Bug 1107714

Hi David

I have updated the patch with disabling set-wallpaper button after click and initializing screenWidth (shorter dimension) and screenHeight (longer) for wallpapers. 

Initializing screenWidth and screenHeight fixes intermittent IndexSizeError seen due to incorrect screen.width and screen.height when share activity loads. Please let me know if it should be logged and fixed as a separate bug.

Thanks!
Attachment #8544225 - Flags: review- → review?(dflanagan)
Setting NI flag for PeteS to help validate if attached patch fixes the issue reported in this bug. Thanks!
Flags: needinfo?(psiphantong)
Comment on attachment 8544225 [details] [review]
Patch with fix of Bug 1107714

I flagged a couple of nits on github, but with those addressed, this looks good to me.
Attachment #8544225 - Flags: review?(dflanagan) → review+
Patch updated with feedback on github and landed on master

https://github.com/mozilla-b2g/gaia/commit/3439292b8a118a9e1ee80c01cf39d2c7b7ac1612
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Please request Gaia v2.2 approval on this when you get a chance :)
Flags: needinfo?(pdahiya)
Target Milestone: --- → 2.2 S4 (23jan)
It's bizarre, I am able to replicate this bug in today's build with the patch. There's is obviously something else that I have missed going wrong. 

David, My patch even after disabling set button doesn't suffice. Could there be a system app bug while setting wallpaper manager? It will be great if you can provide inputs on what else should be looked into for fixing this issue? Thanks
Flags: needinfo?(pdahiya) → needinfo?(dflanagan)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Also while trying I am able to replicate this bug with downloaded wallpaper from #comment 0 as far back as Flame-kk 2.0.
Attached file also patch the system app (obsolete) —
Punam,

It looks like there are two different bugs here. In one case everything looks fine while sharing the wallpaper, but when we return to the homescreen, we find a zoomed in wallpaper that is not what we thought we were getting.  In the other bug, when the wallpaper app is launched, it displays the image incorrectly. This second one seems to be timing dependent and I saw it intermittently.

Try merging this patch with your code for not allowing the button to be pressed multiple times and see if it seems to fix everything. (I've only tested on master, and this seems like something that needs testing in 2.2 as well.)

My patch does three things:

1) It patches apps/system/js/wallpaper_manager.js to take device orientation into account. In the system app we've got an OrientationManager that can tell us what the default orientation for the device is, so this patch uses that so it does the right thing on landscape mode tablets and on phones. This fixes the bug where the share activity seems to work correctly, but the wallpaper looks wrong when we return to the homescreen.

2) It modifies the wallpaper app to also try to handle the tablet case by checking screen.mozOrientation.  In my testing mozOrientation is always correct, even when the screen size is wrong when we start up, so we use that to figure out what the default orientation of the screen is.

3) It applies the same orientation fix to the window dimensions as it does to the screen dimensions.  In my testing, I found that when this share activity handler is launched when the gallery is in landscape the app is launched with the screen in portrait mode, but the window in landscape mode, then we get a resize event and the window switches to portrait mode, too.  So without dealing with the window dimensions we can see a bug where the image is scaled incorrectly before it is set as the wallpaper.
Flags: needinfo?(dflanagan)
Needinfo Alive because this bug seems to indicate that there are window management issues for this case of an activity handler locked to default orientation invoked by an app that is not in the default orientation. I think we've worked around that here, but this seems like something you should know about if you don't already.

Also, Alive: would you be a good reviewer for the changes to system/js/wallpaper_manager.js? If not, who would you recommend?
Flags: needinfo?(alive)
(In reply to David Flanagan [:djf] from comment #21)
> Needinfo Alive because this bug seems to indicate that there are window
> management issues for this case of an activity handler locked to default
> orientation invoked by an app that is not in the default orientation. I
> think we've worked around that here, but this seems like something you
> should know about if you don't already.

Hmm, looked the video and it seems we locked to the wallpaper's orientation correctly when the activity transition ends, but something unknown happened so the image is not resized correctly.

Currently we are not able to resize the new wallpaper window on transition beginning (otherwise the background gallery will restyle/reflow - unless we don't care), so it might not be an easy fix from window mgmt.

> 
> Also, Alive: would you be a good reviewer for the changes to
> system/js/wallpaper_manager.js? If not, who would you recommend?

Tim or me!
Flags: needinfo?(alive)
(In reply to David Flanagan [:djf] from comment #20)
> Created attachment 8550619 [details] [review]
> also patch the system app
> 
> Punam,
> 
> It looks like there are two different bugs here. In one case everything
> looks fine while sharing the wallpaper, but when we return to the
> homescreen, we find a zoomed in wallpaper that is not what we thought we
> were getting.  In the other bug, when the wallpaper app is launched, it
> displays the image incorrectly. This second one seems to be timing dependent
> and I saw it intermittently.
> 
> Try merging this patch with your code for not allowing the button to be
> pressed multiple times and see if it seems to fix everything. (I've only
> tested on master, and this seems like something that needs testing in 2.2 as
> well.)
> 

Thanks David for the patch. Your fix in system and wallpaper app works and fixes the maximized wallpaper seen when returning to home screen. My patch for not allowing set button to be pressed had landed on master comment #16, so patch attachment 8550619 [details] [review] has my changes.

I have created and tested  merged patch for 2.2 https://github.com/mozilla-b2g/gaia/pull/27536 .


> My patch does three things:
> 
> 1) It patches apps/system/js/wallpaper_manager.js to take device orientation
> into account. In the system app we've got an OrientationManager that can
> tell us what the default orientation for the device is, so this patch uses
> that so it does the right thing on landscape mode tablets and on phones.
> This fixes the bug where the share activity seems to work correctly, but the
> wallpaper looks wrong when we return to the homescreen.
> 
> 2) It modifies the wallpaper app to also try to handle the tablet case by
> checking screen.mozOrientation.  In my testing mozOrientation is always
> correct, even when the screen size is wrong when we start up, so we use that
> to figure out what the default orientation of the screen is.
> 
> 3) It applies the same orientation fix to the window dimensions as it does
> to the screen dimensions.  In my testing, I found that when this share
> activity handler is launched when the gallery is in landscape the app is
> launched with the screen in portrait mode, but the window in landscape mode,
> then we get a resize event and the window switches to portrait mode, too. 
> So without dealing with the window dimensions we can see a bug where the
> image is scaled incorrectly before it is set as the wallpaper.
Comment on attachment 8550619 [details] [review]
also patch the system app

Setting review flag for Alive for the fix in system app. Thanks!
Attachment #8550619 - Flags: review?(alive)
Attached file Patch with fix of Bug 1107714 for 2.2 (obsolete) —
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #22)
> (In reply to David Flanagan [:djf] from comment #21)

> 
> Hmm, looked the video and it seems we locked to the wallpaper's orientation
> correctly when the activity transition ends, but something unknown happened
> so the image is not resized correctly.

Yes, the wallpaper does eventually get the correct portrait orientation. But when the app is first launched, it starts up in landscape orientation, which is what was causing the image to be sized incorrectly. I'm actually seeing two resize events, one where the screen dimensions change from landscape to portrait and another where the window dimensions change from landscape to portrait. (And there is actually a time where the screen is portrait but the window is landscape!)  Anyway, it seems like a race condition of some sort in gecko or in the system app.

> Currently we are not able to resize the new wallpaper window on transition
> beginning (otherwise the background gallery will restyle/reflow - unless we
> don't care), so it might not be an easy fix from window mgmt.

I'm not sure how to resolve that issue.  It would be okay with me if the gallery reflowed, though I suppose others might complain about that. It would be nice if the initial load event or DOMReady event could be deferred until the orientation was correct. But that is probably also hard to achieve.

> > 
> > Also, Alive: would you be a good reviewer for the changes to
> > system/js/wallpaper_manager.js? If not, who would you recommend?
> 
> Tim or me!

Thanks. Punam has flagged you for the review.
Attachment #8550619 - Flags: review?(alive)
Requested Ryan to revert the patch landed in comment#16. Setting review flag on the single merged patch. Thanks!
Attachment #8544225 - Attachment is obsolete: true
Attachment #8550619 - Attachment is obsolete: true
Attachment #8552036 - Flags: review?(alive)
Comment on attachment 8552036 [details] [review]
Merged Patch with fix of Bug 1107714

It's fine but we need to have unit test in wallpaper_manager_test. Lemme know if you have trouble to do that.
Attachment #8552036 - Flags: review?(alive)
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #29)
> Comment on attachment 8552036 [details] [review]
> Merged Patch with fix of Bug 1107714
> 
> It's fine but we need to have unit test in wallpaper_manager_test. Lemme
> know if you have trouble to do that.

Hi Alive

The change in this patch checks for orientation before setting screenWidth and screenHeight. I am new to wallpaper_manager and its unit tests and it will be great if you can give some direction for unit test of this patch. 

I am thinking of test similar to 'change to new small wallpaper blob' that checks for modified blob  screenWidth and screenHeight. 
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/test/unit/wallpaper_manager_test.js#L213

Is that a correct approach and if yes how do we mock orientation to portrait or landscape for tests. Thanks
Flags: needinfo?(alive)
(In reply to Punam Dahiya from comment #30)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #29)
> > Comment on attachment 8552036 [details] [review]
> > Merged Patch with fix of Bug 1107714
> > 
> > It's fine but we need to have unit test in wallpaper_manager_test. Lemme
> > know if you have trouble to do that.
> 
> Hi Alive
> 
> The change in this patch checks for orientation before setting screenWidth
> and screenHeight. I am new to wallpaper_manager and its unit tests and it
> will be great if you can give some direction for unit test of this patch. 
> 
> I am thinking of test similar to 'change to new small wallpaper blob' that
> checks for modified blob  screenWidth and screenHeight. 
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/test/unit/
> wallpaper_manager_test.js#L213
> 
> Is that a correct approach and if yes how do we mock orientation to portrait
> or landscape for tests. Thanks

It's simple! Just require mock_orientation_manager.js in wallpaper_manager_test
and change the default orientation by MockOrientationManager.defaultOrientation = 'portrait-primary' | 'landscape-primary'.
Flags: needinfo?(alive)
Comment on attachment 8552036 [details] [review]
Merged Patch with fix of Bug 1107714

Thanks Alive for your inputs. I have updated patch with the unit tests. Please review.
Attachment #8552036 - Flags: review?(alive)
Comment on attachment 8552036 [details] [review]
Merged Patch with fix of Bug 1107714

Great, thank you!
Attachment #8552036 - Flags: review?(alive) → review+
Thanks Alive for review. Patch landed on master

https://github.com/mozilla-b2g/gaia/commit/d7190a6a31487f968dc65b33c55e09ecbc0469a1
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Attachment #8552000 - Attachment is obsolete: true
Comment on attachment 8552036 [details] [review]
Merged Patch with fix of Bug 1107714

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
Not a regression #comment 3, this bug is probably never caught because of the edge use case of setting wallpaper from gallery via share activity when device is in landscape orientation.

[User impact] if declined: 
1. While sharing wallpaper from gallery app (in landscape mode), when the wallpaper app is launched, it sometimes displays the image incorrectly.

2. After sharing wallpaper from gallery app in landscape mode and setting wallpaper,  when user return to the home screen, wallpaper is zoomed in.
 
[Testing completed]: On master
[Risk to taking this patch] (and alternatives if risky):

Low. Patch puts missing device orientation check in system and wallpaper app so it works correct on landscape mode tablets and on phones. Patch also disables setButton after getting clicked to prevent hammering.

[String changes made]: None
Attachment #8552036 - Flags: approval-gaia-v2.2?(bbajaj)
Attachment #8552036 - Flags: approval-gaia-v2.2?(bbajaj) → approval-gaia-v2.2+
This issue is verified fixed on Flame 2.2 and Master.

Result: The wallpaper is set in the same way regardless the landscape and portrait mode from Gallery.
 
Device: Flame 2.2 (319mb, full flash)
Build ID: 20150129003432
Gaia: 6e494f1d2676d231abba7dcc2e2822d1170d2d02
Gecko: 5e6fac01a72f
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

Device: Flame Master (319mb, full flash)
Build ID: 20150129010239
Gaia: 9d2378a9ef092ab1fc15c3a9f7fc4171aab59d57
Gecko: 6bfc0e1c4b29
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(psiphantong) → needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.