1209402 - [TaskManager] Implement New Sheet and New Private Sheet

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Task Manager, defect)

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] mcav:tm-new-sheet > mozilla-b2g:master

Comment 2

[Marcus Cavanaugh [:mcav]]

Comment on attachment 8668143 [details] [review]
[gaia] mcav:tm-new-sheet > mozilla-b2g:master

This patch adds "New Sheet" and "New Private Sheet" buttons to the Task Manager.

Eric, the timings for the opening animation are located in window.css; search for "openAppFromNewCard". I chose values that seemed like a reasonable start; feel free to alter the values or suggest better ones. Note that the first delay keeps the card hidden, since for performance reasons the animation is triggered before we scroll to the to-be-opened card (see task_manager.js line 580).

Implementation Notes:

- To contain the buttons, #cards-view has been enclosed in a new #task-manager element. This led to a few changes to reference "this.scrollElement" when dealing with scroll-related properties, as well as a few CSS changes.

- We previously waited for AppWindow to dispatch "_opened"; this proved to be unreliable on Flame due to a gap between the opening transition finishing and the "_opened" event. We now wait for the appropriate "animationend" event, which addresses the problem on both devices and more correctly matches the intent of what we're waiting for. I've updated eventSafety accordingly.

- In RTL, the buttons are on opposite sides (thanks to flexbox).

Tests Added:

- Integration tests for each button
- Unit tests for each button
- Test for eventSafety's animationend handling

Comment 3

[Marcus Cavanaugh [:mcav]]

Comment on attachment 8668143 [details] [review]
[gaia] mcav:tm-new-sheet > mozilla-b2g:master

Upon waking up in the middle of the night, I realized that I haven't accounted for what happens if the user mashes (or double-taps) the new sheet buttons. I anticipate I'll need to add the appropriate transition flags to prevent new sheets from being opened after the button has been pressed once.

Clearing the review flag for now; will re-flag when I've addressed that. (ui-review can continue without that fix.)

Comment 4

[Marcus Cavanaugh [:mcav]]

Comment on attachment 8668143 [details] [review]
[gaia] mcav:tm-new-sheet > mozilla-b2g:master

As it turns out, we already do handle button-mashing correctly, since our "click" is removed immediately upon calling "hide()", and we call hide() synchronously. I've added a unit test to ensure we don't regress that protection. (Full patch notes in Comment 2.)

Comment 5

[Etienne Segonzac (:etienne)]

Comment on attachment 8668143 [details] [review]
[gaia] mcav:tm-new-sheet > mozilla-b2g:master

This is such a nice feature!
I'll take another quick look before r+'ing because of the private browsing issue but it should be quick :)

Comment 6

[Eric Pang [:epang] UX]

Comment on attachment 8668143 [details] [review]
[gaia] mcav:tm-new-sheet > mozilla-b2g:master

This is looking really good! 

Just a couple of issues, which will be addressed in a followup bug.

1. When a new tab is opened it currently shows a black status bar with white icons then updates to the light chrome with dark status bar icons.
- Is it possible to have the new tab match the end state?

2. Similar to above the same thing happens with a new private tab.  The chrome also switches the light chrome when it should be the private browsing purple.
- The private tab should match the state of a new private window.

Thanks!

Comment 7

[Marcus Cavanaugh [:mcav]]

Comment on attachment 8668143 [details] [review]
[gaia] mcav:tm-new-sheet > mozilla-b2g:master

I filed bug 1210453 about the dark-to-light app chrome problem.

The private browsing glitch was because the window wasn't actually private, as Etienne pointed out. This patch addresses that issue and adds a test to ensure it's actually opening a private window.

Unfortunately, BrowserConfigHelper is a bit of a mess; I think it should propagate "isPrivate" all the time rather than just 'if (!app)', but for now I just made our code match Browser's, since there isn't a centralized place to really call this logic and I'm not comfortable messing too much with BrowserConfigHelper right now.

The last thing noted is performance; Eric mentioned noticing some jankiness when there are a bunch of windows. I currently notice the jank when entering or leaving task manager, period, with a lot of windows, so I think that may be unrelated to this patch. I've filed bug 1210462 to investigate, unless you think it's substantial enough to address here.

Comment 8

[Etienne Segonzac (:etienne)]

Comment on attachment 8668143 [details] [review]
[gaia] mcav:tm-new-sheet > mozilla-b2g:master

Looking good!

The PR conflicts with a recent RTL patch but rebasing should be straightforward enough (and won't require an extra review round)

Comment 9

[Marcus Cavanaugh [:mcav]]

master: https://github.com/mozilla-b2g/gaia/commit/258d6da499c384d62b865fccafb6ec208f14b67f