1120974 - Opening card view with keyboard active results in zoomed card

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect)

Description

[Chris Lord [:cwiiis]]

Attachment: Facebook with stretched preview

STR:

- Open any app with a text-box (such as the messages app)
- Focus the text-box
- Long-press the home button

Expected:

Card view opens with preview of app.

Actual:

Card view opens with preview of app, stretched to the wrong aspect ratio.

Comment 1

[Chris Lord [:cwiiis]]

I would suggest that we structure our JS/CSS so that the app's preview always shows the entire screenshot, and always at the correct aspect ratio.

For this case with the keyboard, though we can't screenshot the keyboard, we could easily overlay an image of the keyboard on the screenshot. I think we should consider doing this too.

Comment 2

[Chris Lord [:cwiiis]]

Nominating for blocking as it's probably quite easily fixed, and it's very easy to encounter this bug (and it looks terrible - see screenshot for reference).

Comment 3

[Ben Francis [:benfrancis]]

Sam were you taking this one?

Comment 4

[Sam Foster [:sfoster] (he/him)]

Would welcome help. I can take it if no-one else has bandwidth.

Comment 5

[Sam Foster [:sfoster] (he/him)]

We talked about a couple of ideas on how to handle this. It occurs for both task manager and edge gesture.

1) Dismiss the keyboard before starting the edge gesture / card view animation. This is probably not an option as we wouldnt have an opportunity to animate the keyboard out
2) Omit the keyboard in the screenshot but ensure the screenshot doesn't get scaled/squished
3) Use a placeholder image for the (localized) keyboard

Alive, can you look at this, or make a recommendation?

Comment 6

[Julien Wajsberg [:julienw]]

(In reply to Sam Foster [:sfoster] from comment #5)
> We talked about a couple of ideas on how to handle this. It occurs for both
> task manager and edge gesture.
> 
> 1) Dismiss the keyboard before starting the edge gesture / card view
> animation. This is probably not an option as we wouldnt have an opportunity
> to animate the keyboard out


IMO we don't really care about the keyboard animation at this moment. But you'd still have to wait for a resize+reflow in the app, which take time.

> 2) Omit the keyboard in the screenshot but ensure the screenshot doesn't get
> scaled/squished
> 3) Use a placeholder image for the (localized) keyboard

May I add:
4) take a screenshot before the keyboard is displayed, and use that screenshot while the keyboard is displayed. Never take a screenshot while a keyboard is displayed.

> 
> Alive, can you look at this, or make a recommendation?

Comment 7

[GitHub Autolander Bot]

Attachment: [gaia] sfoster:task-manager-screenshot-proportion-bug-1120974 > mozilla-b2g:master

Comment 8

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8566366 [details] [review]
[gaia] sfoster:task-manager-screenshot-proportion-bug-1120974 > mozilla-b2g:master

From the PR: There are a few scenarios in which the aspect ratio of the screenshot and app-window (via -moz-element) arent aligned with the current window size which gives us the card size. The previous solution of doubling up the backgrounds on the screenshot element didnt give us any way to allow for that - all we can do is offset and size (scale) the backgrounds. This patch creates seperate elements for the -moz-element (I called it reflection, maybe projection is better? I dunno) and the actual screenshot blob. This allows finer control over sizing and seems-to-work-for-me (tm).

Comment 9

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8566366 [details] [review]
[gaia] sfoster:task-manager-screenshot-proportion-bug-1120974 > mozilla-b2g:master

The transition is off when you have a shorter window - such as when the keyboard was visible when you long-tapped home. Because of the 50% 50% transform-origin, it scales (shrinks) to a point that is 50% of its height - not the full window height. I.e. it ends up at some point above the middle, then snaps to the middle when the card view is shown. Ideally, we want something like: 

    transform-origin: calc({window.innerWidth} - 50%) calc({window.innerHeight} - 50%);

That should be robust for transitioning arbitrary window dimensions into card view. Unless we use a CSS variable for the window innerWidth/Height we're out of luck trying to do that in the stylesheet, we would need to set style.transformOrigin from js I think, probably in AppWindow.prototype.enterTaskManager?

Comment 10

[Sam Foster [:sfoster] (he/him)]

