Bugzilla

Comment 3

•

9 years ago

Jet, we probably need some fine tuning here.

Flags: needinfo?(bugs)

Updated

•

9 years ago

blocking-b2g: 2.5? → 2.5+

Jet Villegas (inactive)

Comment 4

•

9 years ago

These are the tune-able prefs for modifying scroll snapping behavior:
https://bugzilla.mozilla.org/attachment.cgi?id=8569537&action=diff

Sam, I don't have an Aries but will go looking for one. If you've already got one, please see if you can repro the bug and fiddle with the prefs above.

Flags: needinfo?(bugs) → needinfo?(sfoster)

Comment 5

•

9 years ago

Ok I had a fiddle, of the two prefs, layout.css.scroll-snap.prediction-sensitivity looks most interesting. I'm restarting after adjusting the value but THB I'm not seeing much difference: I've dialled it way down to < 0.1, and up to 1.0 and I still find it too easy to swipe past the next card. But I've spent a lot of time with the task manager at this point so I might be the wrong person to tune this - I've retrained my swipes so my experience might not be representative. Am I doing it wrong (I'm setting a value for the pref in webIDE, then restarting b2g) or maybe I'm misunderstanding the expected value range? If you can confirm, I can find someone else to take a look.

Flags: needinfo?(sfoster) → needinfo?(bugs)

:kip (Kearwood Gilbert)

Comment 6

•

9 years ago

Hi Sam,

Reducing layout.css.scroll-snap.prediction-sensitivity should generally cause the nearer snap point to be selected.  I'm not familiar with WebIDE; however, I use a bash script to quickly experiment with various preferences:

#!/bin/bash

PREFS_JS=$(adb shell echo -n "/data/b2g/mozilla/*.default")/prefs.js
adb pull $PREFS_JS
vim prefs.js
adb shell stop b2g
adb push prefs.js $PREFS_JS
adb shell start b2g

:kip (Kearwood Gilbert)

Comment 7

•

9 years ago

Also, I'd suggest that 0.1 be too short a value.  If you need to set the value less than 0.5 seconds or so, I'd recommend reducing the layout.css.scroll-snap.prediction-max-velocity preference instead.  You may find that there will be no difference when changing layout.css.scroll-snap.prediction-max-velocity in small increments, until you reach a threshold that clamps the distance based on the speed of the flinging motion.

Jet Villegas (inactive)

Updated

•

9 years ago

Flags: needinfo?(bugs) → needinfo?(sfoster)

Mahendra Potharaju [:mahe]

Comment 9

•

9 years ago

There's a video on the most recent dupe that illustrates this problem: https://www.youtube.com/watch?v=rihp8rGquqg

I tried again editing those prefs and again wasn't really able to discern a difference in behavior with the values I put in (using the shell script to edit and restart b2g) so I've suggested :mcav sit down with :jet at some point to review

Flags: needinfo?(sfoster)

Comment 10

•

9 years ago

Jet/Sam, 

There hasn't been any activity on this 2.5 blocker. Looking at comment 9, NI'ed to bring it up on your list.

Flags: needinfo?(sfoster)

Flags: needinfo?(bugs)

Comment 11

•

9 years ago

Redirecting to :mcav

Flags: needinfo?(sfoster)

Comment 12

•

9 years ago

Marcus, see Comment #9

Flags: needinfo?(m)

Updated

•

9 years ago

Whiteboard: [3.0-Daily-Testing], [Spark] → [3.0-Daily-Testing], [Spark][systemsfe]

Comment 14

•

9 years ago

I think our issue here is the current default behaviour of snapping. It feels like it's calculating where you'd end up, then picking the nearest snap point, where as really, we want to pick the closest snap point *before* where you'd end up. I also think some extra friction on top of this change of behaviour when snapping is enabled may also be desired.

cc'ing Botond, as it's an APZC bug and he may be able to help anyone that has the time to take this on.

Comment 15

•

9 years ago

