Sam Foster [:sfoster] (he/him) Assignee
	Description • 9 years ago

The new CSS scrolling features from bug 1071326 allow us to re-visit the implementation of the scrolling/panning of the task-manager/tab-view and fully implement the 'Switching Tasks/Panning the List' features spec'd in https://mozilla.app.box.com/s/eaqmiu8u4egc70csygje

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 1 • 9 years ago

Jet, a little bird told me you might have a patch in the works for this?

	Etienne Segonzac (:etienne)
	Comment 2 • 9 years ago

posting the branch here too just in case: https://github.com/jetvillegas/gaia/tree/APZC-card-view

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 4 • 9 years ago

I'm going to assume that Jet's branch is an experiment and not intended to land in gaia master. It looks very like what I had prototyped a while back, I'll grab his diff and see how it plays out here.

	Jet Villegas (inactive)
	Comment 5 • 9 years ago

(In reply to Sam Foster [:sfoster] from comment #4)
> I'm going to assume that Jet's branch is an experiment and not intended to
> land in gaia master. It looks very like what I had prototyped a while back,
> I'll grab his diff and see how it plays out here.

Feel free to play around with that. Two known issues:

1. cards could wrap incorrectly after a card is swiped up and away, likely due to a width calculation error in the JS
2. horizontal swipe is smoother (uses css scroll snapping) than the vertical swipe (uses JS.) Kip suggested trying to use a nested css scroll snapping div for the vertical case.

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 6 • 9 years ago

I've got panning and swipe-to-close working. There's a 100 tweaks and tests to fix, but this is ready for some initial feedback on the approach and implementation as it stands.

	Etienne Segonzac (:etienne)
	Comment 7 • 9 years ago

Comment on attachment 8614775 [details] [review]
PR: Use CSS Scroll Snapping in card view

In broad strokes (happy to discuss more on IRC):

* I don't think the nested scrollable divs will work (with the current platform implementation at least). You're always moving the cards horizontally at the same time you're moving the selected card vertically which is not a god experience.

I think we should go with touch handlers on each card (always listening, you should be able to swipe close one of the side cards too).
Because we're not using transforms as a way to position the cards anymore it'll be much easier than before. Cards won't snap to the center of the screen when they're dragged up like they do on master. The other issue on master is the huge threshold we have before moving a card up. We should instead use the angle like in the EdgeSwipeDetector [1] to quickly (ie after less than 10px in one direction) decide if it's a horizontal or vertical gesture. If it's vertical translateY() the card and preventDefault, otherwise let APZC work its magic.

* I _do_ think we should let all the snapping work to gecko. I tried without the _centerCurrentCard call and it was working very well for me. We may need to tweak the physic on the platform side (the tiniest fling will make you move 2 steps) but I never got stuck in a state where a card wasn't centered.

* If not highlighting the current card means we can do without the `TaskManager.ScrollHandler` I'd like to do so for this first patch. It's going to be a pain to test well and this PR should really be removing more code than it's adding :)


Hope the feedback doesn't sound too negative, the performance while panning *is* amazing!
This patch is going to make a lot of people happy :)


[1] https://github.com/mozilla-b2g/gaia/blob/3c3d9206ca6de0f85a6df8c12c3aec8e2800912a/apps/system/js/edge_swipe_detector.js#L350-355

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 8 • 9 years ago

Comment on attachment 8614775 [details] [review]
PR: Use CSS Scroll Snapping in card view

This is looking good / working well for me. I'm going to move on to fixing up the tests and clean up - there's a bunch more CSS rules can go, possibly more orphaned js. Let me know if you have any feedback at this stage.

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 9 • 9 years ago

Comment on attachment 8614775 [details] [review]
PR: Use CSS Scroll Snapping in card view

Yzen, this is WIP but with some clean-up and tests updated, close to something I want to land on master - hopefully early next week. The patch changes the way the cards are laid out (position: left) in the card strip, and how the strip is navigated (simply scrolling/overflow-x). I've left the 'wheel' event handling in TaskManager, but the other close-to-swipe handling is now in Card, so it might make sense to move that there also. That patch also largely removes the notion of a current card. There's still a getter which looks at which cards is centered in the viewport, but we don't maintain the 'position' property. I'm happy to make the necessary changes to restore/improve the a11y support here, but I would appreciate your input on how to test, and any other implications of this patch which I'm missing.

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 10 • 9 years ago

