935260 - (task-manager) [Window Management] Rework CardView with new WindowManagement

Status: VERIFIED FIXED

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

Description

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

Currently CardView is doing some significant work.
IMO CardView should be very simple. It's just an UI reflection of current AppWindow instances list.

I'd like to move all app specific logic into AppWindow,
and if possible, just overlay the appWindow by the order of current stack instead of creating cards.

Comment 1

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

Make a mistake :/

Comment 2

[Etienne Segonzac (:etienne)]

Hey Aus, is there a duplicate somewhere or is this the bug you're working on?

Comment 3

[Aus Lacroix [:aus]]

Nope, as far as I know this is the only bug related to this. Although, I was expecting one from Product as well that contains the acceptance criteria and the likes.

Comment 4

[David Scravaglieri [:scravag]]

Ghislain, acceptance criterias are in bug 939809

Comment 5

[Aus Lacroix [:aus]]

Starting this today.

Comment 6

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

May I know the brief plan about how would do this?

IMO CardView shouldn't have too many codes, it just needs to call something upon appWindow.
It's more like "sheet-view-manager" that I mentioned an the summary.

Comment 7

[Gregor Wagner [:gwagner]]

Aus, can you attach your WiP here?

Comment 8

[Aus Lacroix [:aus]]

I attached the first relevant pull request to bug 939809.

Comment 9

[Eric Pang [:epang] UX]

Hey Aus,

Just wanted to add a link to the visuals specs.
They can be found here on box: https://mozilla.box.com/s/mx5roldjoln2iojemdgs

When revisions are made I'll be sure to let you know, but let me know if any specific specs are needed or if you have any questions, thanks!

Comment 10

[Aus Lacroix [:aus]]

Eric, the file has been removed from box, can you update the link?

Comment 11

[Eric Pang [:epang] UX]

(In reply to Ghislain Aus Lacroix [:aus] from comment #10)
> Eric, the file has been removed from box, can you update the link?

Hi Aus, sorry about that!  I reorganized to a different location on box.

You can find the specs here:

Portrait:
https://mozilla.box.com/s/nwr2w7g32sbkfjx0efau

Landscape:
https://mozilla.box.com/s/7wsuppddj8mewzkch5yr

Flow:
https://mozilla.box.com/s/tzxu9z9j5c1vs5y9y5z4

Let me know if anything else is needed :).

Comment 12

[Aus Lacroix [:aus]]

Nope! Thanks Eric! :)

Comment 14

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

I'm working on some steps towards this in bug 967420, which I've added as a dependency. I'll put an initial refactor pull request of the existing CardView on there - rather than here as that work isn't expected to go all the way to fixing this ticket.

Comment 15

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

Attachment: Rework/refactor cards view as TaskManager, Card, TaskCard

This is a continuation of work that started in bug 967420. The goal is to land the refactor without regressing the existing carousel-style/CardsView behavior and resolve this bug, then follow up in bug 967420, bug 967428 to get the new Task Manager feature complete and fully retire the CardsView. 

Until then, we've got a Card and TaskCard class and forks in TaskManager to handle both the new Haida case (behind an isRocketBar flag) and the existing CardsView case. Its expected Card will be short-lived and TaskManager can be simplified when the old behavior goes away, but allowing both to exist in parallel for now buys time to work through the details and gives us escape hatch if its not feature complete in time for whatever reason. 

Its quite a large patch so I'm sure I've missed something in there, but I've tried to address all previous comments.

Comment 16

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

Comment on attachment 8416807 [details] [review]
Rework/refactor cards view as TaskManager, Card, TaskCard

Only a little issues left + failed tests before r+ and land. Thank you Sam.

Comment 17

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

Aus, I'm going to work through addressing :alive's comments. I'll also update the patch to use the HAIDA build flag to set the isRocketBar flog (somehow). Let me know if you have anything to add. I had a couple specific questions: 

* In zindex.css, you had commented out the rule at https://github.com/sfoster/gaia/blob/task-manager/apps/system/style/zindex.css#L52. I've not seen the impact of leaving this rule out while still running the old CardView. Do we need this still, i.e. should I keep the rule but add a not(.task-manager) or override rule negating its effect when we're using the new task-manager vs. old cards view? 

* in the goToHomescreen method we currently stopImmediatePropagation: https://github.com/sfoster/gaia/blob/task-manager/apps/system/js/task_manager.js#L638 and :alive asks 'Do we expect task-manager to block home button?'. I'm not sure, do we?

