817074 - Implement a "swiping" animation when the user presses a back/forward button.

Status: RESOLVED WONTFIX

(Firefox :: General, enhancement)

Description

[Josiah Bruner [:jsbruner]]

This was part of this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=678392

You will need that to implement the patch located there. That will enabled the swiping gesture on OS X. Now, in addition to using the swipe gesture on trackpads/magic mouse, I think it would be nice if this was implemented on all platforms, when the user presses the back/forward buttons. 

The key here is that the animation should be visible, but brief. The timing needs to be right on this. If it takes too long, the user will get annoyed with it, if it is too short, it will look annoying, which defeats the purpose.

If you would like to see an idea, and you are on OS X, go to Game Center. Click on something, and then press the back button. You will see an animation take you back. This is visually appealing, and would add something that others browsers don't have.

Comment 1

[Josiah Bruner [:jsbruner]]

I will start working on this in a little bit, most likely after I can confirm that bug 678392 is working pretty well. Also, if Stephen could explain the new API I can call to get this to work, that would be great.

Comment 2

[Josiah Bruner [:jsbruner]]

Okay. This is what happens when an Objective-C developers jumps right in to C++. I really need Stephen to supply the lines of code that are necessary to do this:

Just a few frame animation. 
After the animation goes back a page.

That is all. Also, I will need to automate a delta change, does the delta range from 0-10 or something?

Comment 3

[Stephen A Pohl [:spohl]]