(In reply to Sam Foster [:sfoster] from comment #9)
> Comment on attachment 8614775 [details] [review]
> PR: Use CSS Scroll Snapping in card view
> 
> Yzen, this is WIP but with some clean-up and tests updated, close to
> something I want to land on master - hopefully early next week. The patch
> changes the way the cards are laid out (position: left) in the card strip,
> and how the strip is navigated (simply scrolling/overflow-x). I've left the
> 'wheel' event handling in TaskManager, but the other close-to-swipe handling
> is now in Card, so it might make sense to move that there also. That patch
> also largely removes the notion of a current card. There's still a getter
> which looks at which cards is centered in the viewport, but we don't
> maintain the 'position' property. I'm happy to make the necessary changes to
> restore/improve the a11y support here, but I would appreciate your input on
> how to test, and any other implications of this patch which I'm missing.

Hi Sam, thanks for flagging me, for the most part it works almost as the older version. Here are some issues I found:

* we use screen shot as an open app button (https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/card.js#L100) but we can reach it with screen reader (when exploring the screen by touch (EBT)) because it has pointer-events: none; style applied to it. This should not be the case, I believe.

* The part responsible for handling horizontal wheel events (https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/task_manager.js#L517-L524) is done differently now and it seems like the setVisibleForScreenReader is not set appropriately on the right card when the such event happens.

To test it, if you turn the screen reader on and get into cards view:
* you can explore by touch (by tapping and holding and moving your finger on the screen and seeing if screen reader picks up things underneath your finger)
* to generate wheel event, swipe with 2 fingers left or right and see if the card changes. 
What I observed with the patch: the visible[current] card remains the same, even though the position changes correctly
Expected: position changes and also because the visible[current] card gets updated, the screen reader shifts the focus to the new card automatically.

Hope this helps.

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 11 • 9 years ago

but we can reach it with screen reader (when exploring the screen by touch (EBT))  =>
but we can *not* reach it with screen reader (when exploring the screen by touch (EBT))

	Etienne Segonzac (:etienne)
	Comment 12 • 9 years ago

Comment on attachment 8614775 [details] [review]
PR: Use CSS Scroll Snapping in card view

Made a few comments but this is definitely a good base to polish and test :)

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 13 • 9 years ago

Comment on attachment 8614775 [details] [review]
PR: Use CSS Scroll Snapping in card view

Looks like I have a UI test failing, all seems well on b2g-desktop and AFAICT nothing substantive has changed that should throw off the integration tests so I'll have to dig into that. Can I get your review in the meantime? Since the last round, I got the unit tests fixed up and there have been mostly small changes to address your last feedback plus a little easing to make vertical movement a bit more deliberate.

	Etienne Segonzac (:etienne)
	Comment 14 • 9 years ago

Comment on attachment 8614775 [details] [review]
PR: Use CSS Scroll Snapping in card view

Made quite a few comments so I'll need one last look but we're really close!

Also flagging Rob (feel free to redirect) for ui-review, mainly for the border-radius on the cards. I remember adding it as a test initially and I'm not sure 1. if we still want it (it's not perfect on the bottom corners) 2. if it's the correct value.

When this land we should also file a follow up platform bug to tweak the snapping physic because it's _really_ hard to move only one card at a time. Like Nical says, "this like playing the card view in hard mode" :)

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 15 • 9 years ago

> When this land we should also file a follow up platform bug to tweak the
> snapping physic because it's _really_ hard to move only one card at a time.
> Like Nical says, "this like playing the card view in hard mode" :)

I'm working on revising the patch, in the meantime lets put this on Kip's radar as the existing patch is useful for reproducing this behavior (and the new one wont change that).

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 16 • 9 years ago

Comment on attachment 8614775 [details] [review]
PR: Use CSS Scroll Snapping in card view

