Closed Bug 1061324 Opened 10 years ago Closed 10 years ago

Rework cards view into cards strip with app chrome

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::System::Window Mgmt
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: etienne, Assigned: vingtetun)

References

Details

(Whiteboard: [systemsfe])

Attachments

(4 files, 2 obsolete files)

chrome.in.cardview.patch
159.85 KB, patch
sfoster
: review+
Details | Diff | Splinter Review
Follow up
46 bytes, text/x-github-pull-request
vingtetun
: review+
Details | Review
Follow follow up
46 bytes, text/x-github-pull-request
vingtetun
: review+
Details | Review
Demo video
1.79 MB, video/mp4
Details
Currently going from an app to the card view is pretty disturbing: performance issues aside the "cards" do not include the appchrome (titlebar/rocketbar/statusbar). This is especially bad for the "new tab app", which always has an expanded rocketbar *and* an easy access to the cardview. Hoping to get this in for 2.1
Attached file WIP (obsolete) —
Hey Sam, I took a look with Vivien this afternoon and he found a way to moz-element the appWindow. I think it's a good compromise between the ideal task strip and the current task manager. And it makes for a bite-sized patch. What do you think? PS: I really tried to find an existing bug but couldn't, feel free to format this bug to make it more SystemFE compliant :)
Assignee: nobody → etienne
Attachment #8482405 - Flags: feedback?(sfoster)
Attached patch cards.patch (obsolete) — — Splinter Review
Latest version. Less flashes. Sadly it regress fps - likely due to the overhead of moz-element + scale. Let's see if it can be improved...
Attachment #8482405 - Attachment is obsolete: true
Attachment #8482405 - Flags: feedback?(sfoster)
Hi Sam, thanks for the chat earlier. As I mentioned, I think this looks good, definitely an improvement over what he have and we should go for it. I've reviewed the patch with Rob and Jacqueline and here are what we agree are priorities for getting fixed: BLOCKERS Browser icons missing Need an empty state (Aus is working on this -"Your recent apps window show up here" ) High UX Priority It would be great if we could get free scrolling added - the flat style lends itself to the free scrolling Lower UX priority Framerate for dismissing card seems low, but no worse than before.
There are also some visual items that need working on. The title text looks too big and the background should be a scrim above the device wallpaper. I'll leave Eric to comment here. There's also going to be an impact on how we show the Tab Manager, so we should also take this into account. Eric, we've listed out some questions and issues on the Etherpad below, it'll also tell you how to you can install the patch: https://etherpad.mozilla.org/bug1061324uxfeedback
Flags: needinfo?(epang)
Francis/Eric: We know already we can't get this to spec for 2.1. The goal with this bug is to land a low-risk patch that improves the current situation and might be able to get uplifted. So we'll need to evaluate each visual and interaction detail in this context. Can we live with the solid color background for now? What if browser window icons prove difficult/slow/expensive - can we live with none or a default? If you edge swipe you'll notice we currently have no icons for browser windows. Vivien/Etienne: Adding cards (having more apps/windows open) seems to degrade scrolling performance noticeably - the hope was to use native scrolling for this. That makes the centering/snapping behavior dependent on bug 945584, but in the context of 2.1 automatically centering the card when scrolling was felt to be less valuable than the smooth native scrolling experience, and UX is ok without the snap-to-center for now. So it might be worth exploring setting overflow-x:auto on #cards-list rather than fighting with making the transform-based scrolling perform. We would still need y-axis movement for the close gesture. I had something like this working in a now-defunct branch https://github.com/sfoster/gaia/pull/5/files#diff-ee764fea0d2152491b89b7bf963a6e57R386 Browser windows missing icons: Currently we consult the manifest.icons for the card and swipe overlay icons. We have these nice helpers now for browser windows (IconsHelper.getBestIcon()) but I'm not sure that will be practical here - we may have to use a default or live without? I have to break here, but I can look at the native horizontal scrolling later tonight. The only other thing I see here as possible improvements is tracking how close a card is to the viewport and display:none/disable more stuff until it gets close - like the icon svg filter?
Ni? flags so it doesnt get lost in the noise
Flags: needinfo?(fdjabri)
Flags: needinfo?(etienne)
Flags: needinfo?(21)
(In reply to Francis Djabri [:djabber] from comment #3) > BLOCKERS > Browser icons missing > Need an empty state (Aus is working on this -"Your recent apps window show > up here" ) > > High UX Priority > It would be great if we could get free scrolling added - the flat style > lends itself to the free scrolling > As much as I would free scrolling, it will never be as smooth as you want without bug 950934, with is far from beeing doable for 2.1. Bug 950934, would generally offer a better scrolling experience for everything that is displayed in the system app (system wide list, notifications in the utility tray, etc...). I think we should consider speaking with Milan, to see what can be done for 2.2 for this particular point.
Flags: needinfo?(21)
(In reply to Sam Foster [:sfoster] from comment #5) > Francis/Eric: > > We know already we can't get this to spec for 2.1. The goal with this bug is > to land a low-risk patch that improves the current situation and might be > able to get uplifted. So we'll need to evaluate each visual and interaction > detail in this context. Can we live with the solid color background for now? > What if browser window icons prove difficult/slow/expensive - can we live > with none or a default? If you edge swipe you'll notice we currently have no > icons for browser windows. > > Vivien/Etienne: > > Adding cards (having more apps/windows open) seems to degrade scrolling > performance noticeably - the hope was to use native scrolling for this. That > makes the centering/snapping behavior dependent on bug 945584, but in the > context of 2.1 automatically centering the card when scrolling was felt to > be less valuable than the smooth native scrolling experience, and UX is ok > without the snap-to-center for now. So it might be worth exploring setting > overflow-x:auto on #cards-list rather than fighting with making the > transform-based scrolling perform. We would still need y-axis movement for > the close gesture. I had something like this working in a now-defunct branch > https://github.com/sfoster/gaia/pull/5/files#diff- > ee764fea0d2152491b89b7bf963a6e57R386 As I answered to UX, free scrolling will not be nice until we have APZ in the main process. Otherwise we will lack sub-pixel scrolling, the overflow effect, the physics will be different than the rest of Gaia, etc.. Let's continue with the approach we have for now and we will discuss with the GFX team to see what they can offer for the next release. > > Browser windows missing icons: Currently we consult the manifest.icons for > the card and swipe overlay icons. We have these nice helpers now for browser > windows (IconsHelper.getBestIcon()) but I'm not sure that will be practical > here - we may have to use a default or live without? Let's use a default for now if there is no icon yet for a particular web page. > > I have to break here, but I can look at the native horizontal scrolling > later tonight. The only other thing I see here as possible improvements is > tracking how close a card is to the viewport and display:none/disable more > stuff until it gets close - like the icon svg filter? I think the issue is with creating/destroying the card layer. will-change does not help much here because the card that are moved offscreen are discarded. We have some ideas with Etienne to optimize that, let's see how far we are tonight.
(In reply to Francis Djabri [:djabber] from comment #4) > There are also some visual items that need working on. The title text looks > too big and the background should be a scrim above the device wallpaper. > I'll leave Eric to comment here. > > There's also going to be an impact on how we show the Tab Manager, so we > should also take this into account. > > Eric, we've listed out some questions and issues on the Etherpad below, > it'll also tell you how to you can install the patch: > > https://etherpad.mozilla.org/bug1061324uxfeedback Hi Francis, I've added my comments to the etherpad. Sam, based on comment 5, I think we should try out the opacity over the wallpaper if possible and see how it preforms. Also, if apps have icons but browser screens don't this might end up causing confusion for the user. I think they are needed for consistency, let's see how they preform.
Flags: needinfo?(epang)
I've created a PR with some work on icons and background: https://github.com/vingtetun/gaia/pull/124 That gives us a default (branding) browser icon for browser cards, and where available we should get the page's icon/favicon. I set the page title at 1.4rem/14px rather than 1.3rem/13px the spec called for, for consistency with the size we currently use with the browser-only cards view. The background is dialed in at 80% black.
I also made a start on adding the favorite button, but its not ready. I think we should pursue that in its own bug rather than holding up this one.
Here's where we're at right now, there was some major effort during the last two days to keep performance good with a lot of apps open and the icon support is already merged in this branch. https://github.com/vingtetun/gaia/compare/tweak-cardsview?expand=1 TODO: - let's feedback/review each others first - let's ask UX to take another look - let's add the missing unit-tests And hopefully we'll finish the outstanding nits Monday (European time) to have it in formal review Monday (US time).
Flags: needinfo?(etienne)
(In reply to Etienne Segonzac (:etienne) from comment #12) > > And hopefully we'll finish the outstanding nits Monday (European time) to > have it in formal review Monday (US time). Sounds good, Etienne. FYI Francis and I reviewed the patch with Sam this afternoon and recommended that we use a generic browser icon instead of the favicons. The quality of the icons is still not at a level we're comfortable shipping with and in some instances the icon didn't match the website. The other key issue from the UX perspective was the missing empty state, which we'd like to see restored. Francis is at a conference on Monday and Tuesday so I'll be filling in on Task Switcher while he's away. I'll set time aside for 8am PST on Monday to review the patch and answer any questions. Obviously this is a priority for ux so we really appreciate the effort everyone has put into this. Thanks! Rob
Etienne, the empty state is the message we used to have in cards view when the stack is empty. Originally in the haida Task Manager spec that message was to go away and it would simply not open. But, in adding the browser-windows filter and the buttons in the browser to view all windows, it become important again and needs to stay. For my part, I'm going to strip back the icon support I added to leave just the .browser class that gives us the browser icon, and continue getting our unit tests back into shape. I hope to get a little time on this over the weekend, but failing that I'll connect with you Monday, Paris time.
(In reply to Sam Foster [:sfoster] from comment #11) > I also made a start on adding the favorite button, but its not ready. I > think we should pursue that in its own bug rather than holding up this one. This is correct. I also would like to compromise on the favorite button compared to what is in the spec. Let's discuss it on monday.
Heads up regarding bug 1064185 (filed a separate bug since it happens with all cards view versions). Vivien, Sam, be aware of this when doing performance tweaks since opening from an app or from the homescreen noticeably changes the fps results (and thus causing weird results if you try a small change but test it once from an app and once from the homescreen).
Looks great. I'm just testing a patch to remove the now-defunct isTaskStrip code branches and tests and I'll put in a new PR against vingtetun/tweak-cardsview. How do you want get proceed with review on this Vivien? I believe Alive is on PTO and (if its ready) we need to land today.
Flags: needinfo?(21)
Depends on: 1064185
Depends on: 1064397
Updating bug title to better reflect its scope. Unblocking from 1064185 which looks to be covered by the current patch - can dupe it when confirmed.
No longer depends on: 1064185
Summary: Display the app chrome in the card view → Rework cards view into cards strip with app chrome
Whiteboard: [systemsfe]
Comment on attachment 8486534 [details] [diff] [review] chrome.in.cardview.patch Review of attachment 8486534 [details] [diff] [review]: ----------------------------------------------------------------- See some comments and nits in the PR. But I want to say here also this is a huge improvement and long overdue refactoring of some very crufty legacy code. I've been using this for the last hour or so on my 319MB Flame and I've seen only one or 2 glitchy transitions that whole time. The try run shows a "TestCardsViewTwoApps.test_kill_app_from_cards_view | StaleElementException: StaleElementException: The element reference is stale. Either the element is no longer attached to the DOM or the page has been refreshed." which I've not seen before. With that fixed I think we're are good to go here.
Attachment #8486534 - Flags: review?(sfoster) → review+
Looping in Alive now that he's back from PTO. Alive, if you read from the top comments you'll see this patch is an interim solution that gets us 1) the statusbar/appchrome in the app cards, and 2) the horizontal-style task switcher. A lot of work has gone into getting smooth transitions to/from cards view and we've also been able to remove quite a lot of dead code along the way. The plan as it stands is to land this on master, and then see if it, or some subset of it can be uplifted to 2.1. It builds on the old card view and as such is a less risky, more incremental change. We should then re-group on the various bugs under bug 967420. There are still a number of interaction details to address to complete that and more under-the-hood work we'll want to do in 2.2. I still want to go forward with the plan to implement most of cards view as an AppWindow subcomponent, but this patch buys us time to do that, and the transitions work should carry over nicely. I'm sure you don't need an invitation to feedback or review, but please call out any concerns you have
Flags: needinfo?(alive)
(In reply to Sam Foster [:sfoster] from comment #22) > Looping in Alive now that he's back from PTO. Alive, if you read from the > top comments you'll see this patch is an interim solution that gets us 1) > the statusbar/appchrome in the app cards, and 2) the horizontal-style task > switcher. A lot of work has gone into getting smooth transitions to/from > cards view and we've also been able to remove quite a lot of dead code along > the way. The plan as it stands is to land this on master, and then see if > it, or some subset of it can be uplifted to 2.1. It builds on the old card > view and as such is a less risky, more incremental change. > > We should then re-group on the various bugs under bug 967420. There are > still a number of interaction details to address to complete that and more > under-the-hood work we'll want to do in 2.2. I still want to go forward with > the plan to implement most of cards view as an AppWindow subcomponent, but > this patch buys us time to do that, and the transitions work should carry > over nicely. > > I'm sure you don't need an invitation to feedback or review, but please call > out any concerns you have I quickly read the patch but don't see any problem. Thanks for letting me know. I am working on dependency issues recently so what I care is if we are introducing more improper dependency in the patch but it seems no.
Flags: needinfo?(alive)
I /wanted/ to fix that UI test, but after fighting with environment issues and getting nowhere I've got nothing to show for it. My thought was - as a first step - to change the wait_for_cards_view to something like self.wait_for_condition(lambda m: 'cards-view' in m.find_element(*self._screen_locator).get_attribute('class')) ..which is a more authoritative test for whether cards view has concluded showing. If no-one has got to it I'll pick this back up tomorrow.
> ..which is a more authoritative test for whether cards view has concluded > showing. If no-one has got to it I'll pick this back up tomorrow. Just took a stab at it, lets see if try likes it.
Try is green. Patch is r'+ed. Landing. https://github.com/mozilla-b2g/gaia/commit/b3698e7e93b5945b5aa5f857dd01932894f57a85
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee: etienne → 21
FYI - it seems that landing this has caused a fairly frequent unit test intermittent failure: https://tbpl.mozilla.org/php/getParsedLog.php?id=47783309&tree=B2g-Inbound Vivien or Etienne - can you look into this? I'll try to look shortly as well.
Flags: needinfo?(etienne)
Flags: needinfo?(21)
(In reply to Kevin Grandon :kgrandon from comment #27) > FYI - it seems that landing this has caused a fairly frequent unit test > intermittent failure: > https://tbpl.mozilla.org/php/getParsedLog.php?id=47783309&tree=B2g-Inbound > > Vivien or Etienne - can you look into this? I'll try to look shortly as well. Looking. thanks for noticing.
Flags: needinfo?(21)
I think I can reproduce it, working on a followup.
Attached file Follow up —
Attachment #8487289 - Flags: review?(21)
Flags: needinfo?(etienne)
Comment on attachment 8487289 [details] [review] Follow up r+ with one nit. Thanks for the quick followup.
Attachment #8487289 - Flags: review?(21) → review+
Depends on: 1066131
Depends on: 1066299
Depends on: 1066307
Depends on: 1065987
Attached file Follow follow up —
Sorry for the mess, making the whole test suite use fakeTimers now. (I'll make sure to retrigger a bunch of time anyway).
Attachment #8488480 - Flags: review?(21)
Depends on: 1068200
Depends on: 1054949
Depends on: 1088784
Cancelling stale ni? flags
Flags: needinfo?(fdjabri)
Depends on: 1111871
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: