860493 - (history-swipe) Enable history swipe animations by default using arrow indicators

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect, P1)

Description

[Stephen A Pohl [:spohl] (OOO until 11/7)]

+++ This bug was initially created as a clone of Bug #678392 +++

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7) AppleWebKit/534.48.3 (KHTML, like Gecko) Version/5.1 Safari/534.48.3

Steps to reproduce:

By default, history swipe animations (bug 678392) are currently turned off. Enable them once bug 817700 is fixed.


Actual results:

No history swipe animations by default.


Expected results:

History swipe animations by default.

Comment 1

[Stephen A Pohl [:spohl] (OOO until 11/7)]

Attachment: Patch

Comment 2

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 735992 [details] [diff] [review]
Patch

Fine with me, on the assumption you'll wait to land it until bug 817700 is fixed.

Comment 3

[Stephen A Pohl [:spohl] (OOO until 11/7)]

Bug 817700 landed on mozilla-inbound, so requesting checkin for this patch.

Comment 4

[Lukas Blakk [:lsblakk] use ?needinfo]

this would be a ff24 note if bug 817700 lands successfully there.

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to [e-mail] [:lsblakk] from comment #4)
> this would be a ff24 note if bug 817700 lands successfully there.

it's in our relnote db still, but pushed to ff99 until we have a release to attach to.

Comment 6