PR updated with the transformOrigin/enterTaskManager thing. Oddly, LayoutManager doesn't cache window.innerHeight anywhere, and layoutManager.height is not what we want - we want the total height which the card view will use, not the height of the current app. So I guess I'll need to add that on either LayoutManager or AppWindowManager (TaskManager does cache it, but only on first render)

Comment 11

[Etienne Segonzac (:etienne)]

Comment on attachment 8566366 [details] [review]
[gaia] sfoster:task-manager-screenshot-proportion-bug-1120974 > mozilla-b2g:master

> 1) Dismiss the keyboard before starting the edge gesture / card view
> animation. This is probably not an option as we wouldnt have an opportunity

We do dismiss the keyboard at the beginning of an edge gesture, and the transition is not that bad.
I like the windowReflection but it makes for a big change and I don't think it's the best experience to have a partial screenshot.

Dismissing the keyboard before entering the cardview is the best option imo. Just tested on device to make sure there was no big race condition issue and it's pretty solid :) (I'll send the patch in a few.)

Comment 12

[GitHub Autolander Bot]

Attachment: [gaia] etiennesegonzac:bug-1120974 > mozilla-b2g:master

Comment 13

[Etienne Segonzac (:etienne)]

Comment on attachment 8567970 [details] [review]
[gaia] etiennesegonzac:bug-1120974 > mozilla-b2g:master

What do you think?

Comment 14

[Etienne Segonzac (:etienne)]

Note: I tried to add an integration test but I have no idea how to check the size of the background image...

Comment 15

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8567970 [details] [review]
[gaia] etiennesegonzac:bug-1120974 > mozilla-b2g:master

Its great that dismissing the keyboard works - I should have thought to check what we did for edge gesture. This is a much simpler solution, for that I like it. But it still seems to me that we can't make any guarantees about the size/aspect ratio of the screenshot we'll get in cards view, so the doubled-up background is a bit brittle. I'm fine with postponing the app reflection / app screenshot change, I just suspect we'll come back to needing it. See bug 1128376 for another example. 

I'm not sure about the integration test either, but I can try some things tomorrow.

Comment 16

[Etienne Segonzac (:etienne)]

Comment on attachment 8567970 [details] [review]
[gaia] etiennesegonzac:bug-1120974 > mozilla-b2g:master

> I'm fine with postponing the app
> reflection / app screenshot change, I just suspect we'll come back to
> needing it. See bug 1128376 for another example. 

That's a really good point.
I'm going to try my luck once more and tackle both issues with this patch (otherwise it's just asking for conflict) but if there's yet another edge case we missed we can go the reflection route.

Comment 17

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8567970 [details] [review]
[gaia] etiennesegonzac:bug-1120974 > mozilla-b2g:master

Works great, and Gaia-Try looks good (bar a couple of unrelated, known intermittents). Thanks!

Comment 19

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8567970 [details] [review]
[gaia] etiennesegonzac:bug-1120974 > mozilla-b2g:master

Revising my r+ to f+ as I was testing for bug 1033652 and bug 1128376 and it looks like this patch will work best for non-portrait cases if we delete the rotatingDegree/landscape handling entirely in Card.js, from https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/card.js#L329 

When I remove all that code (isLandscape block as well as the rotate-n class) I get reasonable results with all the orientations. Cut the Rope is fighting us by doing transform:rotate(n) in a setTimeout, but we can address separately that in bug 1033652

Comment 21

[Etienne Segonzac (:etienne)]

Comment on attachment 8567970 [details] [review]
[gaia] etiennesegonzac:bug-1120974 > mozilla-b2g:master