(In reply to Josiah from comment #2)

I'll get to this once the patch for bug 678392 is finalized.

Comment 4

[Josiah Bruner [:jsbruner]]

(In reply to Stephen Pohl [:spohl] from comment #3)
> (In reply to Josiah from comment #2)
> 
> I'll get to this once the patch for bug 678392 is finalized.

 Thanks. That is what I assumed.

Comment 5

[Stephen A Pohl [:spohl]]

Attachment: Wip

This demonstrates swipe animations when clicking on the back/forward buttons.

Comment 6

[Stephen A Pohl [:spohl]]

Note: This patch requires at least the current patch for bug 678392 to work. For better performance, it is recommended to apply the following patches:

1. Patch for bug 817700.
2. Patch for bug 678392.
3. "Patch for availability check" for bug 678392.

Comment 7

[Josiah Bruner [:jsbruner]]

(In reply to Stephen Pohl [:spohl] from comment #5)
> Created attachment 697263 [details] [diff] [review]
> Wip
> 
> This demonstrates swipe animations when clicking on the back/forward buttons.

Thanks Stephen! I will take a look at that either sometime tonight or possibly tomorrow morning. Either way, here is what needs to be considered, although this will be MUCH easier to implement then an entire swiping animation.

1. This should work on ALL platforms. I have a Windows, Mac, and Ubuntu machine, but any other testers would be appreciated.

2. The timing needs to be perfected. If the animation is slow, users will get annoyed that it is there. If it is too fast, it will look odd. We need to find the right balance.

3. It needs to appear natural. This might already be the case, I need to look at the patch.

4. Since this will be run on machines other than OS X, we need to consider that graphics and memory can and very well may be lower. This animation needs to be smooth on low-end hardware. If this is not possible, we will need to disable it.

5. That brings me to my last point (For now). The user should be able to disable it. Since it is a button animation, and is not used throughout other apps, the user may very well hate it. It needs to be easily removable. So hiding it in about:config is NOT an option.

I will consider more later.

Comment 8

[Josiah Bruner [:jsbruner]]

Attachment: Combined patch of the other neccessary bugs.

This is a patch combining the other three necessary patches. 

Swiping Implementation

Swiping Check

Blob fix.

So, you need only use the two patches in this bug for this to work.

Comment 9

[Stephen A Pohl [:spohl]]

Fyi, the patch here only enables animations on the back/forward buttons for platforms that already support swipe gestures. You will want to have a look at gHistorySwipeAnimation._isSupported() and the nsISwipeService component interface to see how you can add other platforms. You'll find it all in the patch for bug678392.

Comment 10

[Josiah Bruner [:jsbruner]]

(In reply to Stephen Pohl [:spohl] from comment #9)
> Fyi, the patch here only enables animations on the back/forward buttons for
> platforms that already support swipe gestures. You will want to have a look
> at gHistorySwipeAnimation._isSupported() and the nsISwipeService component
> interface to see how you can add other platforms. You'll find it all in the
> patch for bug678392.

So, you are saying that the animation will not work right now on Windows/Linux/Other platforms. Only OS X. 

I can not seem to test on my other platforms, as I have no hg account, so I can't use a tryserver. And I don't have cross-compiling set up. I will try setting up a new private repo on my Linux system shortly.

Comment 11

[Josiah Bruner [:jsbruner]]

Also, how are you updating the animation at a certain speed. I am assuming you are using this for that:

let id = setInterval(function() {
        gHistorySwipeAnimation.updateAnimation(counter++ / 10);
        if (counter > 10) {
          if (id)
            clearInterval(id);
          gHistorySwipeAnimation._navigateToHistoryIndex();
        }
      }, 10);

But how can you modify the time with that? Or is it not suppose to?

Comment 12

[Josiah Bruner [:jsbruner]]

(In reply to Josiah from comment #11)
> Also, how are you updating the animation at a certain speed. I am assuming
> you are using this for that:
> 
> let id = setInterval(function() {
>         gHistorySwipeAnimation.updateAnimation(counter++ / 10);
>         if (counter > 10) {
>           if (id)
>             clearInterval(id);
>           gHistorySwipeAnimation._navigateToHistoryIndex();
>         }
>       }, 10);
> 
> But how can you modify the time with that? Or is it not suppose to?

Nevermind, I got it. :)

Comment 13

[Josiah Bruner [:jsbruner]]

Attachment: Patch for Bug. Improved speed.

This patch is a slight modification of the first patch. It increases the smoothness of the animation and slightly slows down the speed, which in my opinion, looks better.

I have yet to start working on the ability to use this on other platforms, but I have successfully set up a linux working directory. Tomorrow I should be able to start tackling this.

Comment 14

[Josiah Bruner [:jsbruner]]

The animation works if I force return true for isSupported. Is there a reason we should not return true? If the swiping isn't supported, then it will just never get called. This makes my end a lot easier. 

@Stephen, If it is because you don't want it to work on OS X when swiping is turned off (Which is how it should be). In this case, it should be changed to always return true UNLESS swiping is disabled and they are on OS X.

For testing, I will upload another patch with this change shortly.

Comment 15

[Stephen A Pohl [:spohl]]

isSupported checks whether swipe gestures are available on the platform, so the current implementation seems correct. Since we're not dealing with gestures here, you will probably want to add a second method to check whether animations on back/forward buttons are enabled. That method will then have to check whether animations are turned on by default on a platform and/or if the user changed the default setting.

Comment 16

[Josiah Bruner [:jsbruner]]

Attachment: Main Patch

This patch brings swiping to all platforms (At least it should. I haven't tested Linux yet). I did not use the isSupported  method, instead, I created a new method as suggested, called hasButtonSwipeSupport(). 

This works in it's current state, however there are some problems. The biggest one is this:

Sometimes while going back (And only going back, forward does not have this problem) the screenshot appears chopped, as if the refresh rate is very low. 

I do not believe it is related to this bug's code. It most likely is a problem with the swiping code from bug 817074. However, it is not noticeable on that bug as the animation is controlled directly.

Comment 17

[Josiah Bruner [:jsbruner]]

Sorry. I meant I haven't tested Windows yet. Linux works fine. Kind of.

Comment 18

[Josiah Bruner [:jsbruner]]

... Okay, and reverse what I said about the bug. Only if you go forward does the problem occur. Going back is fine. 

Stephen, if you could set up a try server for these two patches, I would appreciate it. I don't have access to try servers at the moment.

Comment 19

[Josiah Bruner [:jsbruner]]

A very basic build for OS X is available here: https://dl.dropbox.com/shz/alfg4yh6yvwsvdr/VfFv6k8D2T/Nightly.app?top_level_offset=8&dl=1

That is a build straight from my own computer, so it may or may not work on your version of OS X.

Comment 20

[Josiah Bruner [:jsbruner]]

Oops. Wrong link. Here: https://dl.dropbox.com/s/ez6ewzqb6twsfb0/Nightly.app.zip?dl=1

Comment 21

[Stephen A Pohl [:spohl]]

Try builds:
http://ftp-scl3.mozilla.com/pub/mozilla.org/firefox/try-builds/[e-mail]-1dd1106156f2/

Comment 22

[Mozilla RelEng Bot]

Try run for 1dd1106156f2 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=1dd1106156f2
Results (out of 9 total builds):
    success: 8
    warnings: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-1dd1106156f2

Comment 23

[Josiah Bruner [:jsbruner]]

(In reply to Stephen Pohl [:spohl] from comment #21)
> Try builds:
> http://ftp-scl3.mozilla.com/pub/mozilla.org/firefox/try-builds/spohl@mozilla.
> com-1dd1106156f2/

Thanks. So, I can confirm that the animation does work on all platforms. Excellent. Now, one of the biggest problems is actually not the messed up drawing (Which is a problem), but instead there is an issue that will not allow you to navigate back and forward, when pages are reloading lots of data. Exactly why this happens I am not sure of yet. 

What I do know is that when trying to go back or forward when loading lots of data can cause the animation to essentially reset itself, or drastically slow the animation down. For reference. Some pages that do this are Cnet.com and Amazon.com

I will look at this some more tomorrow.

Comment 24

[Josiah Bruner [:jsbruner]]

Marking as assigned. Should have done this earlier.

Comment 25

[Josiah Bruner [:jsbruner]]

Attachment: Main Patch (WIP)

Updated patch. Properly sets up the ability to enable and disable the animation on buttons only. This DOES NOT add the ability to do this yet. Starting that next, this simply adds the proper code, as my previous patch had problems here.

Now, I am not sure how to add anything to the settings screen. If someone could explain what class I am suppose to use, that would be great.

Comment 26

[José Jeria]

I guess an UI review would also be in order for this. FWIW, I personally think that this could end up being annoying. I only seen this behavior in Game Center, and I guess it makes sense there, since it is a very graphical application. Other applications that support the 2 finger swiping, such as Safari or Xcode, don't support this for their back and forward buttons.

Comment 27

[Josh Matthews [:jdm]]

The preferences screen code is kept in http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/. You'll find that each panel is a separate pair of xul/js files.

Comment 28

[Josiah Bruner [:jsbruner]]

(In reply to José Jeria from comment #26)
> I guess an UI review would also be in order for this. FWIW, I personally
> think that this could end up being annoying. I only seen this behavior in
> Game Center, and I guess it makes sense there, since it is a very graphical
> application. Other applications that support the 2 finger swiping, such as
> Safari or Xcode, don't support this for their back and forward buttons.

A UI review for sure. I can not be positive that this is the most appealing way to do this, although I personally think it looks great. Others may disagree. However, it does bring something to Firefox the other browsers don't have. Something I think Firefox is missing. We tend to add things based on "other applications", when we could be trying to innovate. That is the intent here. It may or may not work.

I hope it does get added to Firefox, as it will be easily disabled, and I think today's world needs more realistic animations. Since going back and forward is part of history, it seems logical to show the user that they are going back or forward.

 (In reply to Josh Matthews [:jdm] from comment #27)
> The preferences screen code is kept in
> http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/
> . You'll find that each panel is a separate pair of xul/js files.

Thanks. :-)

Comment 29

[Josiah Bruner [:jsbruner]]

Do any of you have a Windows machine you can test on? I tried it on Linux, OS X, and Windows. Although they work on all platforms, I found the animation to be significantly smoother on OS X and then Linux. Windows seems to be struggling, and it was not a bad machine. <4 GB of RAM, Windows 7, 2.3 GHZ processor. 

I would like to know if it is just me who has this problem, or if this is a common issue. My machine does not have a very good graphic's card, but I am surprised a simple animation would be that intense.

Comment 30

[Josiah Bruner [:jsbruner]]

At least the combined patch is broken. I can not update until the Swipe Availability patch is updated. Until then this bug is on hold. I will update as soon as possible.

However, I did add the UI checkbox in settings. I still have to figure out how to actually store this setting. If anyone knows how settings are stored in Firefox, I would appreciate learning about it.

Comment 31

[Josh Matthews [:jdm]]

Global settings are usually preferences. The controls in the settings panels are usually linked to preferences that are defined in the xul file, and these preferences are read at other parts of the code that make use of them.

Comment 32

[Josiah Bruner [:jsbruner]]

(In reply to Josh Matthews [:jdm] from comment #31)
> Global settings are usually preferences. The controls in the settings panels
> are usually linked to preferences that are defined in the xul file, and
> these preferences are read at other parts of the code that make use of them.

I see. So I don't have to manually store them, meaning I don't have to code the part about saving data? The xul files will store changes when the toggle is pressed, or are used to store the data somewhere? That is what I am understanding, is that correct?

Comment 33

[Josiah Bruner [:jsbruner]]

Attachment: Combined patch of the other neccessary bugs. (Updated)

Updating patch combining necessities.

Comment 34

[Josh Matthews [:jdm]]

You may need to add your pref to http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/init/all.js as well.

Comment 35

[Josiah Bruner [:jsbruner]]

Hmm. Alright, perhaps I need more explanation. 

In advanced.xul I added this line:

<preference id="general.buttonSwipe"	       name="general.buttonSwipe"       type="bool"/>

In the same file, I added this to the group:

<checkbox id = "useSwiping"   label="Use swiping when clicking buttons"    preference="general.buttonSwipe" />

That shows up.

What do I do to read this? I tried in browser.js:

if (general.buttonSwipe = true)

That didn't work. And I am not surprised.

Comment 36

[Josiah Bruner [:jsbruner]]

Oh, also in the libpref/src/init/all.js I added this:

// Swiping Pref
pref("general.buttonSwipe", true);

Comment 37

[Josiah Bruner [:jsbruner]]

Wait. I got it to work. Kind of. The UI does work, and you can toggle the setting. However changes only apply after restart. How do I activate immediate changes.

Sorry for all the questions, but I'm new. :)

Comment 38

[Josiah Bruner [:jsbruner]]

Attachment: Main Patch (WIP) (Update)

This patch should add the ability to enable and disable the swiping animation. My previous comment still applies though.

Comment 39

[Josh Matthews [:jdm]]

To check the value of the preference, you need to use the preferences service and the getBoolPref method: Services.prefs.getBoolPref("general.buttonSwipe"), as long as Services.jsm is imported in the same file.

Comment 40

[Josiah Bruner [:jsbruner]]

Attachment: Main Patch (WIP) (Update 2)

Updated patch. This correctly implements the ability to enable and disable the swiping animation. However, it has some flaws. Here are a few things I noticed:

1. On OS X only, the addition of another checkbox slightly breaks the preferences box UI.

2. If when Firefox launches, the checkbox for animation is off AND the system is not compatible with swiping gestures, the history never get initalized. This means that when I go and check the box for animations, I must restart Firefox first. Best seen in Linux, does not happen on my version of OS X.

Comment 41

[Justin Dolske [:Dolske]]

Have you done any performance measurements on this?

One vague concern I'd have, from a quick skim of this bug, is ensuring that the animation doesn't add too much overhead. (EG, it would clearly be a bad thing to make back/forward navigation go from 10ms to 1000ms) This is, of course, a tricky thing to measure because the swipe animation should quite intentionally take a small amount of time.

Comment 42

[Josiah Bruner [:jsbruner]]

(In reply to Justin Dolske [:Dolske] from comment #41)
> Have you done any performance measurements on this?
Not really. Although it is a concern I have. However, from what I have seen, performance is actually fine, but there are a few problems:

1. The smoothness of the animation seems to SLIGHTLY vary with OS X as the best, then Ubuntu, and Windows last. I am not sure why this is the case, it might have to do with bug 678392 and not this one.

2. The biggest performance problem is that when pages are still loading quite a bit of data, trying to go back with the buttons will either cause the animation to start and abruptly stop, leaving the user on the same page. Or, it causes a VERY slow animation. One way to stop this is to cancel loading before the animation takes place, but I am not sure how that would effect other things.


> One vague concern I'd have, from a quick skim of this bug, is ensuring that
> the animation doesn't add too much overhead. (EG, it would clearly be a bad
> thing to make back/forward navigation go from 10ms to 1000ms) This is, of
> course, a tricky thing to measure because the swipe animation should quite
> intentionally take a small amount of time.

Indeed, this is something to be worried about. However, that animation speed is a constant, and should in theory, not change. But like I said, I have found certain environments to alter this speed, which I must address. As for the browsing experience, this is kind of a problem.

Like you said, for most users, the current speed works fine and doesn't really matter. However, for more intense web browsers, this animation might not be fast enough. It can be easily disabled, but settings are something that should be used sparingly.

Once I am closer to finishing, I'm sure Stephen or somebody could add another try server. This will need to go through UI review and must be tested by many people.

Comment 43

[Stephen A Pohl [:spohl]]

A few more things to consider:
1. The current patch makes use of setInterval for the animation. I used setInterval to demonstrate how the animation may work on the back/forward buttons in the first patch, but I don't make any claims that this is the right approach. We should confirm. Even if it is the right approach we need to figure out the correct argument for the interval as well as the counter used to move the animation along the x axis.

2. Clicking on the back/forward buttons repeatedly and quickly has to work as expected, i.e. animations have to be interrupted and the next animation should start.

3. Slower animations may look nicer but may slow down a user. Personally, I like animations to be fast to not slow me down, or I will turn them off. We'll have to strike the right balance. I'm guessing UX may be able to help here?

4. We're currently calling 'private' methods and properties of gHistorySwipeAnimation from within BrowserBack and BrowserForward. For example, we shouldn't directly access _historyIndex and/or _navigateToHistoryIndex() (note the underscore). Again, this was just to demonstrate how animations could work on the back/forward buttons, but this will need to be refactored for the final patch.

Comment 44

[Josiah Bruner [:jsbruner]]

Attachment: Main Patch (WIP) (Update 3)

Another patch update. This increases the smoothness of the animation, and cancels loading before navigating away, which is beneficial.(Not necessarily the right way to do this) The animation is also slightly faster.

Comment 45

[Josiah Bruner [:jsbruner]]

Attachment: Combined patch of the other neccessary bugs. (Updated 2)

Update to the combined patch. Simply adds the fix for the availability patch on the latest trunk.

Comment 46

[Josiah Bruner [:jsbruner]]

Attachment: Combined patch of the other neccessary bugs. (Update 3)

Patch updates Combined Patch with latest changes to the blob issue. The button swipe patch is broken at the moment, I will fix it soon.

Comment 47

[Josiah Bruner [:jsbruner]]

Attachment: Main Patch (WIP) (Update 4)

Fixed patch. Now works on latest trunk. Performance seems to be better for some reason, but I did not make any significant changes.

Stephen: If you could open up another try server, I would be oh so happy.

Comment 48

[Mozilla RelEng Bot]

Try run for ff20a0393631 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ff20a0393631
Results (out of 1 total builds):
    success: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ff20a0393631

Comment 49

[Josiah Bruner [:jsbruner]]

Attachment: Main Patch (WIP) (Update 5)

Updated patch. Now settings toggle works perfectly. Also, performance is dramatically better.

Comment 50

[Mozilla RelEng Bot]

Try run for 94543479123e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=94543479123e
Results (out of 4 total builds):
    success: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-94543479123e

Comment 51

[Josiah Bruner [:jsbruner]]

Attachment: Main Patch (V 0.1)

Major update to patch. This is getting close to being done now. I have now switched to requestAnimationFrame which should improve performance on later platforms. This replacing the setInterval, and should be quite beneficial. 

Stephen, if you could set up another tryserver that would be great. I would like to have people start testing this, and decide if the animation should be slower/faster, or discover if there are other issues.

Comment 52

[Josiah Bruner [:jsbruner]]

FYI, Stephen, you don't have to run the try server. Ms2ger is doing it for me. Just a heads up.

Comment 53

[Josiah Bruner [:jsbruner]]

Try Builds: 

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-779069a6209e/

Comment 54

[Josiah Bruner [:jsbruner]]

Attachment: Main Patch (V 0.2)

Updated patch. Little changes. Should improve speed on slower platforms, but I have yet to verify.

Comment 55

[Josiah Bruner [:jsbruner]]

Latest builds: 

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-7b2ddeccd9e3/

Comment 56

[Josiah Bruner [:jsbruner]]

Attachment: Combined patch of the other neccessary bugs. (Update 4)

Updated combined patch. Adds updates Stephen recently made. Also includes the "Stuck Page" patch.

Comment 57

[Josiah Bruner [:jsbruner]]

Here's the deal. As of now, this works quite well. (As far as I know), however, there is still an issue on low-powered devices. I can not switch to CSS animations/transitions for this, at least not easily, which means this needs to somehow work. Options:

Start the animation, and after several frames, determine the proper speed. This could work, but it would require a lot of code, something I would prefer not to have stuffed into Browser.js

Second, just determine if the FPS is below a certain point (We would need to figure that out), and if so, just increase the speed quite a bit. Of course, the animation time will vary, but hopefully not by much.

Third, just slightly increase the speed altogether, then hopefully on faster platforms, it won't really be noticed, but should be a little faster on slower ones. Obviously, the negative points are obvious. The faster computers animation would be quite faster, and slower ones would have more of a normal animation. 

Thoughts?

Comment 58

[Stephen A Pohl [:spohl]]

I've thought about this and by now I believe that CSS transitions/animations are the way to go here. I don't think that we're in a big hurry, so I would favor a proper solution over something that feels rushed. The animation should occur on the box elements that are part of the animation overlay.

Comment 59

[Josiah Bruner [:jsbruner]]

(In reply to Stephen Pohl [:spohl] from comment #58)
> I've thought about this and by now I believe that CSS transitions/animations
> are the way to go here. I don't think that we're in a big hurry, so I would
> favor a proper solution over something that feels rushed. The animation
> should occur on the box elements that are part of the animation overlay.

If you would like to provide an example on how to do this with CSS by altering the current code, that would be appreciated. I looked at CSS transitions, and I don't think that is what we want. I also examined animations, which looks a little better, but I am a little lost on how to get that to work with this situation. Perhaps an example would benefit me.

http://www.pastebin.mozilla.org/2078822

Comment 60

[Josiah Bruner [:jsbruner]]

At the moment, I disagree that we should use CSS animations here. It is quite difficult to implement in Browser.js, and even if that works, I'm not sure it is the best idea. 

My reason for this is by looking at other animations done in javascript files. Now, although there are few animation in Firefox, one I know of is the tab groups one. That is a slightly more complex animation (But not really much harder than ours), that is controlled without css. It is done in javascript.

Second, I would prefer to not be dependent on different code for the animation for using the buttons, and the swiping. Using css causes us to re-write the animation code, which would be a lot harder to maintain.

Third, what is really going to be the benefit here. requestAnimationFrame already provides performance enhancers and saves battery, very similar to css animations. Except, that requestAnimationFrame allows us to keep using the same code. 


Now, by no means should we rush things, but I think it would be better to spend time enhancing performance on requestAnimationFrame, since it already works pretty well, and not switch to css, which may or may not improve speed.

Therefore, for now, I will keep using requestAnimationFrame, however if anyone thinks css is better, I would appreciate it if you wrote a separate patch to test with. If there is a significant difference, then fine, we will switch, but otherwise it just makes it harder to maintain order.

Comment 61

[Josiah Bruner [:jsbruner]]

Attachment: Main Patch (V 0.3)

Updated patch. First of all, I am very pleased with this. I still am using requestAnimationFrame, however the animation is based on time-elapsed, instead of counting on the correct FPS callback. Therefore, the animation will cover the same amount of time no matter what the speed is. (Downside being of course, animation is not quite as smooth as these platforms, but CSS wasn't going to help much there either.


Note: At the moment, this patch is based on a 1 second time scale. Meaning, the animation will take 1 whole second. This allows us to easily test it, and as an example. In the next patch, speed will decrease to a "normal" amount.

Comment 62

[Josiah Bruner [:jsbruner]]

Attachment: Combined patch of the other neccessary bugs. (Update 5)

Patch updating patch dependencies.

Comment 63

[Josiah Bruner [:jsbruner]]

I just realized that my main patch does not work with the latest dependencies. I will try to fix that up over the weekend.

Comment 64

[Josiah Bruner [:jsbruner]]

Attachment: Main Patch (V 0.4)

Patch which allows compatibility with the combined dependencies. Also speed is more normal, though I think it should be slighter faster.

Patch cleanup.

Comment 65

[Josiah Bruner [:jsbruner]]

Try builds coming shortly. Appear to be successful on all platforms except for OS X??? For some reason the build has failed on OS X, but not the debug version of it. If you want to test on OS X, you must use the Debug version.

All other platforms have built fine. Only the Windows Debug has to finish.

Comment 66

[Josiah Bruner [:jsbruner]]

Try builds available at:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5b2da1e5e340/

Remember OS X users must use debug. I will try to diagnose the error on the normal OS X build later.

Comment 67

[Josiah Bruner [:jsbruner]]

Updated try builds available here: 

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-77918f556541/

Note: Fixes OS X error.

Comment 68

[Josiah Bruner [:jsbruner]]

Attachment: Combined patch of the other neccessary bugs. (Update 6)

Update of combined patch. Should work properly now.

Comment 69

[Josiah Bruner [:jsbruner]]

Attachment: Combined patch of the other neccessary bugs. (Update 7)

Updates for the latest trunk.

Comment 70

[Josiah Bruner [:jsbruner]]

Attachment: Main Patch (V 0.4.1)

Updated patch. Soft codes the text in the settings menu. Other than that no major changes. 

Also flagging for feedback by Stephen Pohl, who is looking at CSS animations.

Comment 71

[Stephen A Pohl [:spohl]]

Comment on attachment 709319 [details] [diff] [review]
Main Patch (V 0.4.1)

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

Please follow the Mozilla coding style:
https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style
For example, respect the 80 character-per-line limit and the policies around braces.

Variable and function names similar to "buttonSwipe" should have better names. "buttonSwipe" does not convey the idea that a swipe animation occurs when the back/forward buttons are pressed.

The feedback provided here isn't complete, but hopefully it'll give you an idea of what needs to be addressed.

Before we go the route of requestAnimationFrame, I'd like to see a patch that uses CSS animations/transitions. This will allow us to compare the two, identify the advantages/disadvantages of both and make an educated decision as to which one to pick.

::: browser/base/content/browser.js
@@ +6,5 @@
>  let Ci = Components.interfaces;
>  let Cu = Components.utils;
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");

Make sure that this is removed in your final patch.

@@ +1116,5 @@
>     * by the platform/configuration.
>     */
>    init: function HSA_init() {
> +      if (!this._isSupported()) {
> +          if (!this.hasButtonSwipeSupport()) {

Combine if statements with &&

@@ +1377,5 @@
> +    
> +checkButtonSwipeSupport: function HSA_checkButtonSwipeSupport() {
> +    if (gHistorySwipeAnimation.active)
> +    {
> +        //Is active, check for a change in preferences

Avoid duplicating code from hasButtonSwipeSupport

Comment 72

[Josiah Bruner [:jsbruner]]

Attachment: Main Patch (V 0.4.2)

Fixes for current trunk and dependencies.

Comment 73

[Josiah Bruner [:jsbruner]]

Oops. Didn't see that you already gave feedback. Ignore that.

Comment 74

[Josiah Bruner [:jsbruner]]

Attachment: Main Patch (V 0.5)

Updated Patch. Improved formatting that should be more in line with the Mozilla Coding Style. Also, naming convention has been changed.

hasButtonSwipeAnimationSupport <- hasButtonSwipeSupport

Note though, pref has been changed to general.buttonSwipeAnimSupport.

I will be starting a try build a little later, and use that for UX review. However, it is still unknown if we should use CSS transitions/animations instead. But, it should give the proper idea, and we can at least get UX review over with.

Comment 75

[Josiah Bruner [:jsbruner]]

Attachment: Combined patch of the other neccessary bugs. (Update 8)

Comment 76

[Josiah Bruner [:jsbruner]]

Attachment: Main Patch (V 0.5.1)

Main Patch, updated for current trunk and latest changes to bug dependencies. Also, is assuming all three parts for bug 678392.

Comment 77

[Josiah Bruner [:jsbruner]]

Try builds available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-2fef9d0e319e/

Comment 78

[Josiah Bruner [:jsbruner]]

Comment on attachment 713532 [details] [diff] [review]
Main Patch (V 0.5.1)

Asking Boriss for UI-review. Review has been put on the patch, but you should really just use the latest trybuilds. OS X in peticular, since it provides the "normal" experience, other platforms still have some issues.

Comment 79

[Josiah Bruner [:jsbruner]]

Attachment: Combined patch of the other neccessary bugs. (Update 9)

Comment 80

[Josiah Bruner [:jsbruner]]

Attachment: Main Patch (V 0.5.2)

Updated for current trunk and changes to bug 678392. Carrying ui-review flag for Jennifer. 

No other functional changes.

Comment 81

[Josiah Bruner [:jsbruner]]

Attachment: Main Patch (V 0.5.3)

Updated patch. Fixes an error causing the preference pane to break on OS X. Also adds an accesskey, though I am not yet sure if that is necessary.

Carrying ui-review flag.

Comment 82

[Josiah Bruner [:jsbruner]]

Latest try builds:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-02a3acdc1d5a/

Comment 83

[Josiah Bruner [:jsbruner]]

Attachment: Combined patch of the other neccessary bugs. (Update 10)

Updated for current trunk and latest changes to other bugs.

Comment 84

[Josiah Bruner [:jsbruner]]

Attachment: Main Patch (V 0.5.4)

Updated for current trunk. No functional changes. Carrying ui-review flag.

Also, I noticed that for some reason it appears that Linux runs the animation slower than OS X does. Since it is based on a time web api, I am curious on how this could occur. The animation is smooth, but takes longer. If anyone has any ideas on this let me know.

Comment 85

[Josiah Bruner [:jsbruner]]

Attachment: Main Patch (V 0.6)

Patch increases speed on all platforms to what it will most likely be when this gets pushed to users. Also changes the pref shortcut toggle to "u" instead of "Q".

Comment 86

[Josiah Bruner [:jsbruner]]

Latest try builds available at:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-7b0a04145bd7/

Note: All debugs failed test, I believe though that is because of another failure and is unrelated to this bug.

Comment 87

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Mockup: Forward button not appearing correctly in attachment 718635

(In reply to Josiah [:JosiahOne] from comment #86)
> Latest try builds available at:
> 
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/josiah@programmer.
> net-7b0a04145bd7/
> 
> Note: All debugs failed test, I believe though that is because of another
> failure and is unrelated to this bug.

I'm seeing both a significant performance delay (1) and a visual bug (2) on this.

(1) While swiping left or right the page appears quickly, once the new page has slotted into place, users should be ready to go and see no further movement.  However, once the old page has been swiped away, there's a delay for the page to reload and also a delay for the Forward button to appear (if it wasn't there already) or disappear (if it was there and no further forward page exists).

Perhaps for now we have to reload upon swipe, and this is unlikely a blocker.  However, the appearance and disappearance of the forward button needs to happen as quickly and smoothly as the page itself animates in.

(2) Sometimes when the forward button appears, it does not display correctly, but appears faded out on its the left side (2a).  It also sometimes doesn't appear correctly if the URL bar is focused during swipe (2b)

Comment 88

[Jennifer Morrow [:Boriss] (UX)]

Comment on attachment 718635 [details] [diff] [review]
Main Patch (V 0.6)

># HG changeset patch
># User JosiahOne <[e-mail]>
># Date 1361915019 18000
># Node ID 532944023da3a0efe6a7d305149621f8e2dcbd50
># Parent  e11f53d24e01e2a38689743c382a7375f90de7df
>Bug 817074: Made speed faster, changed shortcut pref toggle to u
>
>diff --git a/browser/base/content/browser-gestureSupport.js b/browser/base/content/browser-gestureSupport.js
>--- a/browser/base/content/browser-gestureSupport.js
>+++ b/browser/base/content/browser-gestureSupport.js
>@@ -526,33 +526,75 @@
>   active: false,
>   isLTR: false,
> 
>   /**
>    * Initializes the support for history swipe animations, if it is supported
>    * by the platform/configuration.
>    */
>   init: function HSA_init() {
>-    if (!this._isSupported() || this._getMaxSnapshots() < 1)
>+    if ((!this._isSupported() && !this.hasButtonSwipeAnimationSupport) || this._getMaxSnapshots() < 1)
>       return;
> 
>     gBrowser.addEventListener("pagehide", this, false);
>     gBrowser.addEventListener("pageshow", this, false);
>     gBrowser.addEventListener("popstate", this, false);
>     gBrowser.tabContainer.addEventListener("TabClose", this, false);
> 
>     this.active = true;
>     this.isLTR = document.documentElement.mozMatchesSelector(
>                                             ":-moz-locale-dir(ltr)");
>     this._trackedSnapshots = [];
>     this._historyIndex = -1;
>     this._boxWidth = -1;
>     this._maxSnapshots = this._getMaxSnapshots();
>     this._lastSwipeDir = "";
>   },
>+    
>+    /**
>+     * Checks to see if the animations for the back/forward buttons are enabled.
>+     * Return true if supported. False if not.
>+     */
>+    hasButtonSwipeAnimationSupport: function HSA_hasButtonSwipeAnimationSupport() {
>+    if (Services.prefs.getBoolPref("general.buttonSwipeAnimSupport")) {
>+        return true;
>+    }
>+    else {
>+        return false;
>+      }
>+    },
>+    
>+    /**
>+     * Check for a change in settings to initalize the swipe support again.
>+     * This is only neccessary if it has previously been disabled.
>+     */
>+    
>+    checkButtonSwipeAnimationSupport: function HSA_checkButtonSwipeAnimationSupport() {
>+    if (gHistorySwipeAnimation.active) {
>+        //Is active, check for a change in preferences
>+        if (this.hasButtonSwipeAnimationSupport()) {
>+            return true;
>+        }
>+        else {
>+            return false;
>+        }
>+    }
>+    else {
>+        //Was not initalized, check for change in preferences
>+        if (Services.prefs.getBoolPref("general.buttonSwipeAnimSupport")) {
>+            //Initalize it
>+            gHistorySwipeAnimation.init();
>+            return true;
>+        }
>+        else {
>+            //Don't initalize
>+            return false;
>+        }
>+      }
>+  },
> 
>   /**
>    * Uninitializes the support for history swipe animations.
>    */
>   uninit: function HSA_uninit() {
>     gBrowser.removeEventListener("pagehide", this, false);
>     gBrowser.removeEventListener("pageshow", this, false);
>     gBrowser.removeEventListener("popstate", this, false);
>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -1575,45 +1575,141 @@
>   }
>   // Modified click. Go there in a new tab/window.
> 
>   duplicateTabIn(gBrowser.selectedTab, where, index - gBrowser.sessionHistory.index);
>   return true;
> }
> 
> function BrowserForward(aEvent) {
>-  let where = whereToOpenLink(aEvent, false, true);
>-
>-  if (where == "current") {
>-    try {
>-      gBrowser.goForward();
>-    }
>-    catch(ex) {
>-    }
>+    let where = whereToOpenLink(aEvent, false, true);
>+    /*
>+     * Check to see if SwipeAnimation is active, can go forward, and that hasButtonSwipeSupport.
>+     * The later is used to execute the animation while hitting the forward button.
>+     * Note that the animation may still be active on OS X, but buttonSwiping is not.
>+     */
>+    
>+    if (where == "current") {
>+      if (gHistorySwipeAnimation.checkButtonSwipeAnimationSupport())
>+      {
>+        if (gHistorySwipeAnimation.active && gHistorySwipeAnimation.canGoForward() &&
>+                                    gHistorySwipeAnimation.hasButtonSwipeAnimationSupport()) {
>+          gBrowser.stop();
>+          gHistorySwipeAnimation.startAnimation();
>+          gHistorySwipeAnimation._historyIndex++;
>+                
>+          if (!window.requestAnimationFrameForward) {
>+            window.requestAnimationFrameForward = (window.mozRequestAnimationFrame ||
>+            function (callback) {
>+              //Return nothing here for the animation to update at a rate of ~60 FPS
>+            });
>+          }
>+                
>+          if (!window.cancelRequestAnimationFrameForward) {
>+            window.cancelRequestAnimationFrameForward = ( function() {
>+            return window.cancelAnimationFrame || window.mozCancelRequestAnimationFrame } )();
>+          }
>+          aTimeValue = Date.now();
>+          drawTheAnimationForward();
>+        }
>+     }
>+     else {
>+       try {
>+         gBrowser.goForward();
>+       }
>+         catch(ex) {
>+       }
>+     }
>   }
>   else {
>     duplicateTabIn(gBrowser.selectedTab, where, 1);
>   }
> }
> 
>+
>+function drawTheAnimationForward() {
>+  var functionTime = Date.now();
>+  //Verified to work. Animate based on time, and not anything else.
>+  var differenceTime = (functionTime - aTimeValue) / 1000;
>+  var request = window.requestAnimationFrameForward(drawTheAnimationForward);
>+    
>+  // Drawing code goes here
>+  gHistorySwipeAnimation.updateAnimation(((differenceTime * -1) * 4));
>+  if ((differenceTime *-1) <= ((1 / 4) * -1))
>+  {
>+    gHistorySwipeAnimation._navigateToHistoryIndex();
>+    window.cancelRequestAnimationFrameForward(request);
>+    return;
>+  }
>+}
>+
>+
> function BrowserBack(aEvent) {
>   let where = whereToOpenLink(aEvent, false, true);
>-
>-  if (where == "current") {
>-    try {
>-      gBrowser.goBack();
>-    }
>-    catch(ex) {
>+  /*
>+   * Check to see if SwipeAnimation is active, can go back, and that hasButtonSwipeSupport.
>+   * The later is used to execute the animation while hitting the back button.
>+   * Note that the animation may still be active on OS X, but buttonSwiping is not.
>+   */
>+     if (where == "current") {
>+       if (gHistorySwipeAnimation.checkButtonSwipeAnimationSupport())
>+       {
>+         if (gHistorySwipeAnimation.active && gHistorySwipeAnimation.canGoBack() &&
>+                                  gHistorySwipeAnimation.hasButtonSwipeAnimationSupport()) {
>+           gBrowser.stop();
>+           gHistorySwipeAnimation.startAnimation();
>+           gHistorySwipeAnimation._historyIndex--;
>+             
>+           if (!window.requestAnimationFrame) {
>+             window.requestAnimationFrame = (window.mozRequestAnimationFrame ||
>+             function (callback) {
>+             //Return nothing again
>+             });
>+           }
>+                
>+           if (!window.cancelRequestAnimationFrame) {
>+             window.cancelRequestAnimationFrame = ( function() {
>+             return window.cancelAnimationFrame || window.mozCancelRequestAnimationFrame } )();
>+           }
>+           aTimeValue = Date.now();
>+           drawTheAnimationBack();
>+        }
>+    }
>+    else {
>+      try {
>+        gBrowser.goBack();
>+      }
>+      catch(ex) {
>+      }
>     }
>   }
>   else {
>     duplicateTabIn(gBrowser.selectedTab, where, -1);
>   }
> }
> 
>+
>+
>+function drawTheAnimationBack() {
>+  var functionTime = Date.now();
>+    
>+  var differenceTime = (functionTime - aTimeValue) / 1000; //Verified to work. Animate based on time, and not anything else.
>+    
>+  var request = window.requestAnimationFrame(drawTheAnimationBack);
>+    
>+  // Drawing code goes here
>+  gHistorySwipeAnimation.updateAnimation((differenceTime * 4));
>+  if (differenceTime >= (1 / 4))
>+  {
>+    gHistorySwipeAnimation._navigateToHistoryIndex();
>+    window.cancelRequestAnimationFrame(request);
>+    return;
>+  }
>+}
>+
> function BrowserHandleBackspace()
> {
>   switch (gPrefService.getIntPref("browser.backspace_action")) {
>   case 0:
>     BrowserBack();
>     break;
>   case 1:
>     goDoCommand("cmd_scrollPageUp");
>diff --git a/browser/components/preferences/advanced.xul b/browser/components/preferences/advanced.xul
>--- a/browser/components/preferences/advanced.xul
>+++ b/browser/components/preferences/advanced.xul
>@@ -25,17 +25,18 @@
>                   type="int"/>
> 
>       <!--XXX button prefs -->
> 
>       <!-- General tab -->
>       <preference id="accessibility.browsewithcaret"   name="accessibility.browsewithcaret"   type="bool"/>
>       <preference id="accessibility.typeaheadfind"     name="accessibility.typeaheadfind"     type="bool"/>
>       <preference id="accessibility.blockautorefresh"  name="accessibility.blockautorefresh"  type="bool"/>
>-
>+        
>+      <preference id="general.buttonSwipeAnimSupport"  name="general.buttonSwipeAnimSupport"  type="bool"/>
>       <preference id="general.autoScroll"              name="general.autoScroll"              type="bool"/>
>       <preference id="general.smoothScroll"            name="general.smoothScroll"            type="bool"/>
>       <preference id="layers.acceleration.disabled"    name="layers.acceleration.disabled"    type="bool"   inverted="true"
>                   onchange="gAdvancedPane.updateHardwareAcceleration()"/>
> #ifdef XP_WIN
>       <preference id="gfx.direct2d.disabled"           name="gfx.direct2d.disabled"           type="bool"   inverted="true"/>
> #endif
>       <preference id="layout.spellcheckDefault"        name="layout.spellcheckDefault"        type="int"/>
>@@ -167,16 +168,20 @@
>                       accesskey="&allowHWAccel.accesskey;"
>                       preference="layers.acceleration.disabled"/>
>             <checkbox id="checkSpelling"
>                       label="&checkSpelling.label;"
>                       accesskey="&checkSpelling.accesskey;"
>                       onsyncfrompreference="return gAdvancedPane.readCheckSpelling();"
>                       onsynctopreference="return gAdvancedPane.writeCheckSpelling();"
>                       preference="layout.spellcheckDefault"/>
>+            <checkbox id="useSwiping"
>+                      label="&buttonSwipe.label;"
>+                      accesskey="&buttonSwipe.accesskey;"
>+                      preference="general.buttonSwipeAnimSupport"/>
>           </groupbox>
> 
> #ifdef HAVE_SHELL_SERVICE
>           <!-- System Defaults -->
>           <groupbox id="systemDefaultsGroup" orient="vertical">
>             <caption label="&systemDefaults.label;"/>
> 
>             <checkbox id="alwaysCheckDefault" preference="browser.shell.checkDefaultBrowser"
>diff --git a/browser/locales/en-US/chrome/browser/preferences/advanced.dtd b/browser/locales/en-US/chrome/browser/preferences/advanced.dtd
>--- a/browser/locales/en-US/chrome/browser/preferences/advanced.dtd
>+++ b/browser/locales/en-US/chrome/browser/preferences/advanced.dtd
>@@ -20,16 +20,18 @@
> <!ENTITY useAutoScroll.label             "Use autoscrolling">
> <!ENTITY useAutoScroll.accesskey         "a">
> <!ENTITY useSmoothScrolling.label        "Use smooth scrolling">
> <!ENTITY useSmoothScrolling.accesskey    "m">
> <!ENTITY allowHWAccel.label              "Use hardware acceleration when available">
> <!ENTITY allowHWAccel.accesskey          "r">
> <!ENTITY checkSpelling.label             "Check my spelling as I type">
> <!ENTITY checkSpelling.accesskey         "t">
>+<!ENTITY buttonSwipe.label               "Use swiping when pressing buttons.">
>+<!ENTITY buttonSwipe.accesskey           "u">
> 
> <!ENTITY systemDefaults.label            "System Defaults">
> <!ENTITY alwaysCheckDefault.label        "Always check to see if &brandShortName; is the default browser on startup"><!--XXX-->
> <!ENTITY alwaysCheckDefault.accesskey    "w">
> <!ENTITY setDefault.label                "Make &brandShortName; the default browser">
> <!ENTITY setDefault.accesskey            "d">
> <!ENTITY isDefault.label                 "&brandShortName; is currently your default browser">
> 
>diff --git a/modules/libpref/src/init/all.js b/modules/libpref/src/init/all.js
>--- a/modules/libpref/src/init/all.js
>+++ b/modules/libpref/src/init/all.js
>@@ -1557,16 +1557,19 @@
> pref("general.smoothScroll.other", true);
> // To connect consecutive scroll events into a continuous flow, the animation's duration
> // should be longer than scroll events intervals (or else the scroll will stop
> // before the next event arrives - we're guessing next interval by averaging recent
> // intervals).
> // This defines how longer is the duration compared to events interval (percentage)
> pref("general.smoothScroll.durationToIntervalRatio", 200);
> 
>+//Button Swipe Prefs
>+pref("general.buttonSwipeAnimSupport", true);
>+
> pref("profile.confirm_automigration",true);
> // profile.migration_behavior determines how the profiles root is set
> // 0 - use NS_APP_USER_PROFILES_ROOT_DIR
> // 1 - create one based on the NS4.x profile root
> // 2 - use, if not empty, profile.migration_directory otherwise same as 0
> pref("profile.migration_behavior",0);
> pref("profile.migration_directory", "");
>

Comment 89

[Josiah Bruner [:jsbruner]]

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #87)
> Created attachment 721931 [details]
> Mockup: Forward button not appearing correctly in attachment 718635 [details] [diff] [review]
> [details] [diff] [review]
> 
> (In reply to Josiah [:JosiahOne] from comment #86)
> > Latest try builds available at:
> > 
> > http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/josiah@programmer.
> > net-7b0a04145bd7/
> > 
> > Note: All debugs failed test, I believe though that is because of another
> > failure and is unrelated to this bug.
> 
> I'm seeing both a significant performance delay (1) and a visual bug (2) on
> this.
> 
> (1) While swiping left or right the page appears quickly, once the new page
> has slotted into place, users should be ready to go and see no further
> movement.  However, once the old page has been swiped away, there's a delay
> for the page to reload and also a delay for the Forward button to appear (if
> it wasn't there already) or disappear (if it was there and no further
> forward page exists).
> 
> Perhaps for now we have to reload upon swipe, and this is unlikely a
> blocker.  However, the appearance and disappearance of the forward button
> needs to happen as quickly and smoothly as the page itself animates in.
> 
> (2) Sometimes when the forward button appears, it does not display
> correctly, but appears faded out on its the left side (2a).  It also
> sometimes doesn't appear correctly if the URL bar is focused during swipe
> (2b)

Thanks for the review.

1. I really should have mentioned this when I requested ui-review, but quite a bit of this (and both of your problems) aren't really related to this bug, but instead bug 678392.

Bug 678392 might be the cause here, but either way I suppose if I call browser reload before the animation starts/during the animation, then we should be ready to go, and the forward button should show/hide itself during the page animations.

NOTE: Gesture swiping is NOT my bug, see bug listed above.

2. Does this happen when using the button only, or does this occur when you use swiping? For this bug you should ignore the swiping on the trackpad. Button only.

Comment 90

[Josiah Bruner [:jsbruner]]

Also, forward button rendering is bug 800443. So unrelated here.

Comment 91

[Jennifer Morrow [:Boriss] (UX)]

(In reply to Josiah [:JosiahOne] from comment #89)
> 1. I really should have mentioned this when I requested ui-review, but quite
> a bit of this (and both of your problems) aren't really related to this bug,
> but instead bug 678392.

That's fair enough - it's clear from your patch and those bugs they're preexisting.  Both Bug 817700 and Bug 678392 will need a ui-review on those aspects, fwiw.  I'll note that on those bugs.

> Bug 678392 might be the cause here, but either way I suppose if I call
> browser reload before the animation starts/during the animation, then we
> should be ready to go, and the forward button should show/hide itself during
> the page animations.
> 
> NOTE: Gesture swiping is NOT my bug, see bug listed above.
> 
> 2. Does this happen when using the button only, or does this occur when you
> use swiping? For this bug you should ignore the swiping on the trackpad.
> Button only.

I'm not able to reproduce this on button/keyboard only, so let's move ahead with this patch and address the reload/artifact problems in Bug 817700 and Bug 678392.

Comment 92

[Josiah Bruner [:jsbruner]]

Great. So UI review is done. Issues with this bug seem minimal, and I at least am not going to worry about using CSS at all. Performance is excellent currently, and this keeps us from maintaining two different drawing functions.

If people could test this and leave comments about any issues that would be great.

Comment 93

[Josiah Bruner [:jsbruner]]

Comment on attachment 718635 [details] [diff] [review]
Main Patch (V 0.6)

Stephen, do you have time to review this?

Comment 94

[Stephen A Pohl [:spohl]]

Comment on attachment 718635 [details] [diff] [review]
Main Patch (V 0.6)

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

This needs a bit more work. Here is some feedback.
Once you've addressed this feedback please send it to :felipe for a review.

* Please remove trailing white space (everywhere)
* Respect 80 character limit (everywhere)
* Fix indent to 2 spaces (everywhere)

::: browser/base/content/browser-gestureSupport.js
@@ +553,5 @@
> +     * Checks to see if the animations for the back/forward buttons are enabled.
> +     * Return true if supported. False if not.
> +     */
> +    hasButtonSwipeAnimationSupport: function HSA_hasButtonSwipeAnimationSupport() {
> +    if (Services.prefs.getBoolPref("general.buttonSwipeAnimSupport")) {

Fix indent and remove braces to match the rest of the file. No braces needed with only one line following the if clause. (fix everywhere)

@@ +556,5 @@
> +    hasButtonSwipeAnimationSupport: function HSA_hasButtonSwipeAnimationSupport() {
> +    if (Services.prefs.getBoolPref("general.buttonSwipeAnimSupport")) {
> +        return true;
> +    }
> +    else {

No need for an else clause. Simply return false. (fix everywhere)

@@ +584,5 @@
> +            gHistorySwipeAnimation.init();
> +            return true;
> +        }
> +        else {
> +            //Don't initalize

s/initalize/initialize/

::: browser/base/content/browser.js
@@ +1580,5 @@
>  }
>  
>  function BrowserForward(aEvent) {
> +    let where = whereToOpenLink(aEvent, false, true);
> +    /*

add new line before comment

@@ +1585,5 @@
> +     * Check to see if SwipeAnimation is active, can go forward, and that hasButtonSwipeSupport.
> +     * The later is used to execute the animation while hitting the forward button.
> +     * Note that the animation may still be active on OS X, but buttonSwiping is not.
> +     */
> +    

remove empty line after comment

@@ +1590,5 @@
> +    if (where == "current") {
> +      if (gHistorySwipeAnimation.checkButtonSwipeAnimationSupport())
> +      {
> +        if (gHistorySwipeAnimation.active && gHistorySwipeAnimation.canGoForward() &&
> +                                    gHistorySwipeAnimation.hasButtonSwipeAnimationSupport()) {

fix vertical alignment (everywhere)

@@ +1593,5 @@
> +        if (gHistorySwipeAnimation.active && gHistorySwipeAnimation.canGoForward() &&
> +                                    gHistorySwipeAnimation.hasButtonSwipeAnimationSupport()) {
> +          gBrowser.stop();
> +          gHistorySwipeAnimation.startAnimation();
> +          gHistorySwipeAnimation._historyIndex++;

Don't access private members and methods directly. Write appropriate getters/setters and/or helper functions in gHistorySwipeAnimation.

@@ +1596,5 @@
> +          gHistorySwipeAnimation.startAnimation();
> +          gHistorySwipeAnimation._historyIndex++;
> +                
> +          if (!window.requestAnimationFrameForward) {
> +            window.requestAnimationFrameForward = (window.mozRequestAnimationFrame ||

You don't need this. Simply call window.mozRequestAnimationFrame.

@@ +1624,5 @@
>    }
>  }
>  
> +
> +function drawTheAnimationForward() {

drop "The" in function name.

@@ +1634,5 @@
> +  // Drawing code goes here
> +  gHistorySwipeAnimation.updateAnimation(((differenceTime * -1) * 4));
> +  if ((differenceTime *-1) <= ((1 / 4) * -1))
> +  {
> +    gHistorySwipeAnimation._navigateToHistoryIndex();

don't call private members/methods on gHistorySwipeAnimation.

@@ +1635,5 @@
> +  gHistorySwipeAnimation.updateAnimation(((differenceTime * -1) * 4));
> +  if ((differenceTime *-1) <= ((1 / 4) * -1))
> +  {
> +    gHistorySwipeAnimation._navigateToHistoryIndex();
> +    window.cancelRequestAnimationFrameForward(request);

cancelRequestAnimationFrameForward should be called before _navigateToHistoryIndex.

@@ +1636,5 @@
> +  if ((differenceTime *-1) <= ((1 / 4) * -1))
> +  {
> +    gHistorySwipeAnimation._navigateToHistoryIndex();
> +    window.cancelRequestAnimationFrameForward(request);
> +    return;

no need for return here.

@@ +1645,1 @@
>  function BrowserBack(aEvent) {

Refactor and consolidate the animation code in BrowserForward and BrowserBack. For example, write a helper function that takes a bool argument and either animate forward or backwards based on the value. This avoids code duplication.

@@ +1687,5 @@
>  }
>  
> +
> +
> +function drawTheAnimationBack() {

Refactor drawTheAnimationForward and drawTheAnimationBack to avoid code duplication.

::: browser/components/preferences/advanced.xul
@@ +175,5 @@
>                        preference="layout.spellcheckDefault"/>
> +            <checkbox id="useSwiping"
> +                      label="&buttonSwipe.label;"
> +                      accesskey="&buttonSwipe.accesskey;"
> +                      preference="general.buttonSwipeAnimSupport"/>

s/buttonSwipeAnimSupport/buttonSwipeAnimationSupport/

::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd
@@ +24,5 @@
>  <!ENTITY allowHWAccel.label              "Use hardware acceleration when available">
>  <!ENTITY allowHWAccel.accesskey          "r">
>  <!ENTITY checkSpelling.label             "Check my spelling as I type">
>  <!ENTITY checkSpelling.accesskey         "t">
> +<!ENTITY buttonSwipe.label               "Use swiping when pressing buttons.">

s/swiping/swipe animations/
s/pressing buttons/pressing back/forward buttons/

::: modules/libpref/src/init/all.js
@@ +1561,5 @@
>  // intervals).
>  // This defines how longer is the duration compared to events interval (percentage)
>  pref("general.smoothScroll.durationToIntervalRatio", 200);
>  
> +//Button Swipe Prefs

s/Swipe/Swipe Animation/

Comment 95

[Josiah Bruner [:jsbruner]]

Attachment: Main Patch (V 1.0)

Addressed feedback from Stephen. Carrying ui-review+ flag. Asking :felipe for review.

Comment 96

[Jennifer Morrow [:Boriss] (UX)]

A few things.  

1. We should have this pref'd off until there's wider UX feedback.  Swiping gestures make sense for this behavior by default, but applying animation on every back/forward click is a major change for the browser.  I'm happy to approve this to land now if it is pref'd off by default, and will bring this to the team for feedback.

2. The Advanced Setting string needs to be changed, as there are a few problems with it.  First, it shouldn't end in a period/full-stop (".") for consistency.  Second, the phrase is confusing.  Currently it says "Use swiping when pressing buttons." - this doesn't indicate what buttons or when this would be used.

Let's change the string to "Animate pages when clicking back and forward."  I'll talk to the UX team today.  This needs another ui review before it lands.

Comment 97

[Josiah Bruner [:jsbruner]]

Attachment: Main Patch (V 1.0.1)

Addressed ui feedback. Pref starts out disabled now. Fixed preference string. Re-flagging Boriss for ui-review. Felipe still has the review.

Comment 98

[Justin Dolske [:Dolske]]

And, again, we need to understand the performance impact of this (both subjective and quantitative) before it land, even pref'd off.

TBH, I'm not sure we should even take this. Animations on non-gesture forward/back doesn't found fantastic UX wise. What's the rationale?

Comment 99

[Josiah Bruner [:jsbruner]]

(In reply to Justin Dolske [:Dolske] from comment #98)
> And, again, we need to understand the performance impact of this (both
> subjective and quantitative) before it land, even pref'd off.
> 
> TBH, I'm not sure we should even take this. Animations on non-gesture
> forward/back doesn't found fantastic UX wise. What's the rationale?

Performance - I agree we should examine this, but am unsure of how to approach this test.

As for your UX-wise, it does make sense really. The back and forward buttons are too navigate between pages. Before it would just flash. This makes more sense, it just hasn't really been done. But of course, it's up to UX in the end. Jennifer is talking to the UX team today anyway. Hopefully we'll get some feedback then.

Comment 100

[Jennifer Morrow [:Boriss] (UX)]

(In reply to Justin Dolske [:Dolske] from comment #98)
> And, again, we need to understand the performance impact of this (both
> subjective and quantitative) before it land, even pref'd off.
> 
> TBH, I'm not sure we should even take this. Animations on non-gesture
> forward/back doesn't found fantastic UX wise. What's the rationale?

For swiping on a trackpad physically, this behavior makes sense because it gives a visual change to an invisible process.  When swiping on a trackpad, there's currently no visual feedback that a user is approaching the threshold for going forward and back.

For clicking, there *is* visual feedback: users are clicking on a visual button and seeing it depress.  I agree that this patch does not add value to non-trackpad behavior, but I'd be fine to ui+ pref off as an advanced option, simply because as an advanced option there's little downside to making the behavior available.  If there's any sort of performance regression at all, we should absolutely block on that.

Comment 101

[Jennifer Morrow [:Boriss] (UX)]

(In reply to Justin Dolske [:Dolske] from comment #98)
> And, again, we need to understand the performance impact of this (both
> subjective and quantitative) before it land, even pref'd off.
> 
> TBH, I'm not sure we should even take this. Animations on non-gesture
> forward/back doesn't found fantastic UX wise. What's the rationale?

How do you feel about this being an about:config setting, assuming absolutely no performance regression?  I think it's more suited to that than "advanced" options.

Comment 102

[Jennifer Morrow [:Boriss] (UX)]

This behavior really doesn't provide any benefit to users, and features that don't provide user benefit are not what we should be tacking onto about:config or Settings, even Advanced Settings.  Josiah, we really appreciate the work here, and I'd love to see this as an add-on on AMO.

Comment 103

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 724968 [details] [diff] [review]
Main Patch (V 1.0.1)

(just clearing up the review request. At the time I gave Josiah direct feedback on IRC about the patch)

Comment 104

[Sean]

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #102)
> This behavior really doesn't provide any benefit to users, and features that
> don't provide user benefit are not what we should be tacking onto
> about:config or Settings, even Advanced Settings.

I disagree; this is important UI feedback. Swiping is an analog action, and requires an "analog" representation on-screen. Otherwise, you don't know how far you have to swipe to complete the action or how far to "un-swipe" to abort the action.

Additionally, I believe this is a factor in keeping people from switching to Firefox on OS X. Safari does this, and users get used to it, prefer it, and feel its absence in other browsers.

Comment 105

[José Jeria]

(In reply to Sean from comment #104)
> I disagree; this is important UI feedback. Swiping is an analog action, and
> requires an "analog" representation on-screen. Otherwise, you don't know how
> far you have to swipe to complete the action or how far to "un-swipe" to
> abort the action.
> 
> Additionally, I believe this is a factor in keeping people from switching to
> Firefox on OS X. Safari does this, and users get used to it, prefer it, and
> feel its absence in other browsers.

You seem to have misunderstood what this bug is about. This is not about adding swiping support to Firefox, but to show the swipe animation when the user clicks on the back/forward buttons. Something that Safari does not do.

You are probably looking for bug 860493 which is currently worked on.

Comment 106

[Mark Banner (:standard8)]

Comment on attachment 724968 [details] [diff] [review]
Main Patch (V 1.0.1)

Clearing old ui-review request that was essentially given in comment 100 to comment 102.