(In reply to Chris Lord [:cwiiis] from comment #14)
> It feels like it's calculating where you'd end up, then picking the nearest
> snap point, where as really, we want to pick the closest snap point *before*
> where you'd end up.

Let me make sure I understand your proposal correctly.

Say we have the following:

S   |    |      |     X   | 
    A    B      C         D

where A, B, C, and D are snap points, S is your starting position, and X is where a fling would end up without snapping. Are you suggesting that, while right now we might snap to D (because it's closer to X than C), we should snap to C instead?

Comment 16

•

9 years ago

(In reply to Botond Ballo [:botond] from comment #15)
> (In reply to Chris Lord [:cwiiis] from comment #14)
> > It feels like it's calculating where you'd end up, then picking the nearest
> > snap point, where as really, we want to pick the closest snap point *before*
> > where you'd end up.
> 
> Let me make sure I understand your proposal correctly.
> 
> Say we have the following:
> 
> S   |    |      |     X   | 
>     A    B      C         D
> 
> where A, B, C, and D are snap points, S is your starting position, and X is
> where a fling would end up without snapping. Are you suggesting that, while
> right now we might snap to D (because it's closer to X than C), we should
> snap to C instead?

Exactly :) (plus also some extra friction) My rationale is that the page should never accelerate after you lift your finger, which is how this ends up feeling (relative to other scrolling)

Comment 17

•

9 years ago

OK, thanks for clarifying! That seems like a reasonable suggestion, and it seems like the scroll snapping spec gives us the flexibility to tweak things like this.

I'm not very familiar with the code that chooses a snap point based on a predicted destination, but I know it's located in ScrollFrameHelper::GetSnapPointForDestination() [1] , so looking at that should be a good starting point for someone who'd like to implement this adjustment. Kip, as the one who wrote this code, can probably provide more specific guidance.

[1] https://dxr.mozilla.org/mozilla-central/rev/607a236c229994df99766c005f9ec729532d7747/layout/generic/nsGfxScrollFrame.cpp#3599

Comment 18

•

9 years ago

(In reply to Botond Ballo [:botond] from comment #17)
> I'm not very familiar with the code that chooses a snap point based on a
> predicted destination, but I know it's located in
> ScrollFrameHelper::GetSnapPointForDestination() [1] , so looking at that
> should be a good starting point for someone who'd like to implement this
> adjustment. Kip, as the one who wrote this code, can probably provide more
> specific guidance.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> 607a236c229994df99766c005f9ec729532d7747/layout/generic/nsGfxScrollFrame.
> cpp#3599

Just been having a look at this - We'd need a way to get an indicator of what initiated the snap here - we don't want to change the behaviour when using the scrollbar or mousewheel, this is really only for flinging. Looking to see how involved this will be...

Comment 19

•

9 years ago

Actually, on further investigation, I think it may be desirable to have this behaviour always, regardless of whether it was a fling that initiated the scroll or not (except for when an absolute scroll position was requested, of course).

Comment 20

•

9 years ago

Attached patch 0001-Bug-1177842-Preserve-context-when-scroll-snapping-fr.patch (obsolete) — Details — Splinter Review

The first part of my suggestion was much easier than expected :)

It still feels too jumpy after this patch, I think (also as I suggested) that there needs to be extra friction as well. This can be done at the APZC level though, perhaps as a multiplier for the destination value?

Attachment #8673668 - Flags: review?(kgilbert)

Comment 21

•

9 years ago

Comment on attachment 8673668 [details] [diff] [review]
0001-Bug-1177842-Preserve-context-when-scroll-snapping-fr.patch

Actually, this doesn't seem to be working as intended, the behaviour is asymmetric... Looking into it.

Attachment #8673668 - Flags: review?(kgilbert)

Marcus Cavanaugh [:mcav]

Updated

•

9 years ago

Flags: needinfo?(m)

Marcus Cavanaugh [:mcav]

Comment 22

•

9 years ago

Chris, should this be assigned to you or are we holding off for a more definitive solution? Just want to make sure the unassigned blockers are still intentionally unassigned.

Flags: needinfo?(chrislord.net)

Comment 23

•

9 years ago

(In reply to Marcus Cavanaugh [:mcav] (Mozilla SF) from comment #22)
> Chris, should this be assigned to you or are we holding off for a more
> definitive solution? Just want to make sure the unassigned blockers are
> still intentionally unassigned.

I've been wanting to get to it, but purposefully haven't assigned because I have other, higher priority tasks to deal with. Whether I can get to it soon depends on how home screen blockers go, however. It would be good to get some platform assistance on these scroll-snapping issues.

My last findings were that my suggestion in comment #14 might not be actionable because scrolling is async but the snap point is fetched content-side, so I think really we just want to add extra friction when snapping.

Flags: needinfo?(chrislord.net)

Comment 24

•

9 years ago

(In reply to Chris Lord [:cwiiis] from comment #23)
> (In reply to Marcus Cavanaugh [:mcav] (Mozilla SF) from comment #22)
> > Chris, should this be assigned to you or are we holding off for a more
> > definitive solution? Just want to make sure the unassigned blockers are
> > still intentionally unassigned.
> 
> I've been wanting to get to it, but purposefully haven't assigned because I
> have other, higher priority tasks to deal with. Whether I can get to it soon
> depends on how home screen blockers go, however. It would be good to get
> some platform assistance on these scroll-snapping issues.
> 
> My last findings were that my suggestion in comment #14 might not be
> actionable because scrolling is async but the snap point is fetched
> content-side, so I think really we just want to add extra friction when
> snapping.

Do you mean that even if you change the content-side code to pick an earlier snap point, with current friction settings the compositor can have scrolled past it by the time content notifies the compositor of the snap point? Doesn't this still cause scrolling back to the content-chosen snap point?

Updated

•

9 years ago

Priority: -- → P3

Comment 25

•

9 years ago

(In reply to Botond Ballo [:botond] (at standards meeting Oct 19-24) from comment #24)
> Do you mean that even if you change the content-side code to pick an earlier
> snap point, with current friction settings the compositor can have scrolled
> past it by the time content notifies the compositor of the snap point?
> Doesn't this still cause scrolling back to the content-chosen snap point?

This is right - especially in the task manager, this frequently leads to you scrolling past 2 snap points, and snapping to the first, which felt very unnatural. I think the easiest thing to do here immediately is just apply a greater friction for snapping end-point prediction.

Updated

•

9 years ago

Attachment #8673668 - Attachment is obsolete: true

Comment 26

•

9 years ago

Attached patch 0001-Bug-1177842-Introduce-separate-friction-for-fling-sn.patch (obsolete) — Details — Splinter Review

Flagging for early review so that we can get it committed quickly if/when we need to.

This value seems ok, but I'll seek UX approval before committing.

What feels odd now is that the deceleration can be quite sudden (you can still scroll 2, 3 even 4 cards in the task manager if you're trying, but a casual fling will fling no more than 1 or 2). It almost feels like we want the spring effect when snapping, but let's keep it simple for now.

I think we could do much better here if the apzc was knowledgeable of snap points along an axis, rather than leaving it to content.

Attachment #8676851 - Flags: review?(botond)

Comment 27

•

9 years ago

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=75803f8e591d

When this completes, could UX give this a go and provide feedback on whether this is better/worse / whether we should tweak some more? The pref is 'apz.fling_snap_friction', but it isn't live, so tweaks will require restarts.

Flags: needinfo?(rmacdonald)

Comment 28

•

9 years ago

Comment on attachment 8676851 [details] [diff] [review]
0001-Bug-1177842-Introduce-separate-friction-for-fling-sn.patch

Review of attachment 8676851 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the requested change. Thanks!

::: b2g/app/b2g.js
@@ +1022,4 @@
>  pref("apz.fling_curve_function_y2", "1.0");
>  pref("apz.fling_curve_threshold_inches_per_ms", "0.01");
>  pref("apz.fling_friction", "0.0019");
> +pref("apz.fling_snap_friction", "0.01");

Please add the pref to modules/libpref/init/all.js as well.

Attachment #8676851 - Flags: review?(botond) → review+

Updated

•

9 years ago

Assignee: nobody → chrislord.net

Comment 29

•

9 years ago

Attached patch 0001-Bug-1177842-Introduce-separate-friction-for-fling-sn.patch — Details — Splinter Review

Comment addressed, carrying r+ - small tweak to value that feels better to me.

Attachment #8676851 - Attachment is obsolete: true

Flags: needinfo?(rmacdonald)

Flags: needinfo?(bugs)

Attachment #8678987 - Flags: review+

Elizabeth Hunt[:ezoehunt]

Comment 30

•

9 years ago

Pushed to inbound: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ed2fdc36728d

Status: NEW → ASSIGNED

Comment 31

•

9 years ago

This seems to resolve the primary issue of being too sensitive. I can now scroll in "smaller increments."

It feels however as if my scroll action has little effect on the device. E.g., I do what I think is a very long swipe and only move 2 cards. I do a small swipe and move 1-2 cards. So it does not feel like there is a relationship between my physical movement and the device's behavior. When I do manage to make the device scroll more than a few cards at a time, I do not know what I did differently to cause that. 

Not sure if this existed before or not: Move to the card to the farthest left. Scroll a very small amount as if you want Card #2 to move into the primary slot.
What happens: Card #2 bounces in (slightly) then bounces back to its original position so 1st card is still primary. 
Expected: "Move forward" 2nd card is in the primary slot. Maybe this is an issue to be resolved post 2.5...

Comment 32

•

9 years ago

Unfortunately there's not much we can do in terms of simple changes to remedy this :(

Currently snap points are dealt with in the Gecko thread and not on the compositor - so when you fling a scrollable view, the compositor (the bit handling input and drawing to the screen) asks the content thread (the bit rendering the web page, running JavaScript, etc.) to snap to the nearest scroll point to a predicted end-point. The content thread can take an arbitrary amount of time to respond to this request, and this time is often far less than the ideal of 'instant'. Once the compositor gets the response, it will smoothly animate to that snap point.

This patch artificially shortens that predicted end-point with an alternative friction value to be applied, but otherwise doesn't change anything - the result is that there's now a strong disconnect between the action and the result (relative to non-snap-point-scrolling), although the result is at least now a bit more user-friendly.

Ideally, I think we want snap points to be dealt with entirely on the compositor, and in addition to that, to be dealt with differently - I think that a better experience would be if snap points were treated as 'magnetic' points on an axis and the simulation changed accordingly. This would feel more natural and look better. If we wanted to make it harder to scroll past snap points, we'd increase that magnetism, instead of having a different friction value for containers using snap points.

This is a pretty big job though, and very much post-2.5.

Carsten Book [:Tomcat]

Comment 33

•

9 years ago

https://hg.mozilla.org/mozilla-central/rev/ed2fdc36728d

Status: ASSIGNED → RESOLVED

Closed: 9 years ago

status-firefox44: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → FxOS-S10 (30Oct)

Comment 35

•

9 years ago

ftr, I'm not really happy with the 'fix' I've checked in here, but I guess it will have to do for now.

I find with the setting I checked in, switching between horizontal panels on the home screen is now harder than it should be (and forget about the vertical home screen paging), but lower values made the task manager still reasonably tricky to use.

I think this really depends on bug 1219296 for a proper fix.

Comment 36

•

9 years ago

Attached patch backout-bug1177842 — Details — Splinter Review

This backs out attachment #8768987 [details]. After living with it for some time, this is a worse experience than the sensitive card view and negatively affects all other scroll-snapping call-sites.

We need a more comprehensive fix in apz for this.

Attachment #8682569 - Flags: review?(botond)

Comment 37

•

9 years ago

Gregor, I think we should back this out as the experience is considerably worse all-in-all (especially panning between the two home-screen panels). I think the blocker status here should be reconsidered. Either we increase friction for all scrolling to help here, or we punt until bug 1219296 is fixed and we can implement something better in apz.

Flags: needinfo?(anygregor)

Comment 38

•

9 years ago

Hmm, while I still think reverting this change is a good move, snapping between home-screen panels appears to have regressed from some other change.

Comment 39

•

9 years ago

(In reply to Chris Lord [:cwiiis] from comment #38)
> Hmm, while I still think reverting this change is a good move, snapping
> between home-screen panels appears to have regressed from some other change.

Filed bug 1221186 for this.

Comment 40

•

9 years ago

Let's reopen the bug if we're going to be landing another patch here.

Status: RESOLVED → REOPENED

Resolution: FIXED → ---

Updated

•

9 years ago

Attachment #8682569 - Flags: review?(botond) → review+

Comment 41

•

9 years ago

Got the ok from Gregor, pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/5dec2ebb1e71

Unblocking 2.5 on this, but let's continue to use this bug to track the real solution once bug 1219296 lands.

blocking-b2g: 2.5+ → ---

Flags: needinfo?(anygregor)

Keywords: leave-open

Updated

•

9 years ago

status-b2g-v2.5: --- → affected

status-firefox44: fixed → affected

Updated

•

9 years ago

Depends on: 1219296

Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)

Comment 42

•

9 years ago

Comment on attachment 8682569 [details] [diff] [review]
backout-bug1177842

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Janky behaviour with css scroll snapping
Testing completed: Manual testing completed
Risk to taking this patch (and alternatives if risky): Basically zero, this backs out a very last-minute patch.
String or UUID changes made by this patch: None

Attachment #8682569 - Flags: approval‑mozilla‑b2g44?

Comment 43

•

9 years ago

backout bugherder

https://hg.mozilla.org/mozilla-central/rev/5dec2ebb1e71

Mahendra Potharaju [:mahe]

Comment 44

•

9 years ago

Unassigning for now until the dependent bug is fixed.

Assignee: chrislord.net → nobody

Comment 45

•

9 years ago

Comment on attachment 8682569 [details] [diff] [review]
backout-bug1177842

Approved for 2.5

Thanks

Attachment #8682569 - Flags: approval‑mozilla‑b2g44? → approval‑mozilla‑b2g44+

Kartikaya Gupta (email:kats@mozilla.staktrace.com)

Comment 46

•

9 years ago

Uplift of the backout:

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/ac21a5ee9f5e

Josh Cheng [:josh]

Updated

•

8 years ago

status-b2g-v2.5: affected → fixed