(In reply to Sam Foster [:sfoster] from comment #19)
> Comment on attachment 8567970 [details] [review]
> [gaia] etiennesegonzac:bug-1120974 > mozilla-b2g:master
> 
> Revising my r+ to f+ as I was testing for bug 1033652 and bug 1128376 and it
> looks like this patch will work best for non-portrait cases if we delete the
> rotatingDegree/landscape handling entirely in Card.js, from
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/card.js#L329 

Yeah... the appWindow's screenshotOverlay is hidden when the orientation of the screenshot is different from the current orientation. So we probably broke this code when the new card design landed.

I've updated the PR. If we build "rotate" logic again it should be done at the appwindow level.

Comment 22

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8567970 [details] [review]
[gaia] etiennesegonzac:bug-1120974 > mozilla-b2g:master

Yep, looks good to me, works well. We're not glitch-free with landscape games but this is a big improvement

Comment 23

[Etienne Segonzac (:etienne)]

I can reproduce the test failure locally. Looks like a keyboard bug, will have a look today.

Comment 24

[Etienne Segonzac (:etienne)]

Comment on attachment 8567970 [details] [review]
[gaia] etiennesegonzac:bug-1120974 > mozilla-b2g:master

PR updated!

Alive, can you take a look?
We have a race on desktop where the keyboard reappears before the closing transition is even finished.
We get 2 inputmethod-contextchange (one for the blur, then a 'textarea' focus right after). I've seen this issue happen in multiple marionette tests.

Comment 25

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

(In reply to Etienne Segonzac (:etienne) from comment #24)
> Comment on attachment 8567970 [details] [review]
> [gaia] etiennesegonzac:bug-1120974 > mozilla-b2g:master
> 
> PR updated!
> 
> Alive, can you take a look?
> We have a race on desktop where the keyboard reappears before the closing
> transition is even finished.
> We get 2 inputmethod-contextchange (one for the blur, then a 'textarea'
> focus right after). I've seen this issue happen in multiple marionette tests.

Do you know what is causing the focus? Is it because of the app tries to focus the textarea again?
Could we put the blur() call in appTransitionController just like focus? Or we couldn't because of closing performance will be affected by blur() right away?

Comment 26

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

(In reply to Etienne Segonzac (:etienne) from comment #24)
> Comment on attachment 8567970 [details] [review]
> [gaia] etiennesegonzac:bug-1120974 > mozilla-b2g:master
> 
> PR updated!
> 
> Alive, can you take a look?
> We have a race on desktop where the keyboard reappears before the closing
> transition is even finished.
> We get 2 inputmethod-contextchange (one for the blur, then a 'textarea'
> focus right after). I've seen this issue happen in multiple marionette tests.

If the app does not focus itself but someone refocus it and causes the blur->focus this is a problem.
And I am curious why removeFocus call in KeyboardManager does not fix this..

Comment 27

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

Comment on attachment 8567970 [details] [review]
[gaia] etiennesegonzac:bug-1120974 > mozilla-b2g:master

Generally, I don't have big concern except blur/focus is called everywhere; hopefully hierarchy manager could take care of this but it seems to be a special case... however it'd be nice if we know what's causing the refocus.

Comment 28

[GitHub Autolander Bot]

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

Comment 31

[Hermes Cheng (inactive after July 27, 2015)]

Sam, could you uplift to 2.2? It is a 2.2 blocker. Thanks.

Comment 32

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8567970 [details] [review]
[gaia] etiennesegonzac:bug-1120974 > mozilla-b2g:master

[Approval Request Comment]
The patch blurs the keyboard before showing the task manager, and removes defunct orientation handling in the task manager

[Bug caused by] (feature/regressing bug #): Task Manager
[User impact] if declined: The screenshots seen in task manager when using the keyboard are distorted 
[Testing completed]: On device, Gaia-Trys
[Risk to taking this patch] (and alternatives if risky): Low risk. 
[String changes made]: None

Comment 33

[Ryan VanderMeulen [:RyanVM]]

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

Comment 36

[Shally Li（leave from Mozilla）]

This bug has been successfully verified on latest Flame v2.2&3.0.
See attachment: verified_v3.0.mp4
Reproduce rate: 0/5

Flame 2.2 build:
Build ID               20150308002503
Gaia Revision          166491b92278dc9e648f8d49ab02d9ca00d74421
Gaia Date              2015-03-06 18:26:27
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/a48af0b5a6e4
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150308.052515
Firmware Date          Sun Mar  8 05:25:25 EDT 2015
Bootloader             L1TC000118D0

Flame 3.0 build:
Build ID               20150308160204
Gaia Revision          fea83511df9ccba64259346bc02ebf2c417a12c2
Gaia Date              2015-03-08 06:36:28
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/eab4a81e4457
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150308.192120
Firmware Date          Sun Mar  8 19:21:31 EDT 2015
Bootloader             L1TC000118D0

Comment 37

[Shally Li（leave from Mozilla）]

Attachment: verified_v3.0.mp4