1058722 - [User Story] Filter non-browsing sessions from task manager when accessed from Browser button

Status: RESOLVED FIXED

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

Comment 1

[Gregor Wagner [:gwagner]]

Aus, can you help out here?

Comment 2

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

What is the plan here? Do we want to apply a filter somehow to the StackManager.snapshot() and put the existing task manager into a "browser-windows-only" mode? Or, show a different instance of TaskManager and feed it the stack we want to show? While TaskManager is instantiable, the assumption has always been that it is a singleton. To avoid getting into situations where 2 task managers are showing, or events are getting consumed by one and not the other, it might be better to provide a way to pass in the filter, or stack to populate it. 

Currently TaskManager is explicitly backed by StackManager. That's where we get the list of AppWindows to show, where we get the position in the stack from, is how we determine what the current app is to revert to etc. It sounds like what we need is a facade of sorts in front of StackManager that the TaskManager consults, rather than the StackManager itself?

Comment 3

[Aus Lacroix [:aus]]

Yep, I have a similar bug that would require filtering of the stack as well so I have a pretty good idea of how we can do this (and it happens to be eerily similar to Sam's ;)).

I can take care of this now that my other blocker has moved to our vendor.

Comment 4

[Aus Lacroix [:aus]]

Will try and get this in for FL.

Comment 5

[Peter Dolanjski [:pdol]]

Aus, Eric added the spec here: https://mozilla.box.com/s/iouyivjtc78t1lhrnbi7
Note that we want this applied to the existing task manager (left side of spec).  If Sam lands the new task manager visuals then we'll need to redo it with the new task manager.

Essentially the task manager looks generally the same but the background is white when launched from the Browser chrome.  Non-browsing windows are not shown at all when accessed in this manner.

Comment 6

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

At this point, there will be little to no changes necessary to adopt this into the new task manager, when it arrives. It should be trivial to merge.

Comment 7

[Aus Lacroix [:aus]]

I should have something ready sometime tomorrow (it should be easy, as Sam mentioned to apply it to old and new task manager, but it is based off of the *new* task manager).

Comment 8

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

We have degrees of newness. If you mean you're basing it off of bug 1047143, that has merged again (so you can rebase against master), hopefully it will stick this time. It implements some of the underlying changes needed for the haida-style task strip. But not the visual changes. At least from UX' point of view, its still the old task manager.

Comment 9

[Aus Lacroix [:aus]]

Attachment: Pull Request - Add ability to filter stack when invoking task manager

The button itself is taken care of in bug 1058096.

Example of how to use filtering:

var taskManagerShowEvent = new CustomEvent('taskmanagershow',
  { 
    detail: { 
      filter: function(app, index, apps) {
                // App Instance Object!
                if (app.isBrowser && app.isBrowser()) {
                  return true;
                }
                return false;
              }
}});

window.dispatchEvent(taskManagerShowEvent);

Comment 10

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

A couple drive-by comments in the PR.

Comment 11

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

Comment on attachment 8480861 [details] [review]
Pull Request - Add ability to filter stack when invoking task manager

I understand a filter function looks general but I dislike the way we are doing it, especially adding additional property to the array instance.

Since the spec is clear, could we just
taskManager.show(browserOnly) and filter by iterate 

this._stack.forEach(function(app) {
  if (app.isBrowser()) {
  } else {
  }
});

Also since you are invoking task manager from the browser window, the position should be correct no need to change.

Lemme know if I misunderstand the requirement.

Comment 12

[Aus Lacroix [:aus]]

Comment on attachment 8480861 [details] [review]
Pull Request - Add ability to filter stack when invoking task manager

I tried to incorporate as many of the review comments as possible. However, we definitely need update the position if we filter the array. The positions are expected to be contiguous as we will often do things like 'position - 1' or 'position + 1' to fetch the next card in the stack.

The CSS changes attempt to implement the design requested here: https://mozilla.app.box.com/s/iouyivjtc78t1lhrnbi7 (the one on the left, based on the *current* task manager).

Comment 13

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

Comment on attachment 8480861 [details] [review]
Pull Request - Add ability to filter stack when invoking task manager

r+ with nits and one open question:
What should be shown if there is no recent browserWindow?

We show 'No recent apps' for normal task manager without app opened, but do we expect to show that? Will that happen?

Ni ux

Comment 14

[Stephany Wilkes]

Reassigning ni? to Francis.

Comment 15

[Aus Lacroix [:aus]]

[Blocking Requested - why for this release]:

Without this, the user has no way to easily move between browser only windows making for a bad browsing experience. It would be pretty bad UX to ship without this.

Comment 16

[Francis Djabri [:djabber]]

I like the way Fennec handles empty states, as they're more instructive than what we have currently. 

Could we follow suit and change the browser tab manager empty state to: "Your recent browser windows show up here" and the normal task manager empty state to: "Your recent app windows show up here"

Comment 17

[Aus Lacroix [:aus]]

Comment on attachment 8480861 [details] [review]
Pull Request - Add ability to filter stack when invoking task manager

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Haida / System Browser work.
[User impact] if declined: Unable to navigate between browser windows easily.
[Testing completed]: On Device (Flame Full, Flame 319) + Unit Tests
[Risk to taking this patch] (and alternatives if risky): Low
[String changes made]: Added one, modified one.

Comment 18

[Aus Lacroix [:aus]]

Commit (master): https://github.com/mozilla-b2g/gaia/commit/472804d7de2797e87fb4cceb32d51aa1cfc10d6f

Leaving open as I am hoping to get 2.1 approval for this item and I want to keep it on my radar.

Comment 19

[Aus Lacroix [:aus]]

Pull Request for v2.1: https://github.com/mozilla-b2g/gaia/pull/23654

Comment 20

[Aus Lacroix [:aus]]

Commit (v2.1): https://github.com/mozilla-b2g/gaia/commit/a47ecb6368c015dd72148acde26413fd90ba3136

Fixed!

Comment 21

[Aus Lacroix [:aus]]

Actually, it looks like we'll need a very small follow-up to update the app chrome code in gaia to send the correct event data. PR incoming.

Comment 22

[Aus Lacroix [:aus]]

I'm unfortunately running into some weird issues with the CustomEvent used to trigger the task manager. I'm going to re-close this and open a follow up bug instead. Apologies for the bugzilla spam.