Thanks for catching that stuff. PR update with: 

* Augment unit tests, 
* attempt fix in UI tests (working a bit blind here, I kinda see why it would fail but it passed locally(!))
* remove section.content element
* Minor refactor, in particular to move some magic/hard-coded numbers onto the prototype and calculated based on window/card dimensions. This should set us up better for landscape view and is a bit more robust for different viewport dimensions in general

With the unit tests, I ended up basically duplicating some of the position/centering logic in TaskManager so I'm not sure how valuable they really are. And we're having to mock scrollTo *and* window.innerWidth so maybe this stuff is better done in a marionette test? (perhaps in follow-up)

	Etienne Segonzac (:etienne)
	Comment 17 • 9 years ago

Comment on attachment 8614775 [details] [review]
PR: Use CSS Scroll Snapping in card view

r=me with the SHB css fix + the `closest`.
Great way to end the week! Hope we can get this into the spark build.

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 18 • 9 years ago

I got the SHB CSS fixes in (verified on my aries device) and fixed the UI test with new logic to ensure the card is centered in the viewport - which is what we want to know with/without the scroll-snap actually. Finished with some minor cleanup and comments (including filing bug 1174325 for enabling landscape orientation) 

Tests look good on treeherder: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=2be1637848116b56a293eed1b305766661333fa7 I did check in with Eli about the apparent coldlaunch time time regression in the raptor tests. I don't think that is accurate - this patch doesnt change the what and when of loading and initialization for the task manager. All the impact (positive I hope) is in runtime performance when the task manager is shown.

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 19 • 9 years ago

Master: https://github.com/mozilla-b2g/gaia/commit/1bf2da102560481748ff3f6202fbed5c4daa5832

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 20 • 9 years ago

Had to back this out - bisection for bug 1174810 points the finger at this patch. I'll look into it and try and get this back in asap.

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 21 • 9 years ago

The problem on the aries device (software home button) seems to be this rule: 

 #screen.software-button-enabled #cards-view {
-  height: calc(100% - var(--software-home-button-height));
+  /* clip the horizontal scrollbar */
-  bottom: var(--software-home-button-height);
+  height: calc(1.5rem + 100% - var(--software-home-button-height));
+  bottom: calc(var(--software-home-button-height) - 1.5rem);
 }
 
The new height value is designed to overflow by 1.5rem to ensure the horizontal scrollbar is clipped and hidden from view. Removing the 1.5rem fixes the bug on the aries device - but in the task manager now we can see the horizontal scrollbar at the bottom. I'm not able to reproduce on Flame, but https://bugzilla.mozilla.org/show_bug.cgi?id=1174810#c4 says it does albeit sporadically. Is this a graphics issue on aries or am I doing something wrong (we use the same trick elsewhere in gaia)

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 22 • 9 years ago

One more option - just show the horizontal scrollbars. They are quite nicely styled and to my mind/eyes don't look out of place here. And now that task manager is just a scrollable div, we would get the usability benefits of scrollbars. Snapping to center still works, it really is just an aesthetic thing. Thoughts Rob? I'll attach a video. 

The CSS/scrollbar-hiding technique may or may not be good practice. It is a hack, but I don't think it should cause this glitch. So to my mind there's a graphics bug in there, but I would rather re-land with a workaround and buy some time for investigating that.

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 24 • 9 years ago

Comment on attachment 8623096 [details] [review]
Small a11y improvement for the card.

Looks good, we'll need to wait to re-land the main patch though before I can add this - I'll probably roll that in at that time assuming a new patch is needed.

	Etienne Segonzac (:etienne)
	Comment 25 • 9 years ago

(In reply to Sam Foster [:sfoster] from comment #21)
> The problem on the aries device (software home button) seems to be this
> rule: 
> 
>  #screen.software-button-enabled #cards-view {
> -  height: calc(100% - var(--software-home-button-height));
> +  /* clip the horizontal scrollbar */
> -  bottom: var(--software-home-button-height);
> +  height: calc(1.5rem + 100% - var(--software-home-button-height));
> +  bottom: calc(var(--software-home-button-height) - 1.5rem);
>  }
>  

