1180168 - Swiping an app to close it is janky and doesn't stop horizontal scrolling

Status: RESOLVED WORKSFORME

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

Description

[Chris Lord [:cwiiis]]

If you drag an app up to close it, it doesn't stop horizontal scrolling, which looks weird. Furthermore, it's also quite janky.

I suggest that after passing the vertical drag threshold, overflow-x should be set to hidden on the scrolling container - this will disable horizontal dragging.

Furthermore, if it isn't already, will-change: transform should also be set on the dragged app.

---

I was also pondering a technique that would be even smoother, whereby you use sticky positioning to set all apps except the centre app as fixed-position on the vertical axis, so you can just rely on vertical scroll position to swipe-close an app. This would be a lot more responsive and you could just rely on axis-lock to determine whether you're swiping horizontally or vertically (you'd likely still need the overflow-x trick above to counteract diagonal scrolling) - this could be a fun thing to experiment with, but the quick fixes are listed above.

Comment 1

[Chris Peterson [:cpeterson]]

Is this is a regression in the recent dogfood OTA update (Build ID 20150707033152)? The task manager seemed OK (to me) until my foxfood phone OTA updated.

Comment 2

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

(In reply to Chris Peterson [:cpeterson] from comment #1)
> Is this is a regression in the recent dogfood OTA update (Build ID
> 20150707033152)? The task manager seemed OK (to me) until my foxfood phone
> OTA updated.

Yeah, kind-of: the latest OTA pulled in bug 1161229, which improves the task-manager performance in lots of ways, but not the swipe-to-delete performance - which was not improved and reportedly sometimes a little worse. We have bug 1177823 on file which should improve it a little. I'm also interested in :cwiis' ideas though and would love to see a patch or prototype if anyone has time before I can get to it.

Comment 3

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

Marcus, do you want to start with this? You might also pull in or dupe bug 1177823. I have a patch underway for 1169012 that touches this code, but I would rather rebase it than block progress on this.

Comment 5

[Ting-Yu Chou [:ting] (away)]

Captured a profile on Nexus5 which drags the app up and down repeatedly.

  http://people.mozilla.org/~bgirard/cleopatra/#report=a08748785087738954cd2da85bb33f5c060fd614

Gecko: cb8bdb8ffaef
Gaia: ef691bb7e97bdbf709c3778c64363998be769a70

Comment 6

[Ting-Yu Chou [:ting] (away)]

(In reply to Ting-Yu Chou [:ting] from comment #5)
> Captured a profile on Nexus5 which drags the app up and down repeatedly.

Card.onCrossSlide() changes list item's translateY, but I am not sure why there're build displaylist/layer calls.

Comment 7

[Ting-Yu Chou [:ting] (away)]

- Calling preventDefault() in onCrossSlide() doesn't prevent horizontal scroll, since preventDefault() works only if it is touchstart/touchend or the first touchmove event [1]. But it's difficult to determine user is swiping horizontally or vertically at the first touchmove event.

- The dragging goes much smoother if preventDefault() whenever receiving a touchmove event. But it gets jankier when there're more cards.

[1] https://dxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#8058

Comment 8

[Ting-Yu Chou [:ting] (away)]

(In reply to Ting-Yu Chou [:ting] from comment #6)
> Card.onCrossSlide() changes list item's translateY, but I am not sure why
> there're build displaylist/layer calls.

While dragging, each nsRefreshDriver::Tick() triggers nsViewManager::ProcessPendingUpdates() since there're animation restyles (AFAIK those are CSS transitions) for elements such as:

  #lockscreen-panel-main,
  #lockscreen-masked-background,
  #AppChromeX

And there could be more after dragging more different apps' card view, I am still trying to figure out why is this happening.

Comment 9

[Ting-Yu Chou [:ting] (away)]

dbaron, I noticed css transitions will be added to pending restyle by CommonAnimationManager::AddStyleUpdatesTo() even:

  1) the element is not visible on screen (though css display is not none and visibilty is not hidden), and
  2) the transition property is not altered

Is this expected or a bug? Thank you.

Comment 10

[Ting-Yu Chou [:ting] (away)]

Clear NI since I filed bug 1189598.

Comment 11

[GitHub Autolander Bot]

Attachment: [gaia] sfoster:taskmanager-crossslide-bug-1180168 > mozilla-b2g:master

Comment 12

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

Comment on attachment 8641955 [details] [review]
[gaia] sfoster:taskmanager-crossslide-bug-1180168 > mozilla-b2g:master

A quick patch to address the side-scrolling while swiping up issue. I'm a bit worried toggling overflow-x mid-drag might trigger reflow and more jank, but the side-to-side stability is a net improvement I think. 

If this looks reasonable I suggest we move the other jank issues mentioned in this bug to other/new bugs and land to get in fixes incrementally

Comment 13

[Etienne Segonzac (:etienne)]

Comment on attachment 8641955 [details] [review]
[gaia] sfoster:taskmanager-crossslide-bug-1180168 > mozilla-b2g:master

Made a few comments on github but I think it's the right approach.

What's killing us (I think) is not the reflow but a huge repaint when we switch the overflowX to hidden, not much we can do about it sadly.

Comment 14

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

Re-assigning to :mcav as his WIP patch in bug 1206928 appears to fix this

Comment 15

[Marcus Cavanaugh [:mcav]]

Fixed in bug 1206928.