Closed Bug 708765 Opened 13 years ago Closed 10 years ago

Add two finger scroll to pan as a percentage of the page

Categories

(Firefox for Android Graveyard :: General, defect, P3)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
ARM
Android
Type:
defect

Tracking

(blocking-fennec1.0 -, fennec+)

Status:
RESOLVED DUPLICATE of bug 721421
Tracking Flags:
Tracking Status
blocking-fennec1.0 --- -
fennec + ---

People

(Reporter: blassey, Assigned: toxophiliterry)

References

Details

Attachments

(2 files, 1 obsolete file)

Screen shot example of pop-up slider implementation
811.57 KB, image/png
Details
Second patch
6.89 KB, patch
kats
: feedback+
Details | Diff | Splinter Review 
The use case I'm trying to solve here is panning/scrolling long distances (top to bottom, bottom to top, top to middle) of a really long page. iOS has a double tap feature that accomplishes the bottom to top case, but I think that is easily confused with double tap to zoom.

So, I propose two finger scrolling scrolls a percentage of the page such that if you were to run two fingers from the bottom of the screen to the top you'd be able to scroll from the bottom to the top of an arbitrarily long page. Also, if you ran your fingers 50% of the screen height (top to middle or 1/4 to 3/4) you'd go from the top of the page to the middle.
Priority: -- → P3
tracking-fennec: --- → 11+
Assignee: nobody → margaret.leibovic
blocking-fennec1.0: --- → ?
blocking-fennec1.0: ? → -
tracking-fennec: 11+ → 16+
tracking-fennec: 16+ → +
I've been too busy to look into this for a long time. I think this could be a good mentor bug, but I'm not sure enough of how to implement it to be a good mentor. Wes, do you have ideas how we'd do this, or at least what part of the code to start poking around in?
I have a reasonably good idea of where to start with this, so I can mentor the bug if Wes doesn't want to.
Yeah, I'd be happy to mentor anyone on this too.
Is there any kind of design process to go through before implementation? My bug 793449 was added as a duplicate of this, although I made the suggestion of adding a hovering "go to top" bottom when scrolling up, so not really the same as Brad's second paragraph. There are other possible designs, such as a scroll thumb that could appear when a page is longer than a certain size. Personally, I would prefer to not use two-finger scroll, as I would like to be able to use the device one-handed (holding in one hand and using a thumb on the screen).
Adding ibarlow and uiwanted to flesh out the UX for this. John, feel free to comment with ideas on ways you think this should be implemented and we ccan discuss them to see which would be most appropriate.
Keywords: uiwanted
(Note that bug 721421 has another implementation suggestion)
Making both kats and wesj into mentors!
Assignee: margaret.leibovic → nobody
Whiteboard: [lang=java][mentor=kats][mentor=wesj]
Hi, I am searching for bugs to work on that will double as credit in my Software Engineering course at school. My partner and I decided that Mozilla seems like a good place to start, and this bug seems reasonable. I notice it has been a while since the mentors were assigned, but I was wondering if this issue is still open and whether we can be assigned to work on it. Thank you very much for your time.
Hi Terry, you're welcome to take this bug and work on it, but be warned that it is a little open ended and so if you have a time constraint this may not be the best bug to take on. However if you do still want to pursue it, you can get started by building Fennec using the instructions at https://wiki.mozilla.org/Mobile/Fennec/Android and post here or find us on IRC in #mobile, and we can guide you through what needs to be done.
Hi Kats, thank you for your quick response. I have spoken briefly with wesj as well. I think my teammate and I will take a leap of faith and stick with this bug. We have roughly 8 weeks for 2 bugs. Ideally, they will be from the same project (i.e. Mozilla or more specifically Mobile/Fennec). We are supposed to do one bug after another, so if we can find a more straight-forward bug we can do that one first thereby giving us more time and experience for this one. In any case, we are quite heavily invested in Mozilla and mobile already seeing as the proposal deadline is midnight of Wednesday the 16th, Pacific time.

So far (after a long night and half a day), I have built Fennec on my Linux machine (Ubuntu) and have successfully moved the APK to my tablet (Google N7). The app seems to be working, but I haven't play with it much yet. I would prefer to have the development tools built on my MacBook Pro instead since that laptop is my "daily driver". wesj tells me you have helped several people with the OS X build before, so I will pm you on IRC and see if you can help guide me through the rest of it.

I guess for right now, we are just trying to get the proposal done. The main item in the proposal that we need help with is some form of evidence to show that our development work will be accepted.
Straight from the proposal spec sheet: "Communicate with the team members to make sure that they are fine with you doing it and will accept it when possible. Find out what you need to give them in order for your code to be accepted. In most cases, it will be a patch file with working code and some tests."

The only question remaining, then, is "How is a bug fix or enhancement accepted?"
Building on OS X should be pretty much the same as building on Linux, except that you need to install XCode first. The instructions at https://wiki.mozilla.org/Mobile/Fennec/Android cover building on OS X; let me know if you run into any problems. Note that if you are using OS X 10.6 (or earlier) there is a known issue with building fennec that sometimes crops up, and there is no easy workaround that I'm aware of, so I would suggest keeping your Linux environment as well in case you run into that.

> The only question remaining, then, is "How is a bug fix or enhancement accepted?"

If you submit a patch that applies the codebase, doesn't cause any of the existing test suites to fail, implements the required functionality, and addresses all reviewer comments (which usually consist of technical and stylistic fixes), then we will "accept" your patch by landing it into the tree. Having tests for the code would be nice as well but I would still accept a patch without tests as testing this stuff properly can be pretty tricky at times and isn't something I expect new contributors to be able to do easily.
Assignee: nobody → toxophiliterry
Hello all,

It has been a while. Our class project consists of tackling two bugs, and we had to handle the other one first to give us more time with this one. As we close out the other one, we would like to get some ideas rolling and get started on this one early.

We have talked over which implementation would be best and here is what we ended up with:
We think it would be best to have a scroll bar show up whenever the user scrolls in quick succession. The user then has the choice to use this scroll bar to do percentage-based scrolling meaning the length of the scroll bar is proportional to the length of the page.