Comment 18

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

I've updated the pull request. Think I hit just about all the suggestions. If there are things I can do in follow-up bugs to expedite landing this that would be great as I'm hitting daily bitrot. Like I should unit test those new AppWindow methods. Remaining known issues: 

* move onviewport, outviewport methods to TaskManager
* goToHomescreen, "Do we expect task-manager to block home button". Hrm, I don't know, do we? What would happen if we didn't stopImmediatePropagation()? Have needinfo'd :aus

I got applyStyle and unapplyStyle methods on AppWindow, which I think answers the request to have enterTaskManager and leaveTaskManager by symmetrical, with one clearly and cleanly undoing the effect of the other.

Comment 19

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

Comment on attachment 8416807 [details] [review]
Rework/refactor cards view as TaskManager, Card, TaskCard

PR updated (I accidentally closed/re-opened it).

Comment 20

[Aus Lacroix [:aus]]

(In reply to Sam Foster [:sfoster] from comment #17)
> Aus, I'm going to work through addressing :alive's comments. I'll also
> update the patch to use the HAIDA build flag to set the isRocketBar flog
> (somehow). Let me know if you have anything to add. I had a couple specific
> questions: 
> 
> * In zindex.css, you had commented out the rule at
> https://github.com/sfoster/gaia/blob/task-manager/apps/system/style/zindex.
> css#L52. I've not seen the impact of leaving this rule out while still
> running the old CardView. Do we need this still, i.e. should I keep the rule
> but add a not(.task-manager) or override rule negating its effect when we're
> using the new task-manager vs. old cards view? 

I think it's safe to ignore those changes I made. They were simply to try to get rid of some of the visual jank. It wasn't meant to be a real solution. :)

> * in the goToHomescreen method we currently stopImmediatePropagation:
> https://github.com/sfoster/gaia/blob/task-manager/apps/system/js/
> task_manager.js#L638 and :alive asks 'Do we expect task-manager to block
> home button?'. I'm not sure, do we?

I'm not sure about this one either. Currently, we handle the homescreen button ourselves and dismiss the task manager. I believe this is still what we want to do, so we would want to block the home button on purpose (which is why that stopImmediatePropagation is there). I would try and check the spec to see if there is any mention of change in behavior there, or ask Francis.

Comment 21

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

I tried your patch on my phone but found the transform doesn't work. The appwindow doesn't transform with card.
What's your plan? Do you want to replace card screenshot with real appWindow? I like this idea but the code seems dead now.

Comment 22

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

Comment on attachment 8416807 [details] [review]
Rework/refactor cards view as TaskManager, Card, TaskCard

Almost OK but something needs to be addressed in final review round:
(1) Unit test important functions for task manager. Also, task_manager_test.js is broken now.
(2) I'd like to know your plan for appWindow transform as I mentioned in previous comment.

But, still great work! Thank you for being patient.

Comment 23

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

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #22)
> Comment on attachment 8416807 [details] [review]
> Rework/refactor cards view as TaskManager, Card, TaskCard
> 
> Almost OK but something needs to be addressed in final review round:
> (1) Unit test important functions for task manager. Also,
> task_manager_test.js is broken now.
> (2) I'd like to know your plan for appWindow transform as I mentioned in
> previous comment.
> 
> But, still great work! Thank you for being patient.

Ok I'll check that out. I did have a clean unit test run and checked it out on my peak phone and all looked good before I put in the r? Just so you know I'm trying not to waste your time!

Comment 24

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

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #21)
> I tried your patch on my phone but found the transform doesn't work. The
> appwindow doesn't transform with card.
> What's your plan? Do you want to replace card screenshot with real
> appWindow? I like this idea but the code seems dead now.

Oh are you talking about the Card / !this.isRocketBar behavior? This is the behavior you see if you build *without* HAIDA=1. I've not make the same changes there as for the HAIDA=1 (this.isRocketBar path.) Maybe I should do that, but the idea was to leave this code basically as-is, just refactored to the new coding conventions. I'll take a look at what would be involved to do this in Card as well as TaskCard

Comment 25

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