Just tried with all the SHB blocks removed.
* good news, it doesn't show the scrollbar, we don't care if the cards view continues below the SHB (the cards are top positioned)
* bad news, still seeing the gfx issue

I vaguely remember a similar issue, still digging...

	Etienne Segonzac (:etienne)
	Comment 26 • 9 years ago

Interesting, can't reproduce when I turn off the Hardware composer.
Maybe we shouldn't work around it in gaia :)

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 27 • 9 years ago

(In reply to Etienne Segonzac (:etienne) from comment #26)
> Interesting, can't reproduce when I turn off the Hardware composer.
> Maybe we shouldn't work around it in gaia :)

Yeah that's why i've need-info'd Sotaro - he has been working on a similar bug in 1171201, it may even be the same root cause.

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 28 • 9 years ago

Re-applies original commit (2be1637) with horizontal scrollbar clipping removed, and over-width (force scrolling/enable overscroll) for single card removed. I also rolled in the a11y (single-line) change. I've left these changes in their own commits for easier re-review, I can flatten when landing. 

The CSS changes mean that horizontal scrollbars *are* visible and usable in task manager. 

I've been playing with this on the aries device, and on low-memory Flame and have seen no issues. .

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 29 • 9 years ago

QA: this patch was previously backed out when it was implicated in bug 1174810. The new patch removes the offending CSS rules (see Comment #21). Could I get another check on aries and flame before attempting to re-land? 

UX: I'm open to discussion on the visible scrollbars. The specs omit them, I think there is a case to be made for leaving them in. Either way it will be a simple follow-up if we think its necessary. 

Clearing ni? for :sotaro, nothing further needed here.

	Etienne Segonzac (:etienne)
	Comment 30 • 9 years ago

Comment on attachment 8624960 [details] [review]
PR: Use CSS Scroll Snapping in card view (redux)

let's do this :)

	Pi Wei Cheng [:piwei] (inactive)
	Comment 31 • 9 years ago

Tested the patch on comment 28 on Aries, and bug 1174810 step 5 issue is still observed, as well as bugs that were duped to bug 1174810 are still occurring. So no, this does not fixed the issue.

I did not test in Flame because aside from bug 1174810 step 5 issue, I couldn't repro the bugs duped to it even before the patch.

Note that I was never able to reproduce bug 1174810 doing all the steps (at step 9 I did not observe the bug), that's why I've been referring to the bug as "bug 1174810 step 5 issue" because that's what I've been able to repro.

	Pi Wei Cheng [:piwei] (inactive)
	Comment 32 • 9 years ago

I think I understood this bug incorrectly on comment 31. I thought you were trying to fix bug 1174810 by landing a patch here. But I think what you're proposing is that the patch for this bug caused bug 1174810? If so that would be unlikely because I was able to reproduce this bug on today's nightly on Aries without the patch.

	Pi Wei Cheng [:piwei] (inactive)
	Comment 33 • 9 years ago

Correction:

> I was able to reproduce this bug on today's nightly on Aries without the patch.

I was able to reproduce *bug 1174810* on today's nightly on Aries without the patch from this bug.

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 34 • 9 years ago

Landed on master: https://github.com/mozilla-b2g/gaia/commit/9d267dd8dc3cf86c1c0536defc76d16ef1512fd3

I think the original patch was probably innocent, but this patch removes the scrollbar clipping that seems to have exacerbated the problem - we can restore that in a follow up if necessary. 

Rob: you are still need-info'd, let me know if you want to file that follow-up, or if we like it as-is.

	:kip (Kearwood Gilbert)
	Comment 35 • 9 years ago

(In reply to Sam Foster [:sfoster] from comment #15)
> > When this land we should also file a follow up platform bug to tweak the
> > snapping physic because it's _really_ hard to move only one card at a time.
> > Like Nical says, "this like playing the card view in hard mode" :)
> 
> I'm working on revising the patch, in the meantime lets put this on Kip's
> radar as the existing patch is useful for reproducing this behavior (and the
> new one wont change that).
Certainly changing the prefs that affect scroll snapping landing position may help:

layout.css.scroll-snap.prediction-max-velocity
layout.css.scroll-snap.prediction-sensitivity

There are some comments in all.js to help in selecting the right values:
https://dxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.js#2183

What could also be causing irregular snap position selection is if the main thread is extremely loaded.  In this case, the animation will appear smooth (animated off-main-thread via APZ); however, the scroll snap position may be calculated with too high of latency, resulting in a snap point that is calculated with information that does not represent the current scroll position well.

	Pi Wei Cheng [:piwei] (inactive)
	Comment 37 • 9 years ago

Regarding the verifyme tag added between comment 34 and comment 35, how do we go about to verify this? Is it just going through the specs sheet at comment 0 and make sure everything follows the specs sheet?

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 38 • 9 years ago

(In reply to Pi Wei Cheng [:piwei] from comment #37)
> Regarding the verifyme tag added between comment 34 and comment 35, how do
> we go about to verify this? Is it just going through the specs sheet at
> comment 0 and make sure everything follows the specs sheet?

Yes, just checking the "Switching Tasks/Panning the List" points in the spec would be a good verification.

	Pi Wei Cheng [:piwei] (inactive)
	Comment 39 • 9 years ago

(In reply to Sam Foster [:sfoster] from comment #38)
> Yes, just checking the "Switching Tasks/Panning the List" points in the spec
> would be a good verification.

Okay thanks. That would be page 7 on this PDF: https://mozilla.app.box.com/s/eaqmiu8u4egc70csygje

The favorite icon on tabs is not implemented so I'll disregard this feature.

Also, on the document it says "If the user taps on the Home key, the user is returned to the app from
which the Tab View was called. The Tabs in the Tab view should scroll to the right until the original Tab is centered and then the Tab should be relaunched."

I'm assuming this is basically saying if I'm in an app and invoke task manager, and I tap on Home button it should take me back to the app I was looking at instead of Home? That would be an incorrect behavior in my opinion and currently this incorrect behavior is not being followed on master.

Aside from that outdated specs, currently on Flame master we're observing bug 1172167 on 319MB memory. Also I noticed bug 1192046 still occurring on Flame and Aries and I think it looks bad.

The rest looks good and I'm marking this as verified.

Device: Aries 2.5
BuildID: 20150925134904
Gaia: 5a0a1bb1a863b07528ebc1a04652ed656d8333b3
Gecko: 543e1b3a25887258a70d1f2dab3cb287d0d500e4
Gonk: 2916e2368074b5383c80bf5a0fba3fc83ba310bd
Version: 44.0a1 (2.5) 
Firmware Version: D5803_23.1.A.1.28_NCB.ftf
User Agent: Mozilla/5.0 (Mobile; rv:44.0) Gecko/44.0 Firefox/44.0

Device: Flame 2.5
BuildID: 20150925030204
Gaia: f1cb3089ee2a77dd779408ca86dbb5e4ed5af0b3
Gecko: e5effeb8e57ceddf679f7784faab0c2cebb1e1e6
Gonk: c4779d6da0f85894b1f78f0351b43f2949e8decd
Version: 44.0a1 (2.5) 
Firmware Version: v18Dv4
User Agent: Mozilla/5.0 (Mobile; rv:44.0) Gecko/44.0 Firefox/44.0

NI Sam again to look at bug 1192046 and see if it could be fixed.

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 40 • 9 years ago

> I'm assuming this is basically saying if I'm in an app and invoke task
> manager, and I tap on Home button it should take me back to the app I was
> looking at instead of Home? That would be an incorrect behavior in my
> opinion and currently this incorrect behavior is not being followed on
> master.

Yeah correct, we changed this decision and the current behavior is good. 

> Aside from that outdated specs, currently on Flame master we're observing
> bug 1172167 on 319MB memory. Also I noticed bug 1192046 still occurring on
> Flame and Aries and I think it looks bad.

The rocketship icon is the fallback, shown if the app doesnt provide an icon or loading it fails. We could do a timer or assign some other style to the "pending" state maybe. I'll comment in that bug.