Cons of some of the other implementations mentioned:
-Percentage-based gestures across the whole screen is difficult (very explicit motion) on larger screens.
-It is also arguably too easy to trigger on smaller (phone) screens.
-Non-intuitive since it would be a hidden gesture that would need to be taught to the user. Confusion may result if they unknowingly skip to the middle or bottom of the page all the time.

Pros of the appearing scroll bar:
-Can be used with one hand (for smaller devices at least)
-Self-explanatory and natural in its usage
-Visible change (scroll bar pops up or fades in) when triggered
-More precision

Please let us know what you think of these ideas. We hope to decide on an implementation to work on within a couple weeks. Meanwhile we will start looking at the relevant code so any pointers in that direction would be most helpful.

Thank you.

-Terry
Status: NEW → ASSIGNED
(In reply to Terry from comment #14)
> We think it would be best to have a scroll bar show up whenever the user
> scrolls in quick succession.

I'm just an end-user here, but your idea sounds good to me. One thing I would add would be to only show the scrollbar when viewing a long page. I'm not sure of a definition of "long" yet, so this might take some experimenting. I would think it best to measure length by multiples of screen height (e.g. say that anything greater than 5 or 10 screen heights is long), rather than in absolute pixels.

What about a horizontal scroll bar for very wide pages? Very wide pages are rare and usually due to poor design, but can happen when viewing certain types of text files or log files on remote systems. It would be good if your solution would work for those too.
(In reply to Terry from comment #14)
> Cons of some of the other implementations mentioned:
> -Percentage-based gestures across the whole screen is difficult (very
> explicit motion) on larger screens.
> -It is also arguably too easy to trigger on smaller (phone) screens.
> -Non-intuitive since it would be a hidden gesture that would need to be
> taught to the user. Confusion may result if they unknowingly skip to the
> middle or bottom of the page all the time.
> 
> Pros of the appearing scroll bar:
> -Can be used with one hand (for smaller devices at least)
> -Self-explanatory and natural in its usage
> -Visible change (scroll bar pops up or fades in) when triggered
> -More precision
> 

I like this breakdown; it makes sense to try the scrollbar approach at least and see how it feels. I'm not sure how discoverable this will be though, since people aren't used to dragging scrollbars on touch interfaces, but it's still probably more discoverable than a two-finger scroll.

> We think it would be best to have a scroll bar show up whenever the user
> scrolls in quick succession. The user then has the choice to use this scroll
> bar to do percentage-based scrolling meaning the length of the scroll bar is
> proportional to the length of the page.

So with respect to the implementation, would this scrollbar be different from the one that already appears while panning? It sounds like you intend to detect multiple scrolls and then have a separate scrollbar show up (maybe replacing the existing one) that is draggable. Can you clarify?

Our existing scrollbar code, if you haven't found it yet, is located at mobile/android/base/gfx/ScrollbarLayer.java. The renderer code in mobile/android/base/gfx/LayerRenderer.java owns the two scrollbars and draws them as needed, and fades/unfades them as well.
OK, we finished the other bug so we can start on this one in earnest now.

Just to clarify on my last post, the idea was to make a little draggable tab pop up when scrolling in quick succession. It does seem to mar the beauty of touch displays, however, and we have since thought of other ideas.

From what I can tell, there is currently a little gray bar that looks like a scroll bar and appears when panning, but I think of it as more of an indicator since it simply shows where you are relative to the page size. It cannot be dragged to improve scroll efficiency.

In the interest of keeping the design clean and having something natural, would accelerated scrolling be appropriate? While playing around with browsers, I came across several others products that implement accelerated scrolling while also carrying the momentum of the scroll. For example, in Dolphin browser, it is easy to scroll from top to bottom of a long page with just a couple of swipes.

This may also save us from having to implement any graphical overlays. I realize we might be deviating a little bit from the original "shortcut" type suggestions, but it seems like this really is the most natural, most easily discovered way and it can be very efficient if done well. We figured two finger scrolling would undoubtedly create issues for features like zoom and the shortcut mentality makes it easier to skip past where you really want to be.

Please let us know what you think.
Accelerated scrolling sounds fine as well. Feel free to go ahead and put together a prototype that implements this; we can see how it feels and go from there.
I finally remembered where I had seen this implementation of a pop-up slider for full control of scrolling. This effect is found when opening a .pdf document in Dropbox on my iPhone.

Something similar to this is an option for improving extreme scrolling capabilities. It offers full control of the scrolling and can set you to a precise location within a split second. The primary drawback, of course, is that it clashes with some design ideals expected of touch devices now.
There are several Android apps that have a popup slider when scrolling large lists, e.g. SeriesGuide (https://play.google.com/store/apps/details?id=com.battlelancer.seriesguide), 920 Text Editor (https://play.google.com/store/apps/details?id=com.jecelyin.editor), F-Droid (http://f-droid.org/). I assume this is part of Android or the GUI toolkit, and actually find it quite annoying when other apps *don't* have popup sliders.
Refer to comment 19
Attachment #717804 - Flags: review?(bugmail.mozilla)
Ignore  the last line of Comment 21. Refer to this comment for the patch. Not comment 19.

OK, we found relevant code in "/src/mobile/android/base/gfx" within "JavaPanZoomController.java" and "Axis.java"

Played around with the friction and acceleration variables. This change improved momentum and acceleration, but removed too much friction.
Made the "LOW_FRICTION" threshold higher so that it would begin increasing the friction earlier.

Went on to add a boolean and an IF block in "updateWithTouchAt()" (commented in the code). The boolean checks for very high scroll speed and jumps to the top or bottom of the page depending on which direction the scroll was.

Finally, attempted to clarify and fix the acceleration implementation. The old one didn't make sense so if anyone could explain it, that would help.
Old code, Lines 195-196 of Axis.java:
float maxChange = Math.abs(mVelocity * timeDelta * MAX_EVENT_ACCELERATION);
mVelocity = Math.min(mVelocity + maxChange, Math.max(mVelocity - maxChange, newVelocity));

My maxChange divided by timeDelta instead of multiplying. I do not understand why it was multiplied to begin with. If timeDelta is in milliseconds, multiplication would make maxChange huge every time. This means "newVelocity" would always be picked by the second line that compares min's and max's...

My new IF block basically checks for a timeDelta within half a second (successive moves), and increases the velocity by the maxChange as long as the newVelocity is greater than the current one in the same direction by an amount of 200 units.

Hopefully, we're attacking the right area of code. I spent a long time tweaking values and rebuilding and testing, so I think I found a pretty comfortable setting. My partner tested on his phone and felt it was OK.

At this point, I do not expect the threshold jump feature to be kept as is, but we may use that logic for some kind of "Home" and "End" buttons or features like a desktop keyboard has. I mean, it works right now if you scroll fast enough, but it is hidden and not consistent so it's far from ideal.

We are open to suggestions at this point regarding how to proceed, but we only have a couple weeks left. I've attached our first patch along with an image of an example of an implementation mentioned earlier.
(In reply to Terry from comment #22)
> Finally, attempted to clarify and fix the acceleration implementation. The
> old one didn't make sense so if anyone could explain it, that would help.
> Old code, Lines 195-196 of Axis.java:
> float maxChange = Math.abs(mVelocity * timeDelta * MAX_EVENT_ACCELERATION);
> mVelocity = Math.min(mVelocity + maxChange, Math.max(mVelocity - maxChange,
> newVelocity));
> 
> My maxChange divided by timeDelta instead of multiplying. I do not
> understand why it was multiplied to begin with. If timeDelta is in
> milliseconds, multiplication would make maxChange huge every time. This
> means "newVelocity" would always be picked by the second line that compares
> min's and max's...

I think the code was correct as written. maxChange is the maximum allowed change in velocity, and the second line clamps the new velocity so that it is no farther than maxChange from mVelocity. The larger maxChange is, the more likely it is that the second line just evaluates as mVelocity = newVelocity.
Comment on attachment 717804 [details] [diff] [review]
First patch - acceleration implemented with top bottom velocity threshold jump

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

This is a pretty good first patch, thanks! You guys definitely found the right piece of code to be attacking, and did a pretty good job with implementing the actual scroll acceleration. I have some comments below.

::: mobile/android/base/gfx/Axis.java
@@ +96,5 @@
>  
>      static void setPrefs(Map<String, Integer> prefs) {
> +        FRICTION_SLOW = getFrameAdjustedFriction(getFloatPref(prefs, PREF_SCROLLING_FRICTION_SLOW, 800));
> +        FRICTION_FAST = getFrameAdjustedFriction(getFloatPref(prefs, PREF_SCROLLING_FRICTION_FAST, 980));
> +        VELOCITY_THRESHOLD = 25 / FRAMERATE_MULTIPLIER;

I don't have a problem with these changes in theory, but they will need to be tested on a wide range of devices. When I tried it on my Galaxy Nexus, it seemed like slower flings would stop quite abruptly but faster flings would go on much longer, so there's this mid-range fling that I could never quite hit.

@@ +97,5 @@
>      static void setPrefs(Map<String, Integer> prefs) {
> +        FRICTION_SLOW = getFrameAdjustedFriction(getFloatPref(prefs, PREF_SCROLLING_FRICTION_SLOW, 800));
> +        FRICTION_FAST = getFrameAdjustedFriction(getFloatPref(prefs, PREF_SCROLLING_FRICTION_FAST, 980));
> +        VELOCITY_THRESHOLD = 25 / FRAMERATE_MULTIPLIER;
> +        MAX_EVENT_ACCELERATION = getFloatPref(prefs, PREF_SCROLLING_MAX_EVENT_ACCELERATION, 20);

I don't think this change to MAX_EVENT_ACCELERATION is needed. If you disagree please let me know why.

@@ +184,5 @@
>      void updateWithTouchAt(float pos, float timeDelta) {
>          float newVelocity = (mTouchPos - pos) / timeDelta * MS_PER_FRAME;
>  
> +        // If there's a direction change, current velocity is very low,
> +        // or current velocity is very high: allow setting of the velocity outright. 

This comment doesn't match the code: if the current velocity is very high then you are spiking the current velocity and completely ignoring the new velocity.

@@ +190,3 @@
>          boolean curVelocityIsLow = Math.abs(mVelocity) < 1.0f / FRAMERATE_MULTIPLIER;
>          boolean directionChange = (mVelocity > 0) != (newVelocity > 0);
> +        boolean curVelocityIsHigh = Math.abs(mVelocity) > 270.0f / FRAMERATE_MULTIPLIER;

The "270" here will need to be pulled out and made a pref but for now we can leave it like this until we tune the behaviour a little more.

@@ +193,5 @@
>          if (curVelocityIsLow || (directionChange && !FloatUtils.fuzzyEquals(newVelocity, 0.0f))) {
>              mVelocity = newVelocity;
> +        } else if (curVelocityIsHigh) {
> +            // Spike acceleration by increasing mVelocity by 50%; sends the page to top or bottom
> +            mVelocity *= 1.5;

The behaviour that results from this seems pretty sane, but in terms of the code it looks weird that you're completely ignoring newVelocity. Perhaps augment the comment to say that the velocity keeps increasing until either the user lifts their finger and we go into the fling with friction, or the user moves their finger such that the first if condition in the block is satisfied.

@@ +200,5 @@
> +            // Then the velocity is increased by the maximum amount "maxChange"
> +            float maxChange = mVelocity / timeDelta * MAX_EVENT_ACCELERATION;
> +            if ((timeDelta < 500 && mVelocity > 0 && (newVelocity - mVelocity) > 200) ||
> +                (timeDelta < 500 && mVelocity < 0 && (newVelocity - mVelocity) < -200)) {
> +                mVelocity = mVelocity + maxChange;

As I explained in comment 23 I think this code was correct as written and doesn't need to be changed. If you disagree please let me know why.
Attachment #717804 - Flags: review?(bugmail.mozilla)
Attachment #717804 - Flags: review-
Attachment #717804 - Flags: feedback+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #23)
> (In reply to Terry from comment #22)
> > Finally, attempted to clarify and fix the acceleration implementation. The
> > old one didn't make sense so if anyone could explain it, that would help.
> > Old code, Lines 195-196 of Axis.java:
> > float maxChange = Math.abs(mVelocity * timeDelta * MAX_EVENT_ACCELERATION);
> > mVelocity = Math.min(mVelocity + maxChange, Math.max(mVelocity - maxChange,
> > newVelocity));
> > 
> > My maxChange divided by timeDelta instead of multiplying. I do not
> > understand why it was multiplied to begin with. If timeDelta is in
> > milliseconds, multiplication would make maxChange huge every time. This
> > means "newVelocity" would always be picked by the second line that compares
> > min's and max's...
> 
> I think the code was correct as written. maxChange is the maximum allowed
> change in velocity, and the second line clamps the new velocity so that it
> is no farther than maxChange from mVelocity. The larger maxChange is, the
> more likely it is that the second line just evaluates as mVelocity =
> newVelocity.

If maxChange = Math.abs(mVelocity * timeDelta * MAX_EVENT_ACCELERATION); then mVelocity - maxChange would always be negative since maxchange is mVelocity * timeDelta. Maybe if timeDelta was converted to seconds this would work.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #23)
> (In reply to Terry from comment #22)
> > Finally, attempted to clarify and fix the acceleration implementation. The
> > old one didn't make sense so if anyone could explain it, that would help.
> > Old code, Lines 195-196 of Axis.java:
> > float maxChange = Math.abs(mVelocity * timeDelta * MAX_EVENT_ACCELERATION);
> > mVelocity = Math.min(mVelocity + maxChange, Math.max(mVelocity - maxChange,
> > newVelocity));
> > 
> I think the code was correct as written. maxChange is the maximum allowed
> change in velocity, and the second line clamps the new velocity so that it
> is no farther than maxChange from mVelocity. The larger maxChange is, the
> more likely it is that the second line just evaluates as mVelocity =
> newVelocity.

To elaborate,

I may be missing something, but here is my breakdown of that code:
Assumption 1) timeDelta is in milliseconds (I traced it back to something along the lines of Android's getEventTime() that uses Java timeMillis())

Assumption 2) timeDelta is the time difference between user's touch events (i.e. between two successive scrolls, a shorter timeDelta equates to a faster scroll)

Problem #1) "maxChange" is composed of "mVelocity" multiplied by a raw "timeDelta" and an acceleration factor, both of which are much greater than one. Therefore, "maxChage" >> "mVelocity" and the nested line:
Math.max(mVelocity - maxChange, newVelocity)
will always return "newVelocity" since (mVelocity - maxChange) is very negative.

Problem #2) Related. Since "maxChange" is always some large, positive number, the outer value comparison is also useless as
Math.min(mVelocity + maxChange, newVelocity);
will also always return "newVelocity"

Essentially, that whole set of statements boils down to "mVelocity = newVelocity" and although that does not reveal any obvious problems during use, it effectively negates any chance of acceleration. This is why scrolling feels capped: it's because it IS capped by the "newVelocity" each time.

Now, I have yet to finish my IF/ELSE case that determines how much to accelerate which is why it stops so fast for the starting scrolls and low speed scrolls so far but I will put that in its own comment.

Please let me know if I am mistaken.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #24)
> Comment on attachment 717804 [details] [diff] [review]
> First patch - acceleration implemented with top bottom velocity threshold
> jump
> 
> Review of attachment 717804 [details] [diff] [review]:
> -----------------------------------------------------------------
> >  
> >      static void setPrefs(Map<String, Integer> prefs) {
> > +        FRICTION_SLOW = getFrameAdjustedFriction(getFloatPref(prefs, PREF_SCROLLING_FRICTION_SLOW, 800));
> > +        FRICTION_FAST = getFrameAdjustedFriction(getFloatPref(prefs, PREF_SCROLLING_FRICTION_FAST, 980));
> > +        VELOCITY_THRESHOLD = 25 / FRAMERATE_MULTIPLIER;
> 
> I don't have a problem with these changes in theory, but they will need to
> be tested on a wide range of devices. When I tried it on my Galaxy Nexus, it
> seemed like slower flings would stop quite abruptly but faster flings would
> go on much longer, so there's this mid-range fling that I could never quite
> hit.
> 

I agree this will require some testing across devices to be absolutely sure it doesn't introduce new problems, but it seems like there is a lot of code already in place to keep things relative. The units are a bit arbitrary...

Anyway, I am seeing the same case on my tablet, and I believe that has to do with my unfinished IF block in the updateWithTouchAt(). I still need to add the ELSE block that sets "mvelocity = newVelocity" if it does not satisfy the IF portion, but I got too tired last night by the time I realized it was missing.

Another idea is to create a middle range of velocity to smooth things over even more, but this may not be necessary if I correct the acceleration portion.

> @@ +97,5 @@
> >      static void setPrefs(Map<String, Integer> prefs) {
> > +        FRICTION_SLOW = getFrameAdjustedFriction(getFloatPref(prefs, PREF_SCROLLING_FRICTION_SLOW, 800));
> > +        FRICTION_FAST = getFrameAdjustedFriction(getFloatPref(prefs, PREF_SCROLLING_FRICTION_FAST, 980));
> > +        VELOCITY_THRESHOLD = 25 / FRAMERATE_MULTIPLIER;
> > +        MAX_EVENT_ACCELERATION = getFloatPref(prefs, PREF_SCROLLING_MAX_EVENT_ACCELERATION, 20);
> 
> I don't think this change to MAX_EVENT_ACCELERATION is needed. If you
> disagree please let me know why.
> 

Not necessarily needed, but the way I have maxChange set up, the Acceleration factor will be the primary determinant in how much is added to the velocity since I divide by timeDelta. All issues pertaining to that segment of code are kind of interrelated.

> @@ +193,5 @@
> >          if (curVelocityIsLow || (directionChange && !FloatUtils.fuzzyEquals(newVelocity, 0.0f))) {
> >              mVelocity = newVelocity;
> > +        } else if (curVelocityIsHigh) {
> > +            // Spike acceleration by increasing mVelocity by 50%; sends the page to top or bottom
> > +            mVelocity *= 1.5;
> 
> The behaviour that results from this seems pretty sane, but in terms of the
> code it looks weird that you're completely ignoring newVelocity. Perhaps
> augment the comment to say that the velocity keeps increasing until either
> the user lifts their finger and we go into the fling with friction, or the
> user moves their finger such that the first if condition in the block is
> satisfied.
> 

This was not exactly the behavior I had in mind as I simply wanted to implement acceleration. I later found out that acceleration is already implemented, but was perhaps not working so well do to the IF/ELSE block with nested Max/Min comparisons as I covered in a comment above.

I think if we can properly implement acceleration a little better, this code will not be needed at all unless we want to explicitly make a separate button or gesture that performs 'home' and 'end' jumps.
(In reply to Terry from comment #26)
> Problem #1) "maxChange" is composed of "mVelocity" multiplied by a raw
> "timeDelta" and an acceleration factor, both of which are much greater than
> one.

the acceleration factor is 0.012 by default (unless you changed the pref), and so unless timeDelta is 83.3 or more, maxChange will be less than mVelocity.
Also FWIW you may wish to read through https://bugzilla.mozilla.org/show_bug.cgi?id=705114#c0 which explains why that code was added initially (in a rather different form a few refactorings ago). I'm fine with changing some of that behaviour as long as it doesn't break that original scenario.
(In reply to Terry from comment #27)
> I agree this will require some testing across devices to be absolutely sure
> it doesn't introduce new problems, but it seems like there is a lot of code
> already in place to keep things relative. The units are a bit arbitrary...
> 
> Anyway, I am seeing the same case on my tablet, and I believe that has to do
> with my unfinished IF block in the updateWithTouchAt(). I still need to add
> the ELSE block that sets "mvelocity = newVelocity" if it does not satisfy
> the IF portion, but I got too tired last night by the time I realized it was
> missing.
> 
> Another idea is to create a middle range of velocity to smooth things over
> even more, but this may not be necessary if I correct the acceleration
> portion.

Ok. I'm not entirely sure what you're planning to change the if/else block to but I'll wait for an updated patch.

> > I don't think this change to MAX_EVENT_ACCELERATION is needed. If you
> > disagree please let me know why.
> > 
> 
> Not necessarily needed, but the way I have maxChange set up, the
> Acceleration factor will be the primary determinant in how much is added to
> the velocity since I divide by timeDelta. All issues pertaining to that
> segment of code are kind of interrelated.

See comments 28 and 29. I have a feeling that maybe you misinterpreted what this MAX_EVENT_ACCELERATION code was for.

> This was not exactly the behavior I had in mind as I simply wanted to
> implement acceleration. I later found out that acceleration is already
> implemented, but was perhaps not working so well do to the IF/ELSE block
> with nested Max/Min comparisons as I covered in a comment above.

We do not have acceleration already implemented. That is, in our existing code, the page will never move faster than the user's finger. As long as the user's finger is on the screen, the page will track the finger exactly, and when the user lifts their finger the page will only ever get slower. With an "acceleration" implementation I would expect the page to go faster than the user's finger while the user's finger is down (and moving quickly).

Does that make sense?
Hey Terry/Daniel, are you guys still working on this?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #31)
> Hey Terry/Daniel, are you guys still working on this?

Yes. We are still working on this. We should have a new patch file up soon.
Yes, sorry for the delay. It's the end of the quarter and things have been hectic. 

Your comments helped a lot, and I was mistaken about the logic in the updateWithTouchAt() code. I missed the part where the preferences are converted into float values. I have since printed out Axis.java for a more thorough code review and it all makes more sense now.

I believe we can implement most of the acceleration within updateWithTouchAt() and minimize the amount of change to the rest of the established code and preferences. I fell asleep while working on this last night, but will certainly have another patch uploaded before the weekend is over.

Just a heads up, our acceptance deadline for this bug is now set for this upcoming Wednesday. I know it's running very close, and I do not expect it to work out perfectly but I figure it would help to know what kind of schedule we're working with. In any case, I plan to see this bug through regardless of the project deadlines and whatnot.

Stay tuned :)
Sorry, I lied. I underestimated how busy this weekend would be for some reason. I have 70% of one class grade and 60% of another all dependent on a few final deliverable items, so I have been stressing out over those.

The new patch will most likely be up by Tuesday night. Unfortunately, that only leaves us with roughly 24 windows to get it accepted, so we'll probably forfeit the points for that. I do plan get it done though.
Scratch that. Spent most of the night struggling with technical difficulties because the fix I am trying is breaking something and I thought it was a build problem so I re-cloned the entire repository and did a fresh build.

We're obviously not making the deadline, but I will keep working on this to try to get it done anyway. I'll probably have time to continue working on this over my Spring break as well.

Working on a second patch now.
Attached patch Second patch — — Splinter Review 
A quick breakdown: created a new pref for FRICTION_FASTER to add one more layer of complexity in terms of filtering for velocity. Now, when the scrolling velocity is 3.5 (arbitrary, can be set as pref later) times that of the VELOCITY_THRESHOLD, it will decrease friction by retaining 990/1000 of the velocity for each frame I believe.

Within the updateWithTouchAt() function, the velocity is set to 150% of the average of the current and new velocities when the timeDelta < half second and curVelocityIsHigh is true.
Attachment #717804 - Attachment is obsolete: true
Attachment #727528 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 727528 [details] [diff] [review]
Second patch

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

I haven't had a chance to put this on a device and see how it feels (and probably won't be able to do that until next week) but the code looks reasonable enough. I'm not sure you need the 500ms check though. Can you explain why you put that in?
Attachment #727528 - Flags: feedback?(bugmail.mozilla) → feedback+
No rush. The class is over, so there are no more deadlines or pressure in that sense but I am determined to make this work. I will likely be less responsive for a week due to spring break, but I will be back on this for next quarter.

I wanted to ask about the timeDelta variable but I decided to keep my changes and just get a second patch in without delaying any longer. I understood the timeDelta to be the time the user's finger spends on the screen. I am not sure if that is right, so if someone could please explain to me what timeDelta is actually measuring or representing that would be most helpful.

Although, thinking about it now, it seems you are right about it being unnecessary since screening for a high velocity should already imply the same thing.

I just tested it again on my Nexus 7 to make sure the other night wasn't some desperate placebo effect. It seems like a decent change over the original, and I like it much better than our first patch's approach. It does not disturb the existing prefs and code nearly as much, and it also seems to preserve low-velocity scrolling much better.

I am not entirely sure the acceleration part is taking effect, which leads me to another thing I've been meaning to ask: is there any way to have fields like mVelocity print to the screen during testing? I know there is some fairly complex math behind all of it, so seeing some concrete values would offer a much better sense of what range of values we're dealing with. It would help to see when the threshold is broken and just how fast scrolling tends to get when the user really wants it to haul screen.

The new layer of friction allows it to flow much more freely at high velocities so that even if the velocity hike isn't occurring, I can get from the top of the page to the bottom with two quick swipes using the Mozilla IRC page as a test subject (https://wiki.mozilla.org/IRC).

P.S. If anyone knows of a longer webpage for testing, that would help too. I have looked around for one casually but they're either much too long (causing delay from loading) or are too filled with junk that confuse swipes for taps or selections.
planet.mozilla.org is pretty long
(In reply to Terry from comment #38)
> I wanted to ask about the timeDelta variable but I decided to keep my
> changes and just get a second patch in without delaying any longer. I
> understood the timeDelta to be the time the user's finger spends on the
> screen. I am not sure if that is right, so if someone could please explain
> to me what timeDelta is actually measuring or representing that would be
> most helpful.

float timeDelta = (float)(time - mLastEventTime);

It is the time between touch events. This is going to be a fairly arbitrary value, depending in part on how the user is moving their finger, and in part on the hardware being used.

> I am not entirely sure the acceleration part is taking effect, which leads
> me to another thing I've been meaning to ask: is there any way to have
> fields like mVelocity print to the screen during testing? I know there is
> some fairly complex math behind all of it, so seeing some concrete values
> would offer a much better sense of what range of values we're dealing with.
> It would help to see when the threshold is broken and just how fast
> scrolling tends to get when the user really wants it to haul screen.

For that sort of thing I usually print to logcat. You can use Log.d(LOGTAG, "blah") to print something out to logcat, and view the logcat stream by running "adb logcat". Printing to the screen is generally much harder - a hack you could try is setting the value as the title of the tab (using Tabs.getInstance().getSelectedTab().updateTitle("blah")) and then it will update in the address bar. You may also need to lock the address bar in place by setting the browser.chrome.dynamictoolbar pref to false in about:config. (Note I have never tried this myself but if you really want the data to be visible on the screen this is probably the simplest way to start)

> P.S. If anyone knows of a longer webpage for testing, that would help too. I
> have looked around for one casually but they're either much too long
> (causing delay from loading) or are too filled with junk that confuse swipes
> for taps or selections.

This bug page is fairly log. I also use timecube.com as it is pretty long and doesn't usually contain links or many images. Just don't try to read or understand the content :) And as mark said, planet.mozilla.org is also really long but tends to take a while to load.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #40)
> 
> float timeDelta = (float)(time - mLastEventTime);
> 
> It is the time between touch events. This is going to be a fairly arbitrary
> value, depending in part on how the user is moving their finger, and in part
> on the hardware being used.
> 

I've traced it back to calls to track(event) which led me to the Android MotionEvent itself. I am trying to determine when touch events actually occur, and that is where I get a bit lost. I know that when a user touches down, that registers as a touch event, but beyond that I am unsure. When is the next event triggered? And in general, what is the timeDelta measuring in terms of what the user is doing?

I will try the "adb logcat" readout next time. The log might be enough to get a good idea of the numbers and what's going on at what time. I will start there and see what happens.

Thank you for the webpage suggestions. They should definitely help me test.
(In reply to Terry from comment #41)
> I've traced it back to calls to track(event) which led me to the Android
> MotionEvent itself. I am trying to determine when touch events actually
> occur, and that is where I get a bit lost. I know that when a user touches
> down, that registers as a touch event, but beyond that I am unsure. When is
> the next event triggered? And in general, what is the timeDelta measuring in
> terms of what the user is doing?

The events are generated pretty much whenever the hardware feels like it. There are no guarantees with respect to how often we'll receive these events. timeDelta is going to be based on how frequently we receive events, and by itself is not very useful to determine what the user is doing. Only in combination with the touch points is it useful.
Sorry for the delay. Another busy quarter ahead.

Anyway, I tried the last patch on a Samsung Galaxy S3 as well and it seemed to work all right. Again, the idea was to maintain low-speed scrolling and have it jump thresholds sooner since the user is either scrolling slow enough to read or really wants to move the page.

I think I can clean it up and comment some items, but I am waiting on some more concrete feedback from anyone else who has tried the patch for its functionality.
I put your latest patch on a build and will use it a bit tonight. Hopefully I'll have some concrete feedback for you tomorrow. Sorry for the lag time!
Flags: needinfo?(bugmail.mozilla)
(In reply to Terry from comment #43)
> Again, the idea was to maintain low-speed scrolling and
> have it jump thresholds sooner since the user is either scrolling slow
> enough to read or really wants to move the page.

So the good news is that it does feel like your patch accomplishes this effect. The low-speed scrolling behaviour is mostly unchanged from before, and for high-speed scrolling the page moves a lot farther than it used to. However I still feel like the mid-range scrolling is harder to get right. For example, on a Nexus 4, if I go to CNN.com, zoom in to the max zoom, start at the top-left corner of the page, and try to fling so that I reach the "The Latest" section in the left column, it's almost impossible. My flings either hit the slow-scroll path and stop before this point, or they go into the fast-scroll path and the page just keeps moving until I'm much further down. When I try this on the current Nightly build I can easily adjust my fling power to hit that section, or any other section, pretty consistently.

Depending on your device the "mid-range fling" might correspond to a different part of CNN.com, but I suggest giving this a try and seeing if you can consistently hit different parts of the page. Maybe it's just a matter of tuning the parameters a little more, and with that things will all just work great.

That being said, I also don't fully like the low-friction behaviour. It feels too much like the page is on ice and will just keep sliding. This is entirely my own view, and others may disagree, but based on past experience I think others will feel the same way (see bug 699303 comment 0 for an example of exactly what I mean).

As much as I hate to drag this on, I think it might be better to try a completely different approach, the one you actually described in comment 17 as "accelerated scrolling". I was thinking a bit about how this could be implemented today, and I think a simple approach to this wouldn't require any changes to the core physics at all. It would simply store the start time of the last fling that was made. If another fling (in the same direction, and with some minimum velocity) is started within some short period of time (probably around ~250ms) of the last fling, then automatically apply some "acceleration" to that second fling by increasing the fling velocity. I think this would be pretty simple to implement and test, and also would not affect existing scrolling behaviour as much except in the cases where the user doing multiple flings, which is exactly the case we want to be affecting.
Flags: needinfo?(bugmail.mozilla)
I think what kats describes in comment #45 is indeed the right way to go. It'll be easier to expose checkerboard though, which will be something to consider - but we can tackle that when we get there.

It also might affect robocop/eideticker tests, so worth considering that too (though if it makes them harder, I'd be all for that tbh).
No need to apologize, Kats. I'm just trying to keep the discussion alive and keep myself involved :)

Thank you both for the feedback, and I agree. It was a bit of a rush during the quarter, so we may have strayed from that solution due to that. However, I can also say that when I tried something similar before, I think the percentage-based friction still slows it down rather fast.

To elaborate, I can visualize the percentage-based friction stopping each scroll in a similar and consistent amount of time regardless of scroll velocity. This is due to the fact that it cuts away the same relative portion of the velocity each time, so no matter what it is, it will be proportionally reduced to 0 in similar amounts of time based on when it falls back below the low velocity threshold. Does that make sense?

(In reply to Chris Lord [:cwiiis] from comment #46)
> I think what kats describes in comment #45 is indeed the right way to go.
> It'll be easier to expose checkerboard though, which will be something to
> consider - but we can tackle that when we get there.
> 

By checkerboard, do you mean the page does not ender and just shows the background? Please explain.

> It also might affect robocop/eideticker tests, so worth considering that too
> (though if it makes them harder, I'd be all for that tbh).

I am also unfamiliar with robocop/eideticker tests, so some ellaboration on that would be helpful to clue me in.

Anyway, I will try the true acceleration solution and tweak around to see if it can work well since the only other good option so far seems to be to make a new case of middle scrolling speed and prefs to catch those better, which is not as clean just by the sound of it.

However, I feel that with a less complex solution (not using more than two cases or ranges of speeds/etc.), we will be limited to picking our poison. On the one hand, it can flow freely without much effort, but will may not stop as soon as you want it to. On the other hand, if we ramp velocity using acceleration with the current friction model, it will still stop fairly shortly before the user will have to scroll once again (many scrolls needed).

Hopefully you guys can visualize what I mean. I am usually working on this during my more boring classes, so I may be slightly distracted in my train of thought ;)

More feedback always welcome.

P.S. I am moving my build and development process to another computer since I no longer need it to be on a portable system. Having some issues at the moment and busy otherwise, so I may be slow in response as well.
(In reply to Terry from comment #47)
> Thank you both for the feedback, and I agree. It was a bit of a rush during
> the quarter, so we may have strayed from that solution due to that. However,
> I can also say that when I tried something similar before, I think the
> percentage-based friction still slows it down rather fast.
> 
> To elaborate, I can visualize the percentage-based friction stopping each
> scroll in a similar and consistent amount of time regardless of scroll
> velocity. This is due to the fact that it cuts away the same relative
> portion of the velocity each time, so no matter what it is, it will be
> proportionally reduced to 0 in similar amounts of time based on when it
> falls back below the low velocity threshold. Does that make sense?

Sort of, yes. As an example, let's say I start with a velocity of 10, and use the FRICTION_SLOW of 0.85, and continue iterating until it drops below the threshold of 0.1. Here it will take 29 frames. However if I bump the velocity to 20, using the same FRICTION_SLOW and threshold values, it takes 33 frames. So we see that a 100% increase in the velocity results in a 13.8% increase in the time of the fling animation, which I believe is what you're getting at.

However, if look at the total distance traveled during this time, you'll see that 10+8.5+...+0.105616049755 = 66.0681757181, and 20+17+...+0.110264476145 = 132.708501302. So even though the *time* taken isn't much longer, the *distance traveled* also increases by ~100% too, which is basically what we're after. In fact I think it's rather nice that we can increase the distance travelled without having the fling animation drag on forever because then the user gets to where they want to go faster. Does that make sense?

> (In reply to Chris Lord [:cwiiis] from comment #46)
> By checkerboard, do you mean the page does not ender and just shows the
> background? Please explain.

Yup, that's what he's referring to. It's referred to as "checkerboard" because originally browsers would show a checkerboard pattern in places where they couldn't draw content fast enough.

> I am also unfamiliar with robocop/eideticker tests, so some ellaboration on
> that would be helpful to clue me in.

These are just benchmarks we have to measure the perceived smoothness of scrolling and checkerboarding. I'm not really concerned about how this impacts those tests; we can adjust them to deal with this as needed.

Thanks for continuing to work on this!
OK, this all sounds good. I have the build environment setup on my desktop now, and I was following up on another bug that had some issues resurface after being approved.

Anyway, I do not have enough time to review the code thoroughly yet, and I'm not sure when I will. I do remember trying to implement something along the lines of your idea earlier, but I was using timeDelta which now seems wrong given your explanation of what timeDelta is.

I believe the velocity change would still occur at updateWithTouchAt() function within Axis.java, but I am not sure where I would go to record the fling times for comparison. Does anyone know off the top of their head where I can find this so that I have somewhere exact to look next time I work on this?
If I understand your question correctly, the piece of code you're looking for is the fling() function in JavaPanZoomController.java. If stopped is false, you should record the current time and the calculated fling velocity, and then if fling is later called very shortly after the first fling you can apply an acceleration factor.
Thank you, kats. I am very sorry this has dragged on. If this feature is something that needs to be implemented with urgency, please feel free to reassign.

However, please don't misunderstand that as a request to be reassigned. I would like to complete this, but it will have to wait until mid-June for me to really dive into this again. I know the changes might not even be that complicated in the end, but I don't want to rush this since I've already made some poor decisions earlier as a result of rushing.

I am about to graduate and finish up an internship, so once mid-June rolls around I will have free reign to work on whatever I wish and finishing this will be at the top of my list.

Thanks for understanding and bearing with me.
No problem, it was a pretty open-ended bug to begin with. I'll leave it with you for now but might steal it away if we need it done with more urgency.
All righty. Not exactly mid-June anymore, but things have finally settled down a bit.

I'm traveling, but I will try my best to come up with a patch soon. I'm on a laptop again now, so building and testing will be a bit more of a pain but I have everything I really need.
(In reply to Terry from comment #53)
> All righty. Not exactly mid-June anymore, but things have finally settled
> down a bit.
> 
> I'm traveling, but I will try my best to come up with a patch soon. I'm on a
> laptop again now, so building and testing will be a bit more of a pain but I
> have everything I really need.

Hi Terry, I am also interested in this feature.

Is there a simple description available about what functionality you indent to implement?

I am coming in from an android ListView FastScroll angle (with thumb) which I try to implement as an offline-capable web app.

See also
http://stackoverflow.com/questions/18317206/why-do-mobile-browsers-not-support-scrollbar-thumb
for my problem statement/question.

It basically works fine on the desktop with Firefox and Chrome, where scrollbar thumb is available and allows fast scrolling to any position in the complete scrollHeight.

With CTRL+SHIFT+MOUSE1 on the scrollbar track piece it even allows direct absolute positioning. This is great!

My hope is that the mobile implementation UX will come close to that (dragging the scroll thumb for quick positioning across all of scrollHeight).

Only the direct absolute positioning will be a challenge on a touchscreen for lack of CTRL+SHIFT+MOUSE1 on a mobile device with only soft input methods, but I could live without that.
(In reply to adrian from comment #54)
> (In reply to Terry from comment #53)
> With CTRL+SHIFT+MOUSE1 on the scrollbar track piece it even allows direct
> absolute positioning. This is great!

> Only the direct absolute positioning will be a challenge on a touchscreen
> for lack of CTRL+SHIFT+MOUSE1 on a mobile device with only soft input
> methods, but I could live without that.

Make that just SHIFT+MOUSE1, CTRL is not needed.

I am actually warming up to the two-finger scroll suggestion.

Fiddling with a tiny scroll thumb, or making it big and losing precious screen space, does not seem attractive.
(In reply to adrian from comment #55)
> (In reply to adrian from comment #54)
> > (In reply to Terry from comment #53)
> > With CTRL+SHIFT+MOUSE1 on the scrollbar track piece it even allows direct
> > absolute positioning. This is great!
> 
> > Only the direct absolute positioning will be a challenge on a touchscreen
> > for lack of CTRL+SHIFT+MOUSE1 on a mobile device with only soft input
> > methods, but I could live without that.
> 
> Make that just SHIFT+MOUSE1, CTRL is not needed.
> 
> I am actually warming up to the two-finger scroll suggestion.
> 
> Fiddling with a tiny scroll thumb, or making it big and losing precious
> screen space, does not seem attractive.

Yes, the arguments against the thumb-scroll were primarily that it clashed with touch interfaces.

The arguments against two-finger scroll included:
1_ Not intuitive, ergo not easily discovered by the user.
2_ Conflict with zoom-in/zoom-out, rotate, [insert other two-finger gestures]
3_ Requires no less than two fingers to accomplish

I would love to continue working on this, but I cannot say when I can delve back into the code. It will have to be on weekends for sure.

Any more ideas on how to implement besides accelerating the scroll velocity upon multiple swipes in quick succession?
See Also: 969799 → 
2-finger-scrolling is very intuitive, because it's very easy to understand that scrolling with 2 fingers is faster than scrolling with 1 finger.

But is has a big downside: you can't use it with one-hand-use (holding the phone in one hand and using the thumb to operate it).

See the suggested one-finger-gesture in bug 969799 (https://bugzilla.mozilla.org/show_bug.cgi?id=969799). 

I think we should use any gesture that supports fast scrolling on lenghty webpages with one finger so that you can use it on phones in one-hand-use. My idea 969799 is free to use, but maybe somebody find an even better one-hand-gesture.
(In reply to Carlos F from comment #58)
> 2-finger-scrolling is very intuitive, because it's very easy to understand
> that scrolling with 2 fingers is faster than scrolling with 1 finger.
> 
> But is has a big downside: you can't use it with one-hand-use (holding the
> phone in one hand and using the thumb to operate it).
> 
> See the suggested one-finger-gesture in bug 969799
> (https://bugzilla.mozilla.org/show_bug.cgi?id=969799). 
> 
> I think we should use any gesture that supports fast scrolling on lenghty
> webpages with one finger so that you can use it on phones in one-hand-use.
> My idea 969799 is free to use, but maybe somebody find an even better
> one-hand-gesture.

How does someone use a finger on phones in one-hand use? I assume you need to use your thumb, since the phone is being cradled in your hand. If using your thumb, you don't have a wide range of motion, especially motion that keeps the thumb on the right (or left) edge.

It's tricky. It's also hard to discover.
So although this bug started out as a two-finger scroll idea, somewhere around comment 14 it morphed into having a scroll dragger, and then later around comment 17 it became accelerated scrolling with multiple flings. So the bug title is actually incorrect, and this bug is basically a duplicate of bug 721421 although some work has been done here.

Since it seems from the comments that most people would be happier with doing accelerated one-finger scrolling rather than a two-finger scroll, and this bug is already getting too long to be manageable, I'm going to post a quick summary over in bug 721421 and dupe this bug to that one.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: uiwanted
Resolution: --- → DUPLICATE
Whiteboard: [lang=java][mentor=kats][mentor=wesj]
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: