935258 - Swipe animations break gmail

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Not sure if they are using History.pushState (in which case this might be a dupe of bug 881964), or if it's just plain old fragment navigations.

Either way swipe animations break gmail.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

What version of OS X are you using?  If it's Mavericks (10.9), this might be bug 927702.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I am using maveriks, but I don't think it's bug 927702. The actual swiping renders fine, it's just that it swipes back to the wrong place. Or rather, it shouldn't swipe at all since the user remains on the same page when going 'back' on gmail.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Jonas, please give a very detailed description of the problem you see, and how to reproduce it.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Steps to reproduce:

1. Open gmail and log in. You should now see the contents of your inbox.
2. Click an email. You should now see the contents of an email.
3. Use touch pad to do a "back" motion.

Expected results:
Should see inbox.
No swiping action should happen since gmail just does a bunch of DOM mutations to change the view between step 1 and step 2. So gmail needs to reverse those DOM mutations to revert back to the inbox.

Actual results:
A swiping animation to a blank screen.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Testing on OS X 10.7.5 with today's mozilla-central nightly, my results are what you describe as correct.  That's not the case with Safari, though -- it allows me to swipe back to the previously displayed page.

Testing on OS X 10.9 with today's mozilla-central nightly, my results are clearly incorrect, but not in the way you describe -- I'm allowed to swipe back to a blank page.  (Safari's behavior stays the same.)

This raises at least two questions:

What is the correct behavior in the case you describe?  Clearly Apple thinks otherwise than you do, but that doesn't mean they're right and you're wrong.

Why does swiping behave differently in this case on OS X 10.7.5 and 10.9.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

> What is the correct behavior in the case you describe?

With the Back button I go back to the previously displayed page (in today's mozilla-central nightly on both OS X 10.9 and 10.7.5).

So (on OS X 10.9) I'm getting different behavior with the back swipe that I do with the Back button -- which sounds wrong.

And so we'd (presumably) have to write code from scratch to make a "go back one page" action take account of the current state of the DOM.  This sounds like a major project -- not just a bug fix.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

> So (on OS X 10.9) I'm getting different behavior with the back swipe
> that I do with the Back button -- which sounds wrong.

Actually the back swipe and the back button behave differently (in
this case) on both OS X 10.9 and OS X 10.7.5.

Comment 8

[Stephen A Pohl [:spohl]]

I may have some time soon to investigate why there's a difference between back button and swiping. We should be able to sync this up. I believe this is a dupe of bug 881964, but I'll investigate before closing as dupe.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Steven: I don't actually care what happens in safari. Before the snapshot feature landed, going "back", through any means available in firefox UI, from step 2 in comment 4 brought you to a rendering of the inbox. That is what I expect as a user.

And, I suspect this is a dupe of either bug 881964 or whatever bug that we're using to track the broken fragment navigation.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #9)

So you don't mind if we behave like Safari, and make the back swipe (and the back button) just go back to the previously displayed screen (which doesn't, of course, require any knowledge of the DOM -- just of the history list).  All you object to, and what this bug is about, is that (on Mavericks only) swipe back to a blank page.

This wasn't at all clear from your previous comments.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

First off, I can reproduce this problem just fine on 10.8.5 on a laptop without retina. So nothing mavericks or retina specific here.

Second, what I care about is that 'back' works. I don't care about reproducing anything safari does. The problems I'm seeing isn't just related to rendering. I.e. it's not just that the right pixels aren't on the screen, I also can't interact with the page at all.

Did you actually try to reproduce the bug? Gmail is very clearly broken. I hope it's obvious that that is not ok.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Resetting to previous summary since this isn't just a problem of the page being blank. It's completely broken.

Comment 13

[Steven Michaud [:smichaud] (Retired)]

> Did you actually try to reproduce the bug?

Of course I did.  See comment #5 and comment #6.

Comment 14

[Stephen A Pohl [:spohl]]

(In reply to Jonas Sicking (:sicking) Please ni? if you want feedback from comment #11)
> Gmail is very clearly broken. I hope it's obvious that that is not ok.

I'm with you. I should have time tonight and/or tomorrow morning to take a good look at this (sorry, I was busier than I had hoped this week). I do want to point out though that swipe animations remain turned off in the release channel no matter when we merge, specifically to fix problems like this one.

Comment 15

[Steven Michaud [:smichaud] (Retired)]

It's possible this bug's STR (from comment #4) "works" on both OS X 10.8 and 10.9.  You definitely don't get a blank page on OS X 10.7.5, though.

Even on OS X 10.7.5, though, you don't go back to the Inbox.  Instead nothing happens at all -- which is arguably also wrong.

Comment 16

[Florian Bender]

So, is this a dupe of Bug 881964 or not?

Comment 17

[José Jeria]

(In reply to Florian Bender from comment #16)
> So, is this a dupe of Bug 881964 or not?

Gmail also uses history.pushState, could be.

Comment 18

[Stephen A Pohl [:spohl]]

Attachment: Patch

This fixes GMail (reported here) and GitHub (reported in bug 881964 comment 8). It does not, however, fix www.dn.se (reported in bug 881964 comment 2). I will keep the bugs separate and make bug 881964 specific to www.dn.se.

The problem was that the event target for "pageshow" events was the window object, rather than the document. There will still be situations when swiping back displays a blank page, but it will now be torn down when the animation stops. Displaying the blank page is actually correct, since we couldn't take a snapshot of that index in history due to the use of pushState. We just need to make sure that we tear down the animation overlay when the animation stops.

Comment 19

[:Felipe Gomes (needinfo for replies!)]

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

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

Personally I think that we shouldn't show the animation and just do the plain old swipe navigation if the page has used pushState.. But I see how that can be confusing to users.

I wonder if there's some way we could snapshot the page on a pushState change, right before the content actually change, (which is the reason we'll be showing the blank snapshot underneath), but that seems difficult. Safari seems able to do it, though. Maybe it's something that bug 933389 will be able to provide.

In any case, this patch fixes gmail navigation and some other websites too, so r+!

Comment 20

[Stephen A Pohl [:spohl]]

Thanks, Felipe! Adding some more background below:

(In reply to :Felipe Gomes from comment #19)
> Personally I think that we shouldn't show the animation and just do the
> plain old swipe navigation if the page has used pushState.. But I see how
> that can be confusing to users.

The user would still have to swipe back twice because of the way GMail uses pushState (and how we handle pushState). You can tell by using the back button instead of swipes: from the GMail inbox, you have to press the back button twice before you get back to the previous webpage.

> I wonder if there's some way we could snapshot the page on a pushState
> change, right before the content actually change, (which is the reason we'll
> be showing the blank snapshot underneath), but that seems difficult. Safari
> seems able to do it, though. Maybe it's something that bug 933389 will be
> able to provide.

The difference seems to be in the way Gecko has pushState implemented vs. how Safari does it. Simply going to www.gmail.com in Firefox adds two new entries to the history array while it only adds one in Safari. Since the second entry is added before the first one could load, there's nothing for us to take a snapshot of. Hence, the blank page when swiping back the first time.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

When I just load gmail neither the back, nor the forward, buttons are enabled. Wouldn't that mean there is only one history entry?

Comment 22

[Stephen A Pohl [:spohl]]

(In reply to Jonas Sicking (:sicking) Please ni? if you want feedback from comment #21)
> When I just load gmail neither the back, nor the forward, buttons are
> enabled. Wouldn't that mean there is only one history entry?

I'm guessing you weren't logged into gmail in this case. If I'm logged in, I only get one history entry. I don't run into a broken gmail behavior (or blank pages) though when I'm not logged in. If I'm logged in I get two entries.

Could you verify that you're seeing the same? The easiest way to do this is probably to open the browser console (Cmd + Shift + J) and type in:

gBrowser.webNavigation.sessionHistory.count

This will give you the number of history entries. Thanks!

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Stephen Pohl [:spohl] from comment #22)
> I'm guessing you weren't logged into gmail in this case. If I'm logged in, I
> only get one history entry. I don't run into a broken gmail behavior (or
> blank pages) though when I'm not logged in. If I'm logged in I get two
> entries.

You're both saying "If I'm logged in, I only get one history entry" and "If I'm logged in I get two entries". I'm guessing the latter is what you mean and that the former is missing a "not"?

I'm referring to when I'm logged in. I.e. all of the below is with a logged in account.

It appears to depend on which URL you load.

If I load "https://mail.google.com/mail/u/0/?shva=1#inbox" then I only get one entry.

If I load "https://mail.google.com" I see a bunch of redirects and then indeed I get two entries. The last one is "https://mail.google.com/mail/u/0/#inbox". Clicking 'back' to go to the previous entry takes me to "https://mail.google.com/mail/u/0/"

> gBrowser.webNavigation.sessionHistory.count

This confirms that in the former case I only get one entry. In the latter I get two.

However note that with swiping disabled, in no case can i click 'back' or 'forward' and end up on a blank page. The new swiping feature shouldn't result in the user ending up on a blank page either. I.e. it's not ok for new features to "break the web".

I guess it would be ok if we during the swiping render a blank page, however that blank page should immediately be re-rendered as a proper page once the user commits the swipe.

Comment 24

[Stephen A Pohl [:spohl]]

(In reply to Jonas Sicking (:sicking) Please ni? if you want feedback from comment #23)
> You're both saying "If I'm logged in, I only get one history entry" and "If
> I'm logged in I get two entries". I'm guessing the latter is what you mean
> and that the former is missing a "not"?

Err, it's getting late here. Yes, your guess is correct. :-)

> It appears to depend on which URL you load.

I wasn't aware of this, but I can reproduce what you've observed. Good to know.
 
> I guess it would be ok if we during the swiping render a blank page, however
> that blank page should immediately be re-rendered as a proper page once the
> user commits the swipe.

100% agreed. The patch here does precisely that.

Comment 25

[Stephen A Pohl [:spohl]]

https://tbpl.mozilla.org/?tree=Try&rev=4fb28e459665

Comment 26

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f25c7ee9d26

Comment 27

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

https://hg.mozilla.org/mozilla-central/rev/3f25c7ee9d26

Comment 28

[Florian Bender]

This is not quite fixed for GitHub, at least: 

0. Nightly 28.0a1 (2013-11-21)
1. Open new tab, go to https://github.com/mozilla/mozilla-central
2. Click on accessible. 
3. After the new view loaded, go back via swipe. 
4. During animation, the new tab page instead of the previous page is shown. 

Shall I file a new bug, or reopen this one?

Comment 29

[Stephen A Pohl [:spohl]]

(In reply to Florian Bender from comment #28)
> This is not quite fixed for GitHub, at least:
> [...]
> Shall I file a new bug, or reopen this one?

What you're seeing is what was discussed in comment 19 and comment 20. It would be good to have another bug for this. With pushState, we don't seem to be getting the equivalent of pagehide/pageshow events (at least as far as I can tell). This bug was mostly concerned about tearing down the animation overlay. Otherwise, the page remained in a state in which it couldn't be interacted with until the page was either refreshed or swiped away a second time. I believe this remains fixed for GitHub too, correct?

Comment 30

[Florian Bender]

Yeah, interaction works, but UX is terrible. Filed Bug 942589 and Bug 942595.