816647 - [Gallery] Set photo as wallpaper does nothing

Status: VERIFIED FIXED

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

Description

[Deepa Subramanian]

Attachment: Logcat- Photo to set as wallpaper

Build ID: 2012 1129071415 V 1.0.0

Steps to Reproduce: 
1. Tap on Gallery App
2. Select a photo to share
3. Select share button
4. From the options select Wallpaper
5. Tap Home button

Actual result: 
Photo selected should be set as wallpaper 

Expected result: Photo is not set as wallpaper

Comment 1

[Chris Lee [:clee]]

This is a regression and not to UX spec.  

Here is the UX spec - https://www.dropbox.com/sh/pygek1oa9a5pspu/ZvJbaVdx4e/Interaction%20Design/Apps/Gallery/B2G_Gallery_DRAFT_v05.pdf

The text should be 'Set as Wallpaper'. Functionally this needs to work, but we can choose the simplest path forward.

Comment 2

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

The functionality does work in setting the wallpaper, the ui doesn't go away.  It looks like it's stuck.  If you close the app, you will see that the wallpaper is set.

Basically neither buttons dismiss the current dialog/ui.

Comment 3

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

To clarify once you hit set as wallpaper, the cancel nor the set as wallpaper button does not dismiss the wallpaper.  The cancel button if hit first will cancel the dialog.

Comment 4

[(inactive after 6/18) Alive Kuo [:alive]]

I wonder this had worked before....
The root cause is the inline activity frame's z-index is 1024 but the fullscreen-ed app's z-index is the MAXIMUM value of z-index.

c.c. David Flanagan, do you remember if you tested this function before and it works? (The path is gallery->single view->share->wallpaper app)

Comment 5

[[e-mail], [e-mail]]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/6798#issuecomment-10986537

Pointer to Github pull-request

Comment 6

[[e-mail], [e-mail]]

Comment on attachment 688132 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/6798#issuecomment-10986537

I press the attach button incautiously
Please ignore the comment.

Comment 8

[(inactive after 6/18) Alive Kuo [:alive]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/6798

https://github.com/mozilla-b2g/gaia/pull/6798
Patch proposal:
Steal/Copy the window height to inline activity frame and reset top to 0 when a DOM element is in fullscreen mode(using FullScreen API).

David Flanagan, please see if this works for you, I have tested on my own. Gallery->Share to Wallpaper is working now.

Comment 9

[David Flanagan [:djf]]

The Gallery app changed recently to use fullscreen by default. (It used to be it would only go into fullscreen mode when hiding the buttons, so you would never be in fullscreen mode and have access to the share button at the same time.)  I didn't test the share activity after making that change.  Sorry I didn't catch this bug then.

The pull request fixes it for me.  I'm not sure I understand the pull request, but I'll try making some comments on github.

Comment 10

[David Flanagan [:djf]]

Comment on attachment 688146 [details]
https://github.com/mozilla-b2g/gaia/pull/6798

The patch works for me (enabling sharing images from gallery to wallpaper and to bluetooth). It is quite simple, so the risk seems low.

However, this is a change to the system app, so we need to be very careful. And it affects the status bar, fullscreen, and inline activites, which are three parts of the system app that I really don't know anything about!

I'm am going to give this a cautious r+, but am also asking timdream to take a look at it.

It would help me be more confident in the patch if you answered these questions:

1) is the change to statusbar.js just a better way to detect fullscreen mode, or is it a required part of the patch?

2) why doesn't the existing code in window_manager.js work? If the app is in fullscreen mode, why isn't its height the same as the window height?

3) How else have you tested this? Have you checked inline activities that are not fullscreen?

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 688146 [details]
https://github.com/mozilla-b2g/gaia/pull/6798

The changes seems fine.

Alive, please test keyboard access of the on-top-of-mozFullScreen inline frame -- if keyboard works you have my r+. I cannot think of other possible side effect on top of my head right now, but do test those if you think of any.

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to David Flanagan [:djf] from comment #10)
> I'm am going to give this a cautious r+, but am also asking timdream to take
> a look at it. 