[Stephen A Pohl [:spohl] (OOO until 11/7)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6155ea7e8dea

Comment 7

[Stephen A Pohl [:spohl] (OOO until 11/7)]

FYI: This feature, its impact on performance etc. has also been discussed in mozilla.dev.platform:
https://groups.google.com/forum/#!topic/mozilla.dev.platform/NOCX_tDfFCA

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6155ea7e8dea

Comment 9

[Matt Brubeck (:mbrubeck)]

This caused some regressions in tpaint and in some pageloader tests (Tp5, SVG, SVG Opacity):

https://groups.google.com/d/topic/mozilla.dev.tree-management/4tZ0XGOXq2Y/discussion
https://groups.google.com/d/topic/mozilla.dev.tree-management/OmqHXxwUQs8/discussion

Comment 10

[Stephen A Pohl [:spohl] (OOO until 11/7)]

mconley was kind enough to send me to this graph [1], which is illustrating the performance regressions mentioned in comment 9.

Benoit, I hear that you might be able to help me understand these regressions better. I believe some amount of regression is inevitable for this feature, but it would help if we had a better understanding of the numbers and whether we can tweak any of the code to reduce these regressions. Please let me know how I can help. I hear SPS profiles are handy sometimes. If you don't have time for this, would you be able to point me to someone else?

Also setting n-i for mconley to get more info on how to create comparison profiles.

[1] https://datazilla.mozilla.org/?start=1382259636&stop=1382547348&product=Firefox&repository=Mozilla-Inbound&os=mac&os_version=OS%20X%2010.8&test=tpaint&graph_search=6155ea7e8dea&project=talos

Comment 11

[Axel Hecht [:Pike]]

On my retina powerbook, I'm not happy with the UX here. I get a blurry screenshot on back and forth, and then it takes about a second to get the actual page to show. It actually feels slow and lagging like this.

Comment 12

[Stephen A Pohl [:spohl] (OOO until 11/7)]

(In reply to Axel Hecht [:Pike] from comment #11)
> On my retina powerbook, I'm not happy with the UX here. I get a blurry
> screenshot on back and forth,

See bug 780362. Note that not having a 'blurry' screenshot would basically quadruple the memory requirements to store the snapshots. This may be a tough sell.

> and then it takes about a second to get the
> actual page to show. It actually feels slow and lagging like this.

This may be worth a bug to track and investigate. Is this true for all websites, or specific ones? How does Safari compare?

Comment 13

[José Jeria]

(In reply to Stephen Pohl [:spohl] from comment #12)
> This may be worth a bug to track and investigate. 

Bug 930768 was opened for this

Comment 14

[Axel Hecht [:Pike]]

(In reply to Stephen Pohl [:spohl] from comment #12)
> (In reply to Axel Hecht [:Pike] from comment #11)
> > On my retina powerbook, I'm not happy with the UX here. I get a blurry
> > screenshot on back and forth,
> 
> See bug 780362. Note that not having a 'blurry' screenshot would basically
> quadruple the memory requirements to store the snapshots. This may be a
> tough sell.

The problem is that it's using the blurry shot even though there's no need. This is pretty disturbing when we used to have bfcache, and even more so when you hit the top and bottom overscroll a bit, at which point the page you're reading suddenly turns blurry for no apparent reason.

> > and then it takes about a second to get the
> > actual page to show. It actually feels slow and lagging like this.
> 
> This may be worth a bug to track and investigate. Is this true for all
> websites, or specific ones? How does Safari compare?

This happens for all pages.

Comment 15

[Stephen A Pohl [:spohl] (OOO until 11/7)]

Thanks, Axel and José. I'm going to suggest backing this out due to bug 930768. We really shouldn't refresh pages where we haven't previously.

Comment 16

[Stephen A Pohl [:spohl] (OOO until 11/7)]

Backed out for causing bug 930768:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a25982be666c

Comment 17

[Axel Hecht [:Pike]]

Thanks. In particular given how close we are to merge day, I appreciate the back-out.

I'd suggest that we try to find an implementation of this that relies a lot less on captured images, and keeps the actual web page around in more scenarios.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

(In reply to Stephen Pohl [:spohl] from comment #10)
> Also setting n-i for mconley to get more info on how to create comparison
> profiles.
> 

Hey Stephen,

Comparison profiles were a key part of how we fixed the Australis tpaint regression. Basically, we push two patches to try - one in the "ideal" state (m-c), and one in the regressed state (your patch). The patches aren't empty though - they redirect try to run a special version of talos that dumps profiles after the talos run.

Once the try pushes have finished running the tpaint test a few times, we have some scripts that can extract the profiles from the talos logs. We can then create a comparison profile with both profiles to see what is uniquely expensive in the "regressing" patch.

I'll push the patches to try to create the initial comparison profile for you to work off of. It's been a while since I've done one of these for ts_paint / tpaint, so it might take a few tries. I'll let you know when it's done. After that, I'll show you how you can create follow-up comparison profiles as you progress.

Comment 19

[David Teller [:Yoric] - still alive but not very active]

It would be great to have some telemetry on the speed of capture.

Comment 20

[David Teller [:Yoric] - still alive but not very active]

Attachment: Telemetry

Comment 21

[Stephen A Pohl [:spohl] (OOO until 11/7)]

Comment on attachment 823927 [details] [diff] [review]
Telemetry

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

Thank you! Looks good to me!

::: browser/base/content/browser-gestureSupport.js
@@ +946,1 @@
>                                            "canvas");

nit: fix indent

@@ +950,5 @@
> +      let ctx = canvas.getContext("2d");
> +      let zoom = browser.markupDocumentViewer.fullZoom;
> +      ctx.scale(zoom, zoom);
> +      ctx.drawWindow(browser.contentWindow, 0, 0, r.width, r.height, "white",
> +      ctx.DRAWWINDOW_DO_NOT_FLUSH | ctx.DRAWWINDOW_DRAW_VIEW |

nit: ctx.DRAWWINDOW_DO_NOT_FLUSH should be aligned with browser.contentWindow

Comment 22

[Stephen A Pohl [:spohl] (OOO until 11/7)]

Attachment: Telemetry

Fixed indent and updated to apply over the patch in bug 867629, which will land on inbound shortly. Carrying over r+.

Comment 23

[bhavana bajaj [:bajaj]]

history swipe animations are still turned off by default in aurora, so leaving it out for release notes on Fx27

Comment 24

[Stephen A Pohl [:spohl] (OOO until 11/7)]

Since merge day has passed and we're at the beginning of a new cycle I'm considering turning this back on in the next day or so (with added telemetry). We'll continue working through the performance regressions. Please let me know if I shouldn't do that.

(In reply to Axel Hecht [:Pike] [pto: November 4] from comment #17) 
> I'd suggest that we try to find an implementation of this that relies a lot
> less on captured images, and keeps the actual web page around in more
> scenarios.

Thanks! Is this to resolve the performance regressions? In terms of memory usage, or responsiveness? I'm definitely open to new and better suggestions, but I think we'd need to understand in what way this would improve performance. FYI: Markus just filed bug 933389, which should make the animations less janky.

Comment 25

[Stephen A Pohl [:spohl] (OOO until 11/7)]

Attachment: Patch

Updated patch to ensure that this remains turned off in release channel until we're sure it should be turned on. Nightly and aurora will allow us to gather feedback and bugs. Asking for another quick review just to be safe.

Comment 26

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 826798 [details] [diff] [review]
Patch

Sounds like a good idea to me.

Comment 27

[Stephen A Pohl [:spohl] (OOO until 11/7)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/09725c69fd76
https://hg.mozilla.org/integration/mozilla-inbound/rev/f488602d26c9

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7384e5f3eb7a

https://tbpl.mozilla.org/php/getParsedLog.php?id=30085139&tree=Mozilla-Inbound

Comment 29

[Stephen A Pohl [:spohl] (OOO until 11/7)]

Comment on attachment 825434 [details] [diff] [review]
Telemetry

This doesn't seem to compile (see comment 28). Yoric, do you know what's going on here?

Comment 30

[Steven Michaud [:smichaud] (Retired)]

Oops.  If I remember right, if you want to use the #ifdef [NAME] or #ifndef [NAME] syntax in a JavaScript file, you have to explicitly allow it (in some other special file).  Let me see if I can find that file.

(I hadn't noticed that you were trying to use this syntax in a JavaScript file.)

Comment 31

[Stephen A Pohl [:spohl] (OOO until 11/7)]

(In reply to Steven Michaud from comment #30)
> Oops.  If I remember right, if you want to use the #ifdef [NAME] or #ifndef
> [NAME] syntax in a JavaScript file, you have to explicitly allow it (in some
> other special file).  Let me see if I can find that file.
> 
> (I hadn't noticed that you were trying to use this syntax in a JavaScript
> file.)

It's the telemetry patch that caused the bustage, not the patch to enable the feature. The firefox.js file already uses this same #define in other scenarios.

Comment 32

[Stephen A Pohl [:spohl] (OOO until 11/7)]

Landed without telemetry patch for now:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ea21428f70c

Comment 33

[Benoit Girard (:BenWa)]

(In reply to Stephen Pohl [:spohl] from comment #10)
> mconley was kind enough to send me to this graph [1], which is illustrating
> the performance regressions mentioned in comment 9.
> 
> Benoit, I hear that you might be able to help me understand these
> regressions better. I believe some amount of regression is inevitable for
> this feature, but it would help if we had a better understanding of the
> numbers and whether we can tweak any of the code to reduce these
> regressions. Please let me know how I can help. I hear SPS profiles are
> handy sometimes. If you don't have time for this, would you be able to point
> me to someone else?
> 
> Also setting n-i for mconley to get more info on how to create comparison
> profiles.
> 
> [1]
> https://datazilla.mozilla.org/
> ?start=1382259636&stop=1382547348&product=Firefox&repository=Mozilla-
> Inbound&os=mac&os_version=OS%20X%2010.
> 8&test=tpaint&graph_search=6155ea7e8dea&project=talos

Follow the instructions on this page:
https://developer.mozilla.org/en-US/docs/Performance/Profiling_with_the_Built-in_Profiler

for OSX now that is has OMTC by default you want to select Multi-thread and enter the thread name: 'GeckoMain,Compositor' and this will additional show the frame times.

Comment 34

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #30)

You're right, Stephen, that this has nothing to do with the build failures (which as you say were due to the Telemetry patch).  But I wanted to follow up on this and find out if my (rather dim) memory is accurate and still true.

It is :-)

For every js/xul file in which you want to use the #ifdef/#indef/#defined syntax, there needs to be a '*' on the left of its entry in its jar.mn file.  For example:

https://hg.mozilla.org/mozilla-central/annotate/fadc8a168bbc/browser/base/jar.mn#l111

Comment 35

[Stephen A Pohl [:spohl] (OOO until 11/7)]

(In reply to Steven Michaud from comment #34)
> For every js/xul file in which you want to use the #ifdef/#indef/#defined
> syntax, there needs to be a '*' on the left of its entry in its jar.mn file.
> For example:

Oh, I see. I can't find firefox.js in a jar.mn file though, even though the file uses these #defines throughout...

Comment 36

[Mike Conley (:mconley) (:⚙️)]

Yeah, firefox.js is already being pre-processed - as you mentioned, there are ifdef's all over the place already.

The compile failures you were experiencing in comment 28 have everything to do with the Telemetry probes, and nothing to do with the firefox.js prefs file.

Comment 37

[Steven Michaud [:smichaud] (Retired)]

> I can't find firefox.js in a jar.mn file though

Now that you say this, I can't either.  But I suspect that this is because it (and possibly also a small number of other js/xul files) are special-cased to always allow the #ifdef/#ifndef/#defined syntax.

The special-casing (or at least some of it) may take place here (in the [Default Preferences] section of the package-manifest.in file:

https://hg.mozilla.org/mozilla-central/annotate/fadc8a168bbc/browser/installer/package-manifest.in#l604

Comment 38

[Robert Strong (they/them - no direct email)]

We've had multiple methods available to preprocess files (including js files) for ages and as noted by mconley firefox.js is preprocessed. In the case of firefox.js I am fairly certain it is handled by the following

http://mxr.mozilla.org/mozilla-central/source/browser/app/Makefile.in#11
http://mxr.mozilla.org/mozilla-central/source/js/src/config/rules.mk#1168

The PP_TARGETS handles preprocessing (PP stands for preprocessing)

Comment 39

[Mike Conley (:mconley) (:⚙️)]

Stephen:

I've successfully created a comparison profile between m-c with history swipe disabled, and m-c with history swipe disabled for the tpaint test.

All samples within this profile are extracted between measurement markers set with tpaint starts recording time, and when it stops - so these are the most relevant samples, with all of the noise stripped out. It doesn't get more distilled than this.

Here's the profile:

http://tests.themasta.com/cleopatra/?report=1d69898529b24d6c31b9bc4ae8756cc33e94c3b2

How to read this profile:

We sample at a rate of 1 sample per millisecond for both profiles, and then weight the samples from the "before" (m-c with no history swipe) with -1. We then toss the samples from the "after" profile (m-c with history swipe) on top.

The result - in the left-most column of each row in the call tree, you see how many milliseconds something takes *more* in the after profile than in the before.

So what you're looking for is stuff in here with high numbers in the left-most column.

When you select a row, the list on the right lists the most expensive functions in that rows children, in order of most expensive (and therefore most interesting) to least.

For you, something called "_ZL19PtrToNodeMatchEntryP12PLDHashTablePK15PLDHashEntryHdrPKv" is costing more in after than before. That looks like a hashtable lookup.

Looking up the callstack, that looks like its a function called by the garbage collector...so I think you may have triggered a GC during the tpaint, and that's contributing to the regression.

You can also filter by name, and only show Javascript calls. I've done that, and HSA__assignSnapshotToCurrentBrowser() in browser.js is costing a lot in "after" - maybe you can find a way of deferring that until after the first paint.

Anyhow, that's a glance. Ping me in IRC tomorrow if you want more help piecing stuff out of this profile.

Comment 40

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

https://hg.mozilla.org/mozilla-central/rev/2ea21428f70c

Comment 41

[David Teller [:Yoric] - still alive but not very active]

Oops, forgot a blocker: bug 860493.

Comment 42

[David Teller [:Yoric] - still alive but not very active]

I meant bug 932281.

Comment 43

[Axel Hecht [:Pike]]

FWIW, I disabled it in my local profile again, it's still not really an option on a retina display.

Comment 44

[Stephen A Pohl [:spohl] (OOO until 11/7)]

With bug 932281 landed we should be ready to land the telemetry patch. I'll set the telemetry patch back to r+ and land it once the try run comes back green:
https://tbpl.mozilla.org/?tree=Try&rev=8fb49ffdea69

Comment 45

[Jonathan Kew [:jfkthame]]

I noticed this had relanded for Nightly (comment 40) thanks to the reappearance of the blurry-page problem (comment 11 and following) as soon as I hit the top/bottom overscroll, let alone any history-swipe.

Personally, I think the UX on Retina screens is sufficiently disturbing that we should not consider shipping this in its current form; we really need to come up with a solution that avoids the use of low-res screenshots before letting this feature go to release channels.

Comment 46

[Stephen A Pohl [:spohl] (OOO until 11/7)]

Hmm, based on the number of complaints about the low-res screenshots we might want to land bug 836430 and simply consider this part of the feature. We can keep trying to improve performance and memory consumption as a whole, but hi-res screenshots on retina would simply be a necessary part of the feature.

Comment 47

[Markus Stange [:mstange]]

I wholeheartedly support this idea.

Comment 48

[Cheba]

I've noticed another problem with navigation. Navigating back (via swipe) to an entry that was added to history via pushState results in a blank page or incorrect screenshot. Also it doesn't seem to always trigger page load so I have a picture on my screen but it's not a real page.

How  reproduce:
Go to GitHub. Open any repository and navigate through directory structure of the repository. Try going back and forth through history a few times via swipes.

Nighly 28.0a1 (2013-11-06)

Comment 49

[Stephen A Pohl [:spohl] (OOO until 11/7)]

(In reply to Cheba from comment #48)
> I've noticed another problem with navigation. Navigating back (via swipe) to
> an entry that was added to history via pushState results in a blank page or
> incorrect screenshot.

See bug 881964.

Comment 50

[Stephen A Pohl [:spohl] (OOO until 11/7)]

Attachment: Telemetry

browser-chrome tests were orange in the previous try run. Fixed this by changing TelemetryStopwatch.stop calls to TelemetryStopwatch.finish. I've also moved the declaration of |canvas| out of the first telemetry try block because it would otherwise be undefined in the second try block.

This is now green on try [1]. Sending to Felipe for a final review.

[1] https://tbpl.mozilla.org/?tree=Try&rev=d1c9e1ef286c

Comment 51

[Stephen A Pohl [:spohl] (OOO until 11/7)]

Telemetry patch landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8e832099d04

Leaving [leave open] on white board since this feature remains turned off on the release channel.

Comment 52

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

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

Comment 53

[David Teller [:Yoric] - still alive but not very active]

The telemetry numbers are a bit worrying:
http://telemetry.mozilla.org/#path=nightly/28/FX_GESTURE_TAKE_SNAPSHOT_OF_PAGE

It's 15ms+ (~1 frame) for 50% of samples and it's 56ms+ for 5%. As a reminder, 50ms between two frames is the generally admitted threshold at which people realize that something is janky.

Comment 54

[Stephen A Pohl [:spohl] (OOO until 11/7)]

I'm hopeful that bug 933389 will improve these numbers. David, could you give us a target number that we should aim for?

Comment 55

[David Teller [:Yoric] - still alive but not very active]

Redirecting towards vladan.

Comment 56

[Vladan Djeric (:vladan)]

(In reply to Stephen Pohl [:spohl] from comment #54)
> I'm hopeful that bug 933389 will improve these numbers. David, could you
> give us a target number that we should aim for?

I'm replying here instead of David.. 

In a perfect world, we'd maintain smooth 60fps animations in Firefox on 3-year old Macbooks during snapshotting. But that's probably unrealistic, and we'll need to get a feeling for what looks acceptable, and what kinds of code changes are possible before quantifying goals. I'm waiting for the patch in bug 933389 to be in a testable shape first before doing some experiments myself.

Btw, if I'm understanding correctly, the "pagehide" event will cause us to take a snapshot of the current page + install the snapshot + compress the snapshot all within one JS event? So if we make some hand-wavy assumptions about the 3 FX_GESTURE_* histograms, that would suggest that snapshotting is currently creating a jank of at least 90ms for the slowest 5% of computers, and at least 24ms for the slowest 50% of computers.

Comment 58

[(no longer active)]

Do we have a good pathway to getting to Safari level performance with our current implementation?  It seems to me like we need to rethink the way this should be implemented (I believe Jeff is going to file a bug with the details.)

Until that happens, I think we should turn this off for Nightly and Aurora as we're not really getting much benefit out of testing this.

Comment 59

[Lukas Blakk [:lsblakk] use ?needinfo]

nominate for relnote-firefox when this has at least landed on trunk so we can have it in our sights for a particular release's notes.

Comment 60

[Stephen A Pohl [:spohl] (OOO until 11/7)]

Turned off for now by backing out the "Patch" patch. There was no need to back out the telemetry patch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/070043d0e2e1

Comment 61

[Stephen A Pohl [:spohl] (OOO until 11/7)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #58)
> Do we have a good pathway to getting to Safari level performance with our
> current implementation?

Bug 933389 has the potential to reduce jank to an insignificant amount.

Comment 62

[Ryan VanderMeulen [:RyanVM]]

(In reply to Stephen Pohl [:spohl] from comment #60)
> Turned off for now by backing out the "Patch" patch. There was no need to
> back out the telemetry patch:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/070043d0e2e1

Merged to m-c.
https://hg.mozilla.org/mozilla-central/rev/070043d0e2e1

Comment 63

[Jet Villegas (inactive)]

Is the work on this feature still going on in other bugs?

Comment 64

[Stephen A Pohl [:spohl] (OOO until 11/7)]

The list of dependent bugs gives an idea of what still needs to be done, but I think the main bug that needs fixing is bug 933389. Once that bug is fixed, we should reevaluate the performance of the feature and see if it's sufficient to enable the feature by default. Most recently, I've been sidetracked by bug 1046306 and dependent bugs, but I'm hoping to come back to this eventually.

Comment 65

[Stephen A Pohl [:spohl] (OOO until 11/7)]

Briefly spoke with mstange at the work week and he brought up the point that apzc will likely require a different implementation for history swipe animations. I'm unassigning myself from this bug until we have a clearer picture of what needs to be done.

Comment 66

[Kartikaya Gupta (email:[e-mail])]

Oh man! It would be great if we could come up with a unified gesture for history-swipe on OS X and edge-gesture app swiping on B2G. The way the edge gestures are done in B2G is giving me fits.

Comment 67

[David Teller [:Yoric] - still alive but not very active]

It is my understanding that the B2G swiping is currently being redesigned to match the OS X behavior.

Comment 68

[David Teller [:Yoric] - still alive but not very active]

Shouldn't we also wait for bug 744100/https://wiki.mozilla.org/User:Roc/ScreenCaptureAPI ?

Comment 69

[Markus Stange [:mstange]]

I have a proof of concept for APZ-based swiping in bug 1016035.
I'd like to swipe the whole browser element; we'll need the screen capture API (or make do with drawWindow) for the previous / next page snapshots though.

Comment 70

[Jim Mathies [:jimm]]

Is this something the gfx team will eventually tackle?

Comment 71

[Markus Stange [:mstange]]

It's been on my list of things to tackle "next" for about a year now...

Comment 72

[Kartikaya Gupta (email:[e-mail])]

I guess that means "yes, eventually" :)

Comment 73

[nandhp]

I gather this animation is not enabled due to performance and memory constraints involved in keeping snapshots of all the webpages in the browsing history. However, in the mean time, the history swipe gesture can be confusing as there is no feedback at all. Are there any plans to implement some other kind of feedback as an intermediate step?

In Chrome, there is a little tab, with an arrow on it, that slides in from the side of the window (see http://www.addictivetips.com/mac-os/disable-two-finger-horizontal-swipe-gesture-for-chrome-or-any-os-x-app/). Previous versions displayed a small rectangular overlay that slid across the screen as the gesture was performed (seen in https://www.youtube.com/watch?v=EuvVC6FS9II). Both of these animations give the user important feedback that something is happening, offering an opportunity to cancel. These animations also have little overhead -- I have implemented a similar effect in my Scrolling Gestures extension, which I wrote to implement a similar gesture on non-macOS systems.

It seems like there should be some form of feedback, even if it can't be quite as glossy as Safari for the time being. I expect the lack of feedback leads to many accidental activations, which can be especially confusing for new Mac users.

Comment 74

[Stefan Plewako [:stef]]

History swipe animations don't even work anymore for me - don't know if this is somehow reported or tracked.

Comment 75

[Pierre-Yves Gérardy]

What is the preference?

If it is `apz.overscroll.enabled`, it is buggy for overscroll bouncing:


Rather than translating the page further then coming back, the edge sticks at the window's edge and the page is stretched. Also, it doesn't bounce back automatically. It stays stretched until the page is scrolled back manually.

http://i.imgur.com/1G8bO9t.png

I didn't try history swipes because I don't know exactly what it is...

Comment 76

[Botond Ballo [:botond]]

(In reply to Stefan Plewako [:stef] from comment #74)
> History swipe animations don't even work anymore for me - don't know if this
> is somehow reported or tracked.

Yes, this bug (which is still open) tracks enabling them.

Comment 77

[Botond Ballo [:botond]]

(In reply to Pierre-Yves Gérardy from comment #75)
> What is the preference?

What is the preference for what?

> If it is `apz.overscroll.enabled`, it is buggy for overscroll bouncing:

'apz.overscroll.enabled' is not (currently) a supported pref. It was used for the overscroll effect in Firefox OS, and it will eventually be used for an overscroll effect on macOS (tracked by bug 1124108), but it's not currently supported on any platform.

Comment 78

[Pierre-Yves Gérardy]

Thanks for the details, Botond :-)

Comment 79

[eqtrian+bugzilla]

The lack of some kind of history swipe animation in Firefox for OS X is really annoying coming from Chrome or Safari.  Hopefully this can be accomplished at some point as it really drags down the user experience on a Mac.  It's impossible to tell if the gesture wasn't recognized or if the previous page in history is just taking a while to load.

Comment 81

[Thomas]

(In reply to eqtrian+bugzilla from comment #79)
> The lack of some kind of history swipe animation in Firefox for OS X is
> really annoying coming from Chrome or Safari.

Agreed, this functionality is very badly needed.

May I suggest we implement the simpler approach found in Chrome and Chromium for now, in order to finally provide this functionality? The more complicated Safari approach could always be added at some point in the future.

Comment 82

[Stephen A Pohl [:spohl] (OOO until 11/7)]

(In reply to Thomas from comment #81)
> May I suggest we implement the simpler approach found in Chrome and Chromium
> for now, in order to finally provide this functionality? The more
> complicated Safari approach could always be added at some point in the
> future.

This is the current plan.

Comment 83

[Stephen A Pohl [:spohl] (OOO until 11/7)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d18006b86017b73b24757a5182b1e16fffcc7115

Comment 84

[Mark]

Nice! 

IMHO 
- the semicircle & arrow could be larger (like Chrome) 
- the arrow font should match the Quantum style forward/back arrows in the toolbar.

Comment 85

[Stephen A Pohl [:spohl] (OOO until 11/7)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=92faf75868991417dabbb076de5288c5b74b2a02

Comment 86

[Stephen A Pohl [:spohl] (OOO until 11/7)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e281015774f722584908fde41ec44e81a7c4dadf

Comment 87

[Stephen A Pohl [:spohl] (OOO until 11/7)]

(In reply to Mark from comment #84)
> Nice! 
> 
> IMHO 
> - the semicircle & arrow could be larger (like Chrome) 
> - the arrow font should match the Quantum style forward/back arrows in the
> toolbar.

Thanks for the feedback. You will be happy to hear that the arrows are just placeholders that I whipped up in 2 minutes to test this approach. :-) I'll be requesting proper UX feedback and graphics as soon as try is green.

Comment 88

[Stephen A Pohl [:spohl] (OOO until 11/7)]

Attachment: Swipe back screenshot

Comment 89

[Stephen A Pohl [:spohl] (OOO until 11/7)]

Attachment: Swipe forward screenshot

Comment 90

[Stephen A Pohl [:spohl] (OOO until 11/7)]

Attachment: Left arrow (placeholder)

Comment 91

[Stephen A Pohl [:spohl] (OOO until 11/7)]

Attachment: Right arrow (placeholder)

Comment 92

[Stephen A Pohl [:spohl] (OOO until 11/7)]

The tl;dr of the planned changes here is that we want to provide some kind of visual indicators when a user performs a history swipe to avoid accidental history navigation. The ultimate goal is to mimic the sliding pages from Safari. In the meantime, the idea is to display arrows that become more and more opaque as the user swipes. An arrow should be fully opaque if the swipe would result in a history navigation.

Bram, would you be able to review this approach from a UX perspective and provide the graphics for these arrows? The attached screenshots show the placeholder arrows in action until we have final UI. Arrows are white on top of grey semicircle, with a transparent background (bugzilla makes the background appear white). Thank you!

Comment 93

[Stephen A Pohl [:spohl] (OOO until 11/7)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1b9e2a24c41594af49c1ff77df6e2d19f3c9a6d

Comment 94

[Stephen A Pohl [:spohl] (OOO until 11/7)]

Attachment: Patch

This patch trims a lot of code related to snapshots and replaces it with arrow indicators. It didn't seem to make sense to keep this code around because we were facing a number of performance issues and the code wasn't compatible with the new arrow indicators. Also, we will want to explore mechanisms to take snapshots without the use of drawWindow, so we would be rewriting a lot of this code anyway.

Once this lands I will go through the list of dependent bugs and prune them of bugs that are no longer applicable due to the removal of snapshots.

There was also the suggestion to keep these arrow indicators behind a pref to allow users to turn them off (not yet implemented in this patch). I'm wondering if it would make sense to disable history swipes entirely with this pref, rather than just the arrow indicators since the problem of accidental history navigation would still exist. Thoughts?

Comment 95

[Stephen A Pohl [:spohl] (OOO until 11/7)]

Attachment: Patch

Comment 96

[Stephen A Pohl [:spohl] (OOO until 11/7)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5271744c2267faea19d50396b1a07df6223f45b2

Comment 97

[Bram Pitoyo [:bram]]

(In reply to Stephen A Pohl [:spohl] from comment #92)

> The tl;dr of the planned changes here is that we want to provide some kind
> of visual indicators when a user performs a history swipe to avoid
> accidental history navigation. The ultimate goal is to mimic the sliding
> pages from Safari.

This is a great idea!


> In the meantime, the idea is to display arrows that
> become more and more opaque as the user swipes. An arrow should be fully
> opaque if the swipe would result in a history navigation.

This is also a workable solution. The challenge, I think, will be in making the transparent → opaque transition fades as smoothly as you swipe your trackpad.

Will we be able to test this feature in a Nightly build soon?


> Bram, would you be able to review this approach from a UX perspective and
> provide the graphics for these arrows?

We’ve got a back-arrow and forward-arrow icons ready to use for just this purpose:

https://design.firefox.com/icons/viewer/#back
https://design.firefox.com/icons/viewer/#forward

What about background colour? If we want a dark grey, then grey-60 or grey-70 is a good choice.

Comment 98

[Bram Pitoyo [:bram]]

Attachment: Swipe back & forward screenshot

Comment 99

[Bram Pitoyo [:bram]]

Amy Lee had done some work on our dark theme, so I’m paging her for additional review. She’d be able to advise on whether the visual design choice made on comment 97 & comment 98 is the right one.

Comment 100

[Bram Pitoyo [:bram]]

Attachment: [e-mail]

Left arrow with grey-60 background.

Comment 101

[Bram Pitoyo [:bram]]

Attachment: [e-mail]

Right arrow with grey-60 background.

Comment 102

[Stephen A Pohl [:spohl] (OOO until 11/7)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dec21cbefc48171906884acf419ada1bbb0b453f

Comment 103

[Amy Lee [:amylee] UX]

     (In reply to Bram Pitoyo [:bram] from comment #99)
> Amy Lee had done some work on our dark theme, so I’m paging her for
> additional review. She’d be able to advise on whether the visual design
> choice made on comment 97 & comment 98 is the right one.

Can you provide me a video of how this works (specifically changing from semi transparent to opaque)? Also will these buttons change based on the browser theme? (i.e Default, Dark, Light theme)


My recommendation:

Background: Grey 70
Arrow: Grey 10 80% Opacity

Hover State Background Overlay - Grey 10, 10% Opacity
Pressed State Background Overlay - Grey 10, 20% Opacity

Comment 104

[Stephen A Pohl [:spohl] (OOO until 11/7)]

Attachment: Patch

Updated with new arrows.

Comment 105

[Stephen A Pohl [:spohl] (OOO until 11/7)]

(In reply to Bram Pitoyo [:bram] from comment #97)
> (In reply to Stephen A Pohl [:spohl] from comment #92)
> > In the meantime, the idea is to display arrows that
> > become more and more opaque as the user swipes. An arrow should be fully
> > opaque if the swipe would result in a history navigation.
> 
> This is also a workable solution. The challenge, I think, will be in making
> the transparent → opaque transition fades as smoothly as you swipe your
> trackpad.
> 
> Will we be able to test this feature in a Nightly build soon?

I just built the current patch (with the new arrows) on try. If you'd like, you could take the build for a spin:

https://queue.taskcluster.net/v1/task/auMDFKvEQ8KlItgLvYMoXw/runs/0/artifacts/public/build/target.dmg


(In reply to Amy Lee [:amylee] UX from comment #103)
>      (In reply to Bram Pitoyo [:bram] from comment #99)
> > Amy Lee had done some work on our dark theme, so I’m paging her for
> > additional review. She’d be able to advise on whether the visual design
> > choice made on comment 97 & comment 98 is the right one.
> 
> Can you provide me a video of how this works (specifically changing from
> semi transparent to opaque)?

Do you have a Mac? If so, you could take the build mentioned above for a spin. It might be easiest to get a feel for the feature by trying it yourself (a video wouldn't show how much the fingers swipe across the trackpad).

> Also will these buttons change based on the
> browser theme? (i.e Default, Dark, Light theme)

This wasn't intended. If we decide to do this, I would suggest that we do this in a followup to provide some kind of indicators in the meantime.

> My recommendation:
> 
> Background: Grey 70
> Arrow: Grey 10 80% Opacity
> 
> Hover State Background Overlay - Grey 10, 10% Opacity
> Pressed State Background Overlay - Grey 10, 20% Opacity

Just to be clear: These aren't buttons. These are just indicators that become more and more opaque as the user swipes across the trackpad (or magic mouse). They are fully opaque when a swipe would result in a history navigation.

Comment 106

[:Felipe Gomes (needinfo for replies!)]

Hey there! This looks nice! I haven't fully reviewed it yet, but here are a couple of comments by looking on the patch and testing the try build:

- The main thing that stands out to me is that there's a significant delay from the moment that I release my fingers (touchup) to when the navigation occurs, and that was not the case without this patch.

Just by looking at the code I couldn't tell where this was coming from, but I imagine it's waiting to decide if a swipe is still going on or not.

- It looks like that this code wouldn't need anymore to keep any track of history or do the navigation by itself. Why not make it just about displaying the arrows, and let the previous code handle the actual navigation through browser.gesture.swipe.left etc?

If I understand it correctly, the code is still doing navigation by itself by calling navigateToHistoryIndex(). It looks like a lot more code could be deleted and simplified.

- We're avoiding including new .png files, and are favoring every image to be defined as SVG. So before this lands we should make the arrows to be SVG files (and perhaps it can be only one file, but rotated?)

Comment 107

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8974736 [details] [diff] [review]
Patch

(removing review request to await for the answers on the previous comment)

Comment 108

[Amy Lee [:amylee] UX]

(In reply to Stephen A Pohl [:spohl] from comment #105)
> (In reply to Bram Pitoyo [:bram] from comment #97)
> > (In reply to Stephen A Pohl [:spohl] from comment #92)
> > > In the meantime, the idea is to display arrows that
> > > become more and more opaque as the user swipes. An arrow should be fully
> > > opaque if the swipe would result in a history navigation.
> > 
> > This is also a workable solution. The challenge, I think, will be in making
> > the transparent → opaque transition fades as smoothly as you swipe your
> > trackpad.
> > 
> > Will we be able to test this feature in a Nightly build soon?
> 
> I just built the current patch (with the new arrows) on try. If you'd like,
> you could take the build for a spin:
> 
> https://queue.taskcluster.net/v1/task/auMDFKvEQ8KlItgLvYMoXw/runs/0/
> artifacts/public/build/target.dmg
> 
> 
> (In reply to Amy Lee [:amylee] UX from comment #103)
> >      (In reply to Bram Pitoyo [:bram] from comment #99)
> > > Amy Lee had done some work on our dark theme, so I’m paging her for
> > > additional review. She’d be able to advise on whether the visual design
> > > choice made on comment 97 & comment 98 is the right one.
> > 
> > Can you provide me a video of how this works (specifically changing from
> > semi transparent to opaque)?
> 
> Do you have a Mac? If so, you could take the build mentioned above for a
> spin. It might be easiest to get a feel for the feature by trying it
> yourself (a video wouldn't show how much the fingers swipe across the
> trackpad).
> 
> > Also will these buttons change based on the
> > browser theme? (i.e Default, Dark, Light theme)
> 
> This wasn't intended. If we decide to do this, I would suggest that we do
> this in a followup to provide some kind of indicators in the meantime.
> 
> > My recommendation:
> > 
> > Background: Grey 70
> > Arrow: Grey 10 80% Opacity
> > 
> > Hover State Background Overlay - Grey 10, 10% Opacity
> > Pressed State Background Overlay - Grey 10, 20% Opacity
> 
> Just to be clear: These aren't buttons. These are just indicators that
> become more and more opaque as the user swipes across the trackpad (or magic
> mouse). They are fully opaque when a swipe would result in a history
> navigation.

My Feedback:

- Can there be a soft fade-out when the arrows disappear? Right now it looks a bit abrupt.
- Can you check to see if the arrows are horizontally centered to the half circle background? It looks a bit off.

Thanks

Comment 109

[Bram Pitoyo [:bram]]

Thanks for your feedback, Stephen.

I agree with Felipe’s comment about the delay. This feature should move inline with how far you’ve swiped, and respond as soon as you perform the swipe.

Felipe also wrote:
> - We're avoiding including new .png files, and are favoring every image to
> be defined as SVG. So before this lands we should make the arrows to be SVG
> files (and perhaps it can be only one file, but rotated?)

I agree. The arrows used for these indicators were the same as our toolbar Back and Forward button.

Can we use the same asset, in the value that Amy had recommended?
> Background: Grey 70
> Arrow: Grey 10 80% Opacity

> - Can there be a soft fade-out when the arrows disappear? Right now it looks
> a bit abrupt.

Having this fade-out would be good, as well.

If possible, the fade-out effect and duration should mirror the one found when our hamburger menu appear and disappear.

Otherwise, I recommend having the effect as recommended by our design system:

> duration: between 150ms–250ms (let’s try 200ms)
> transition timing function: cubic-bezier(.07,.95,0,1)

As noted here: https://design.firefox.com/photon/motion/duration-and-easing.html

Demo: https://design.firefox.com/photon/motion/duration-and-easing.html

This demo should have the arrow horizontally centered to the background, as well.

Comment 110

[Bram Pitoyo [:bram]]

Comment on attachment 8974611 [details]
[e-mail]

No longer required because we’re changing to SVG assets.

Comment 111

[Stephen A Pohl [:spohl] (OOO until 11/7)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=37694ef7831e71fd9cfd18cc33b45943d44ab035

Comment 112

[Stephen A Pohl [:spohl] (OOO until 11/7)]

Attachment: Patch

(In reply to :Felipe Gomes (needinfo me!) from comment #106)
> Hey there! This looks nice! I haven't fully reviewed it yet, but here are a
> couple of comments by looking on the patch and testing the try build:
> 
> - The main thing that stands out to me is that there's a significant delay
> from the moment that I release my fingers (touchup) to when the navigation
> occurs, and that was not the case without this patch.

Fixed.

> Just by looking at the code I couldn't tell where this was coming from, but
> I imagine it's waiting to decide if a swipe is still going on or not.
> 
> - It looks like that this code wouldn't need anymore to keep any track of
> history or do the navigation by itself. Why not make it just about
> displaying the arrows, and let the previous code handle the actual
> navigation through browser.gesture.swipe.left etc?

This was for fast swiping using snapshots, i.e. when we didn't want to remove the snapshots and instead swipe the next page snapshot on top of the previous snapshot. We don't need this with arrow indicators and I've removed it.

> If I understand it correctly, the code is still doing navigation by itself
> by calling navigateToHistoryIndex(). It looks like a lot more code could be
> deleted and simplified.

This was for fast swiping as well. I've removed this.
 
> - We're avoiding including new .png files, and are favoring every image to
> be defined as SVG. So before this lands we should make the arrows to be SVG
> files (and perhaps it can be only one file, but rotated?)

Switched to SVG and rotate transform. Could you review if this is the proper path for this resource? Or is there a better place?

Comment 113

[Stephen A Pohl [:spohl] (OOO until 11/7)]

(In reply to Bram Pitoyo [:bram] from comment #109)
> 
> > - Can there be a soft fade-out when the arrows disappear? Right now it looks
> > a bit abrupt.
> 
> Having this fade-out would be good, as well.
> 
> If possible, the fade-out effect and duration should mirror the one found
> when our hamburger menu appear and disappear.

I have added this fade-out as well.

Comment 114

[Stephen A Pohl [:spohl] (OOO until 11/7)]

One thing that I've noticed in my local builds (and waiting to confirm with the try build in comment 111) is that when content is loading behind the arrow indicators, the arrow indicators flicker when they change opaqueness. It would be great if there was a way to avoid this.

Comment 115

[:Felipe Gomes (needinfo for replies!)]

(In reply to Stephen A Pohl [:spohl] from comment #112)
> > - The main thing that stands out to me is that there's a significant delay
> > from the moment that I release my fingers (touchup) to when the navigation
> > occurs, and that was not the case without this patch.
> 
> Fixed.

Great! Can you provide a new try build for us to test?


I haven't fully gone through the patch yet, but it's a nice clean-up. With this big clean-up, I think it's much easier to add a new pref that controls whether these arrows are shown (while still not having to disable the gestures). So might be worth doing it.

(In reply to Stephen A Pohl [:spohl] from comment #114)
> One thing that I've noticed in my local builds (and waiting to confirm with
> the try build in comment 111) is that when content is loading behind the
> arrow indicators, the arrow indicators flicker when they change opaqueness.
> It would be great if there was a way to avoid this.

Weird.. Do you think this is a graphics bug?

Comment 116

[Stephen A Pohl [:spohl] (OOO until 11/7)]

(In reply to :Felipe Gomes (needinfo me!) from comment #115)
> (In reply to Stephen A Pohl [:spohl] from comment #112)
> > > - The main thing that stands out to me is that there's a significant delay
> > > from the moment that I release my fingers (touchup) to when the navigation
> > > occurs, and that was not the case without this patch.
> > 
> > Fixed.
> 
> Great! Can you provide a new try build for us to test?

The try run is in comment 111. The direct link to the build is:
https://queue.taskcluster.net/v1/task/E_e-D819QSWgkCl5O0EKGA/runs/0/artifacts/public/build/target.dmg

However, there are a lot of try failures. Investigating.

Comment 117

[Mark]

The Try in comment 116 seems to have broken two finger scrolling on my Mac (can't drag two fingers up or down trackpad to scroll the page). The two finger swipe left & right works a treat though :)

Comment 118

[:Felipe Gomes (needinfo for replies!)]

Yeah, the delay is gone from this build, but it looks like the boxes are covering the entire content area, because I don't see the mouse pointer changing to the hand when I hover over links, for example.

They should probably be invisible (display:none or visibility:collapse) until they are being used, and even then, have pointer-events: none in them so that they are transparent for the mouse hit testing

Comment 119

[Stephen A Pohl [:spohl] (OOO until 11/7)]

Attachment: Left arrow (SVG)

Hi Bram, I put the arrow from your codepen example into an SVG. Would you mind taking a look and making sure that this is what we want to display? We rotate this arrow 180deg for the right arrow. Thank you!

Comment 120

[Stephen A Pohl [:spohl] (OOO until 11/7)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c28c5a40549f963c3a082712579fe26694ffb255

Comment 121

[Stephen A Pohl [:spohl] (OOO until 11/7)]

Attachment: Patch

(In reply to Mark from comment #117)
> The Try in comment 116 seems to have broken two finger scrolling on my Mac
> (can't drag two fingers up or down trackpad to scroll the page). The two
> finger swipe left & right works a treat though :)

Thanks for reporting! This should be fixed in the try build in comment 120.

(In reply to :Felipe Gomes (needinfo me!) from comment #118)
> Yeah, the delay is gone from this build, but it looks like the boxes are
> covering the entire content area, because I don't see the mouse pointer
> changing to the hand when I hover over links, for example.

This should be fixed in this patch.

> They should probably be invisible (display:none or visibility:collapse)
> until they are being used, and even then, have pointer-events: none in them
> so that they are transparent for the mouse hit testing

Done.

(In reply to :Felipe Gomes (needinfo me!) from comment #115)
> (In reply to Stephen A Pohl [:spohl] from comment #112)
> I haven't fully gone through the patch yet, but it's a nice clean-up. With
> this big clean-up, I think it's much easier to add a new pref that controls
> whether these arrows are shown (while still not having to disable the
> gestures). So might be worth doing it.

I'll go ahead and do this in a separate patch.

> (In reply to Stephen A Pohl [:spohl] from comment #114)
> > One thing that I've noticed in my local builds (and waiting to confirm with
> > the try build in comment 111) is that when content is loading behind the
> > arrow indicators, the arrow indicators flicker when they change opaqueness.
> > It would be great if there was a way to avoid this.
> 
> Weird.. Do you think this is a graphics bug?

I've been able to fix this by improving the way I set the fade out transition. I have not observed any flickering anymore. Should be able to confirm with the try build in comment 120 once it completes.

Comment 122

[Bram Pitoyo [:bram]]

Comment on attachment 8975717 [details]
Left arrow (SVG)

Looks good to me.

Comment 123

[Stephen A Pohl [:spohl] (OOO until 11/7)]

New try build with the latest patch:

https://queue.taskcluster.net/v1/task/ZyhbtprvQjG4q4AALCWJQg/runs/0/artifacts/public/build/target.dmg

Comment 124

[Stephen A Pohl [:spohl] (OOO until 11/7)]

Attachment: Patch

This adds a missing semicolon discovered by ES lint.

Comment 125

[Stephen A Pohl [:spohl] (OOO until 11/7)]

Attachment: Add pref to disable history swipe animations

This patch removes the old snapshots pref and will start reading browser.history_swipe_animation.disable to check if history swipe animations were disabled. I purposely did not make the pref show by default in about:config as to not clutter it.

Comment 126

[Stephen A Pohl [:spohl] (OOO until 11/7)]

Attachment: Test

This removes tests related to the snapshots swipe animations and adds a minimal test for the new swipe arrows.

Comment 127

[Stephen A Pohl [:spohl] (OOO until 11/7)]

The try run in comment 120 is showing a test failure in browser_panelUINotifications_fullscreen_noAutoHideToolbar.js that I'm investigating.

Comment 128

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8976007 [details] [diff] [review]
Patch

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

Hi there! One final thing.. and sorry for not noticing this before.

I imagine that the changes in browser.xul were because now the arrows don't need to be added per-browser, but rather only once.. right?

Even then, I'd prefer if we didn't change the structure in browser.xul, and just re-use the existing stack like the previous code did. I find that the XUL structure is usually sensitive to changes (perf and other regressions), and it's not always possible to easily find all the code related to it (e.g. there might be CSS selectors that don't mention some elements directly but might be affected by a structure change).

I already look at everything else and it looks good otherwise!

Comment 129

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8976009 [details] [diff] [review]
Add pref to disable history swipe animations

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

I'd be ok with adding this in firefox.js for discoverability

Comment 130

[Stephen A Pohl [:spohl] (OOO until 11/7)]

Attachment: Add pref to disable history swipe animations

(In reply to :Felipe Gomes (needinfo me!) from comment #129)
> Comment on attachment 8976009 [details] [diff] [review]
> Add pref to disable history swipe animations
> 
> Review of attachment 8976009 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd be ok with adding this in firefox.js for discoverability

Added to firefox.js. Carrying over r+.

Comment 131

[Stephen A Pohl [:spohl] (OOO until 11/7)]

Attachment: Patch

(In reply to :Felipe Gomes (needinfo me!) from comment #128)
> Comment on attachment 8976007 [details] [diff] [review]
> Patch
> 
> Review of attachment 8976007 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi there! One final thing.. and sorry for not noticing this before.
> 
> I imagine that the changes in browser.xul were because now the arrows don't
> need to be added per-browser, but rather only once.. right?
> 
> Even then, I'd prefer if we didn't change the structure in browser.xul, and
> just re-use the existing stack like the previous code did. I find that the
> XUL structure is usually sensitive to changes (perf and other regressions),
> and it's not always possible to easily find all the code related to it (e.g.
> there might be CSS selectors that don't mention some elements directly but
> might be affected by a structure change).

Thanks for your reviews! As discussed via IRC, this change appeared to be necessary to allow for the fade-out of the arrows when the swipe completed. However, the most recent changes to the way we initiate the fade-out no longer requires this. Reverting browser.xul to its original form also appears to fix the test failure mentioned in comment 127. I'm going to send all patches to try shortly to confirm.

Comment 132

[Stephen A Pohl [:spohl] (OOO until 11/7)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf07e8d9f38d19b28391c715d5a91e7798801087

Comment 133

[Stephen A Pohl [:spohl] (OOO until 11/7)]

Attachment: Patch

Just noticed a 1px error in the included SVG. Fixed in this patch.

Comment 134

[Stephen A Pohl [:spohl] (OOO until 11/7)]

Attachment: Left arrow (SVG)

Fixed SVG. Carrying over r+.

Comment 135

[Stephen A Pohl [:spohl] (OOO until 11/7)]

Attachment: Test

Forgot to `hg add` the new browser.ini file for the new test, which leads to build failures. Carrying over r+. New try run coming up.

Comment 136

[Stephen A Pohl [:spohl] (OOO until 11/7)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3656cb76112a124cd9cd8ea4d2fcb33dbc449447

Comment 137

[Stephen A Pohl [:spohl] (OOO until 11/7)]

Attachment: Test

Added .eslintrc.js file to fix ESLint errors on try. Carrying over r+.

Comment 138

[Stephen A Pohl [:spohl] (OOO until 11/7)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a43d47a768cbfc844eb9fb20c51bff9f840c69c
Bug 860493: Add arrow indicators for history swipes on macOS. r=felipe,bram

https://hg.mozilla.org/integration/mozilla-inbound/rev/59cc6b99727e58e3b214266c45089d945999c285
Bug 860493: Add pref to disable history swipe animations. r=felipe

https://hg.mozilla.org/integration/mozilla-inbound/rev/c95d5c9994e8f625a9c03ee7aea9dade1ea1c42f
Bug 860493: Add test for history swipe arrows. r=felipe

Comment 139

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/5a43d47a768c
https://hg.mozilla.org/mozilla-central/rev/59cc6b99727e
https://hg.mozilla.org/mozilla-central/rev/c95d5c9994e8

Comment 140

[Mihai Boldan, Desktop QA [:mboldan]]

I managed to verify the issue on Firefox 62.0b11, under macOS 10.13.3. The animations are correctly displayed.
I only noticed that if the touch-pad is releases before the animation is entirely displayed and perform the gestures in order to go to previous/next page right away, the animation is not displayed, but the page is changed. 
Note that if you wait 1 or 2 seconds before trying to change the page, the animation is displayed. 
Stephen, can you please tell me if this is expected?

Comment 141

[Stephen A Pohl [:spohl] (OOO until 11/7)]

(In reply to Mihai Boldan, QA [:mboldan] from comment #140)
> I managed to verify the issue on Firefox 62.0b11, under macOS 10.13.3. The
> animations are correctly displayed.
> I only noticed that if the touch-pad is releases before the animation is
> entirely displayed and perform the gestures in order to go to previous/next
> page right away, the animation is not displayed, but the page is changed. 
> Note that if you wait 1 or 2 seconds before trying to change the page, the
> animation is displayed. 
> Stephen, can you please tell me if this is expected?

Yes, this is bug 1463235. Thanks for verifying!

Comment 142

[Mihai Boldan, Desktop QA [:mboldan]]

Great! Thanks for the reply! Since there is another bug logged for the issue described in comment 140, I'm marking this issue Verified Fixed.