(In reply to Sam Foster [:sfoster] from comment #24)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #21)
> > I tried your patch on my phone but found the transform doesn't work. The
> > appwindow doesn't transform with card.
> > What's your plan? Do you want to replace card screenshot with real
> > appWindow? I like this idea but the code seems dead now.
> 
> Oh are you talking about the Card / !this.isRocketBar behavior? This is the
> behavior you see if you build *without* HAIDA=1. I've not make the same
> changes there as for the HAIDA=1 (this.isRocketBar path.) Maybe I should do
> that, but the idea was to leave this code basically as-is, just refactored
> to the new coding conventions. I'll take a look at what would be involved to
> do this in Card as well as TaskCard

I will play with HAIDA=1 again, thanks. I am fine if it's haida only.

Comment 26

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

(In reply to Sam Foster [:sfoster] from comment #23)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #22)
> > Comment on attachment 8416807 [details] [review]
> > Rework/refactor cards view as TaskManager, Card, TaskCard
> > 
> > Almost OK but something needs to be addressed in final review round:
> > (1) Unit test important functions for task manager. Also,
> > task_manager_test.js is broken now.
> > (2) I'd like to know your plan for appWindow transform as I mentioned in
> > previous comment.
> > 
> > But, still great work! Thank you for being patient.
> 
> Ok I'll check that out. I did have a clean unit test run and checked it out
> on my peak phone and all looked good before I put in the r? Just so you know
> I'm trying not to waste your time!

Don't worry, travis wastes my time much often.

Comment 27

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

Pull request is updated, with requested unit tests, fixing some card swiping/selection regressions and the various nits addressed. Travis verdict still out but linting is green and the unit tests passed for me locally so fingers crossed that'll stay green

I've filed the following follow-ups: 
* Bug 1009805 - Complete renaming of CardView to TaskManager 
* Bug 1009813 - Move card viewport event listeners into TaskManager - 
* Bug 1009827 - Align logic and semantics in TaskManager show method
* Bug 1009793 - In TaskManager, use appWindow's instanceID not origin to lookup cards

..in addition to the existing bug 967428 where I plan to polish on the new card look/feel and transitions, and bug 967420 where I'll resume work on the launching transition.

To see the new task manager behavior (where we use the AppWindow element directly instead of capturing a screenshot) you'll need to build with HAIDA=1.

Comment 28

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

the last push had a unit test failure, and a couple of python ui test failure. I've push again with some fixes that hopefully with fix that unit test. But the ui tests - I've tried running those locally with both my task-manager branch and master branch - with fresh profiles - and I get the same failures. So I'm not sure what I'm doing wrong. Or, alternately why these are passing at all. 

Near as I can tell, the issue is the swipe_to_previous_app method in https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/apps/system/regions/cards_view.py#L58, if you watch the test running in b2g-desktop, you can see the card view doesn't change or advance, so the tap_app method taps on the wrong card, and wait_for_cards_view_not_displayed times out. I tried fiddling with the start and end coordinates for the flick with no success.

Comment 29

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

Rather, the call to is_app_displayed returns false for the Calendar app as the swipe didn't succeed leaving Clock in front. 

Zac, do you know this code at all? Can you see what I'm doing wrong or suggest next steps? If I knew how to make the test pass locally on master branch, I could be confident I did in fact break it and track down how. The latest travis job is at: https://travis-ci.org/mozilla-b2g/gaia/builds/25201264

Comment 30

[Zac C (:zac)]

Hey sfoster, 
In test_cards_view_with_two_apps it's simple - you've changed the order of the apps in the cards view! (intentional or not?)
Thus if you change the test to interact with the Clock app rather than the Calendar app it works fine.

For test_cards_view_kill_apps_with_two_apps, it's a little more complex but not greatly so. Currently we are waiting for 'transition' not to be in the style so we know it's stopped transitioning. however with your patch when it's stopped transitioning the style becomes transition: undefined so our wait never resolves to true. I guess this could be that the patch not supposed to be setting 'undefined' or we could modify what the test checks for when it considers `is_app_displayed`. (nb after this is fixed, you might need to change the order again)

btw, the flick up to remove the card is not working anymore?

Comment 31

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

thanks for that :zac, there were indeed some unintentional changes in there, but ultimately the transition style property not getting properly deleted was a legit regression that I've fixed in the most recent PR. I ran into a consistent problem with the swipe_to_previous_app method on cards_view.py though. Not sure if this is somehow related to the edge gestures, but on both master and my branch I was getting a failure resulting from this swipe gesture not succeeding. I was able to workaround by adjusting where the swipe originates: https://github.com/mozilla-b2g/gaia/pull/18377/files#diff-c277ab50102b302e9ec615d0f0c30e87L66 .. in actual use the sensistivity and swipeable area seems fine so I'm not sure why this test is so touchy. 

Will double-check the flick to remove card - it should be working fine..

Comment 32

[Zac C (:zac)]

That's possible; the swipe can be sensitive because it doesn't tap an element, it just taps at a coordinate and any element that happens to be at that coordinate will get the event. Although as you note the user is a bit wiser than that. Your work around seems fine to me.

PS I must clarify, for the flick up I was dragging-up with the mouse in desktopb2g so not a true flick. I did not build this onto my device.

Comment 33

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

Comment on attachment 8416807 [details] [review]
Rework/refactor cards view as TaskManager, Card, TaskCard

PR updated, travis just went green. Thanks in advance Alive, I know this is a large patch and represents a lot of changes to review. 

* Dont return self from start method
* Guard against no element in enterTaskManager, clear out .app and .manager references in Card.prototype.destroy()
* Removed dead code and tidy up z-indexing selectors
* Use bind instead of self=this
* Implement and use transform method on AppWindow
* Implement unapplyStyle, move settings change observer to registerEvents
* Store GestureDetector on the constructor to ensure we only ever instantiate one
* Fix up unit tests
* Use rocketbar.enabled setting to toggle isRocketbar property; unit test same
* More unit tests for isRocketbar behavior, testing setActive, hide
* allowManualClosing property to allowSwipeToClose, get rid of unused inTimeCapture, lastInTimeCapture
* Move opacity, transition, scale constants to card prototype. Use card.applyStyle to avoid touching the element.style directly. Fix pointer-events so current card should always be clickable. Fix adjacent card positioning so they occupy the 25% at the edge
* Adding unit tests for AppWindow apply/unapplyStyle, enter/leaveTaskManager and transform methods
* Add unit tests for card.destroy and taskManager.closeApp
* Resolve merge conflicts and add mock TaskManager back in to bootstrap test
* Twiddle dials on task-manager unit tests to better reset state between tests

* Adjust swipe_to_previous_app in cards_view ui-tests to start swipe from center
* Fix applyStyle to delete a property when the given value is undefined, which fixes the ui-test
* Fix swipe-up-to-close app regression
* Bump up waitForEvent timeout, fail early in tests if task manager is already showing
* Put back z-index for transitioning apps in cards-view case.
* Add unobserve call for rocketbar.enabled setting.
* Refine is(or is not)Rocketbar holdhome logic

Comment 34

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

What I saw in HAIDA=1:
* The black overlay of task manager disappears. You will notice task is directly on homescreen.
* The app opening transition from task-manager when clicking on the card is strange. There's large empty area on top during opening. I wonder this is because we don't clear the transform before opening.
* There's two closing transition when holdhome from app->task-manager..

Comment 35

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

Comment on attachment 8416807 [details] [review]
Rework/refactor cards view as TaskManager, Card, TaskCard

Basically r+. Please make sure we have followup for HAIDA=1 TaskManager polish as previous comment issues.

Thank you for your hard work.

Comment 36

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

Landed: https://github.com/mozilla-b2g/gaia/commit/83f622db256dad92e8c97cccf9c71611d0e5a737

Jason, this is a pretty large patch that touches most aspects of the cards view. I've already filed some follow-ups (blocked by this bug) but please be on the look out for other issues/regressions. The haida-style task manager is still toggled on the rocketbar.enabled setting, so while that is false you /should/ see no change in the cards view

Comment 37

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

Alive, I'm planning on addressing the HAIDA=1 display/transition issues in bug 967420. There are also other follow-ups blocked by this bug, please let me know or file more if you see any.

Comment 38

[Jason Smith [:jsmith]]

Sounds good. We'll have a full test run starting next week, so we'll be sure to catch the regressions at that point.

Comment 39

[Jason Smith [:jsmith]]

Closing this since this has landed now.

Comment 41

[Oliver Nelson [:oliverthor]]

Verifying fix on 3.0 for flame devices as all dependencies have been verified.
Results: TaskManager responds appropriately and resembles that of the doc retained here: https://mozilla.app.box.com/s/4pfp6y8pjhwojtt9yhl8/1/2243605137/20745078605/1 (minus bookmarks). Apps are embodied as cards that hold their last screenshot with minimal fuss between their launchings.

--------------------------------------------------
Environmental Variables:
Device: Flame 3.0
BuildID: 20150210010523
Gaia: 0cf517083f7eb5fc269e1236edba50534f65e3cd
Gecko: 2cb22c058add
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
--------------------------------------------------