Thanks for that!

> It would help me be more confident in the patch if you answered these
> questions:
> 
> 1) is the change to statusbar.js just a better way to detect fullscreen
> mode, or is it a required part of the patch?

That's just a clean up, not required.

> 2) why doesn't the existing code in window_manager.js work? If the app is in
> fullscreen mode, why isn't its height the same as the window height?

The change in window_manager.js is in setInlineActivityFrameSize() -- only the size of inline activity frame needs to change based on mozFullscreen condition.

> 3) How else have you tested this? Have you checked inline activities that
> are not fullscreen?

I think that part is safe..., but Alive should test his change locally.

Comment 13

[David Flanagan [:djf]]

Thanks for looking at this Tim. I just wasn't confident enough about that part of the system app.

Alive might have a hard time testing the keyboard in an inline activity launched from a fullscreen app.  I'm not sure we have any existing app combination that allows that.

Thanks also for answering my questions. I had actually intended them for Alive, but that wasn't clear from what I wrote. :-)

In #2, my confusion was about how fullscreen works. If the app is fullscreen, I was thinking its iframe to have the same size as the screen.  But maybe that isn't how fullscreen works. I guess just the content element that requests fullscreen gets big, but the containing iframe retains its size.  I'm hoping that Alive's change in this file will fix some of the off-by-statusbar-height bugs I've seen after launching an activity!

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to David Flanagan [:djf] from comment #13)
> In #2, my confusion was about how fullscreen works. If the app is
> fullscreen, I was thinking its iframe to have the same size as the screen. 

A fullscreen app comes with that, but a normal app that do |requestFullscreen()| later will not make it's frame full screen. |requestFullscreen()| will make the full screen element within that app full screen.

> But maybe that isn't how fullscreen works. I guess just the content element
> that requests fullscreen gets big, but the containing iframe retains its
> size. 

Yes.

Comment 15

[Alex Keybl [:akeybl]]

Mass Modify: All un-milestoned, unresolved blocking-basecamp+ bugs are being moved into the C3 milestone. Note that the target milestone does not mean that these bugs can't be resolved prior to 12/10, rather C2 bugs should be prioritized ahead of C3 bugs.

Comment 16

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #11)
> Comment on attachment 688146 [details]
> https://github.com/mozilla-b2g/gaia/pull/6798
> 
> The changes seems fine.
> 
> Alive, please test keyboard access of the on-top-of-mozFullScreen inline
> frame -- if keyboard works you have my r+. I cannot think of other possible
> side effect on top of my head right now, but do test those if you think of
> any.

Keyboard works by canceling mozFullScreen. I am not sure this is expected but is trying to make the inline activity frame not resized by keyboard.

Comment 17

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Alive Kuo [:alive] from comment #16)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #11)
> > Comment on attachment 688146 [details]
> > https://github.com/mozilla-b2g/gaia/pull/6798
> > 
> > The changes seems fine.
> > 
> > Alive, please test keyboard access of the on-top-of-mozFullScreen inline
> > frame -- if keyboard works you have my r+. I cannot think of other possible
> > side effect on top of my head right now, but do test those if you think of
> > any.
> 
> Keyboard works by canceling mozFullScreen. I am not sure this is expected
> but is trying to make the inline activity frame not resized by keyboard.

Just as I said keyboard works "as before" -- cancel the mozfullscreen. Changing this behavior -- if we really don't want to cancel fullscreen -- needs additional logic so in order not to introduce regression I propose not to do it in this bug. Going to merge now.

Comment 18

[(inactive after 6/18) Alive Kuo [:alive]]

https://github.com/mozilla-b2g/gaia/commit/c458b50507bba278518df48f88f768333bb1433f

Comment 19

[mlevin]

Verified fixed on Unagi build 20130103070201.