1089615 - Background flashes between black and white when choosing 'Show windows' in the browser menu

Status: VERIFIED FIXED

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

Description

[Chris Lord [:cwiiis]]

[Blocking Requested - why for this release]: Visible and bad-looking glitch on common functionality

When choosing 'Show windows' from the browser menu, the transition between the browser and the window picker is janky and the background flashes between black and white. This does no occur on master.

Comment 1

[Gregor Wagner [:gwagner]]

Is this with SHB enabled?

Comment 2

[Chris Lord [:cwiiis]]

(In reply to Gregor Wagner [:gwagner] from comment #1)
> Is this with SHB enabled?

This is without SHB (which I forgot about when testing...)

Comment 3

[Gregor Wagner [:gwagner]]

Looks bad but too late to block on it. Sam is taking a look and see if there is a low risk fix.

Comment 4

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

The entry point for the only filtered view we have for task manager (browser-only) is https://github.com/mozilla-b2g/gaia/blob/v2.1/apps/system/js/task_manager.js#L846 - Pre-emptively adding the 'filtered' class to #cards-view looks like it might improve this. I'll prepare a patch.

Comment 5

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

Attachment: Github PR: Fade to white background when showing task manager in browser-windows mode * Apply filtered class on task manager earlier * Pass filter in cardviewbeforeshow event to enable fade out to white in homescreen.

Chris, can you give this patch a whirl. It seems to fix the issue for me, if you can confirm I'll put it in for review and request uplift

Comment 6

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

Comment on attachment 8512218 [details] [review]
Github PR: Fade to white background when showing task manager in browser-windows mode

* Apply filtered class on task manager earlier
* Pass filter in cardviewbeforeshow event to enable fade out to white in homescreen.

Aus, I know the filter method can return false, but do you see any risk in pre-emptively adding the class like this?

Comment 7

[Aus Lacroix [:aus]]

Comment on attachment 8512218 [details] [review]
Github PR: Fade to white background when showing task manager in browser-windows mode

* Apply filtered class on task manager earlier
* Pass filter in cardviewbeforeshow event to enable fade out to white in homescreen.

The changes look OK so far although I have some concerns noted in the PR.

Comment 9

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

I've updated the PR in response to Aus' suggestions and done some more testing. There is no flashing/flickering problem with this patch in all except one case that I've found: from the search app (new tab), tap the 'show windows' boxes button. 

In this one case we see a black background for a fraction of a second before the task manager shows up. I've tracked this down to https://github.com/mozilla-b2g/gaia/blob/v2.1/apps/system/js/task_manager.js#L417 in which we wait for the app to finish its closing transition before adding the 'cards-view' class to the #screen element. The black we are seeing is the #homescreen.appWindow > .fade-overlay.  

I don't want to mess too much with the animation sequence here if possible. I would also prefer to not propagate this 'browser-only' visual state into too many other areas of the codebase by e.g. special-casing CSS rules in window.css. But we do want this transition to be to white rather than black-then-white. Any suggestions Etienne?

Comment 10

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

I wonder if this has to do with the Search app being kinda like a browser window, but kinda not. If I insert a break-point in that finish callback, I can see the same issue when using the 'show windows' context menu item from an actual browser window. But it must have a minimal or zero-length transition because without the break point I don't see any flash of black.

Comment 11

[Chris Lord [:cwiiis]]

Tried applying the patch on this bug and I still see it flash black. Seems to be less of a flash now, and a fair few frames. The patch doesn't make any discernible difference to me.

Comment 12

[Etienne Segonzac (:etienne)]

(In reply to Sam Foster [:sfoster] from comment #9)
> Any suggestions Etienne?

I'm still getting the flash with the patch :/

I think we should pass the |filterName| in the detail field of the |cardviewbeforeshow| event and let the HomescreenLauncher add a "light" class accordingly on the homescreenWindow element (and remove it in |cardviewbeforeclose|).

Doesn't add that much complexity and it should fix the bug for good :)

Comment 14

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

Comment on attachment 8512218 [details] [review]
Github PR: Fade to white background when showing task manager in browser-windows mode

* Apply filtered class on task manager earlier
* Pass filter in cardviewbeforeshow event to enable fade out to white in homescreen.

Updated the PR to add the 'light' class to the homescreen's appwindow based on the evt.detail we pass through from task manager in the cardviewbeforeshow event. If we needed this patch at all in master, I would say we might add a method to AppWindow to set the fadeout color/class, but we dont and that would be a larger patch with broader risk. 

I'm not able to repro at all with this patch applied. I've also tested receiving a call while task manager is showing to ensure we clean up the 'light' class properly. I could add a unit test for that if you want.

Comment 15

[Etienne Segonzac (:etienne)]

Comment on attachment 8512218 [details] [review]
Github PR: Fade to white background when showing task manager in browser-windows mode

* Apply filtered class on task manager earlier
* Pass filter in cardviewbeforeshow event to enable fade out to white in homescreen.

Some comments on github, but this works very well :)

Comment 16

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

Comment on attachment 8512218 [details] [review]
Github PR: Fade to white background when showing task manager in browser-windows mode

* Apply filtered class on task manager earlier
* Pass filter in cardviewbeforeshow event to enable fade out to white in homescreen.

PR updated with new unit tests for HomescreenLauncher and TaskManager, and the requested changes/removals. Ran the tests with and without the functional changes and caught an issue where hide() wasn't been called in a previous suite so we were carrying DOM changes across suites. The new suiteSetup ensures we have a clean sheet for the browser-only/filtered tests

Comment 17

[Etienne Segonzac (:etienne)]

Comment on attachment 8512218 [details] [review]
Github PR: Fade to white background when showing task manager in browser-windows mode

* Apply filtered class on task manager earlier
* Pass filter in cardviewbeforeshow event to enable fade out to white in homescreen.

Thanks! r=me with an updated commit message

Comment 18

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

Comment on attachment 8512218 [details] [review]
Github PR: Fade to white background when showing task manager in browser-windows mode

* Apply filtered class on task manager earlier
* Pass filter in cardviewbeforeshow event to enable fade out to white in homescreen.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): The 'show browser windows' mode of task manager - which is new in 2.1
[User impact] if declined: When choosing 'Show windows' from the browser menu, the transition between the browser and the window picker looks janky and the background flashes between black and whit
[Testing completed]: On device testing in Flame/2.1. New unit tests to cover this change
[Risk to taking this patch] (and alternatives if risky): Low risk, the patch adds an existing CSS class to the task manager earlier than previously, and adds a class to the homescreen's fade out element as necessary. With the test coverage we have, I dont anticipate any fallout at all. 
[String changes made]: None

Comment 19

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

Addin ni? as this is a 2.1-only patch

Comment 20

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

Landed on v2.1: https://github.com/mozilla-b2g/gaia/commit/7154f9aa0a69af56c8bd89be0c98d9791449527b
Commit: 35df4d6f3f8bfad53bff3ce09377298d7a48c898

Comment 21

[Yeojin Chung [:YeojinC]]

This issue is verified fixed on Flame 2.1.

Result: No black background is shown while transitioning to the window manager.

Device: Flame 2.1 (319mb, KK, Shallow Flash)
BuildID: 20141113001200
Gaia: 569a299ca446f714cd98d5881cc058fd6f6e257b
Gecko: d188e92aa5a6
Version: 34.0 (2.1)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0