Closed
Bug 1209402
Opened 9 years ago
Closed 9 years ago
[TaskManager] Implement New Sheet and New Private Sheet
Categories
(Firefox OS Graveyard :: Gaia::System::Task Manager, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: mcav, Assigned: mcav)
References
Details
Attachments
(1 file)
46 bytes,
text/x-github-pull-request
|
etienne
:
review+
epang
:
ui-review+
|
Details | Review |
No description provided.
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Summary: Implement New Sheet and New Private Sheet → [TaskManager] Implement New Sheet and New Private Sheet
Comment 1•9 years ago
|
||
Assignee | ||
Comment 2•9 years ago
|
||
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
Attachment #8668143 -
Flags: ui-review?(epang)
Attachment #8668143 -
Flags: review?(etienne)
Assignee | ||
Comment 3•9 years ago
|
||
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.)
Attachment #8668143 -
Flags: review?(etienne)
Assignee | ||
Comment 4•9 years ago
|
||
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.)
Attachment #8668143 -
Flags: review?(etienne)
Comment 5•9 years ago
|
||
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 :)
Attachment #8668143 -
Flags: review?(etienne)
Comment 6•9 years ago
|
||
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!
Attachment #8668143 -
Flags: ui-review?(epang) → ui-review+
Assignee | ||
Comment 7•9 years ago
|
||
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.
Attachment #8668143 -
Flags: review?(etienne)
Comment 8•9 years ago
|
||
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)
Attachment #8668143 -
Flags: review?(etienne) → review+
Assignee | ||
Comment 9•9 years ago
|
||
master: https://github.com/mozilla-b2g/gaia/commit/258d6da499c384d62b865fccafb6ec208f14b67f
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•