639711 - Selecting "Tabs from other computers" on home screen directs to Preferences

Status: VERIFIED FIXED

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

Description

[Bret Reckard]

Shortly after startup:

Selecting "Tabs from other computers" on home screen directs to Preferences menu and not the Sync menu.  If you wait a 5-count, this link does direct properly, but is confusing for the user in it's current form.

The link should at least direct to the Sync menu with a *working* or some other graphic. Not go to the Preferences menu.

This occurs on my Atrix for Beta5 and Fennec

Comment 1

[Matt Brubeck (:mbrubeck)]

This happens if you select this option during the first 5 seconds after Fennec starts, before Sync has connected.  Looks like a regression from bug 612607.

Comment 2

[Kevin Brosnan [Ex-Mozilla]]

This also happens when the user is disconnected from the network and cannot connect to sync. Need a meaningful error message or a cache for the tabs from other computers.

Comment 4

[Matt Brubeck (:mbrubeck)]

Nominating for 4.next.

Comment 5

[:Atoll]

I've seen this as well on Fennec nightly, Droid2, when I clicked "Tabs" before sync finished connecting to the server.

Comment 6

[Matt Brubeck (:mbrubeck)]

Attachment: patch

This fixes the problem for me, though I'm wondering if we should just remove this entire "else if" block.

Comment 7

[Wesley Johnston (:wesj)]

My intent here was to catch the case where the user had set up sync, but had then disabled it via prefs. In that case, they don't need to set up sync again, just click the "enable" button.

With this patch I think we instead show old tabs that we had previously synced (unless logging in to sync has failed). Maybe that's better behavior anyway? I think maybe the ideal solution is to show the awesomescreen, but with a row giving some information about the current state of sync: "setting up", "disabled", etc? Adding madhava for feedback.

Comment 8

[Matt Brubeck (:mbrubeck)]

(In reply to comment #7)
> I think maybe the ideal solution is to show the awesomescreen, but with a row
> giving some information about the current state of sync: "setting up",
> "disabled", etc? Adding madhava for feedback.

I agree.  When sync is disabled or disconnected, I think the "remote tabs" awesomescreen panel should show the current status, with a button to change it ("Connect" or "Enable" depending on the status).

Comment 9

[Madhava Enros [:madhava]]

Yeah, agreed with c7 and c8. Flipping to the prefs panel is too ambiguous and frustrating for the user.

When a user switches to that awesomescreen tab (however they get there), we _could_ show the tabs that were current when the user was last connected, and reserve the first row for a "Sync is disabled" message and ability to turn it back on right there (thinking out loud here -- maybe we don't want to include the older taps?)  If they're simply not connected, we'll need to pop up the jpake dialog instead.

Comment 10

[Madhava Enros [:madhava]]

Re-read c8 a bit -- Matt, do you think we should have a message in the panel rather than popping up the jpake dialog if the user's not connected?

Comment 11

[Matt Brubeck (:mbrubeck)]

Just popping up the JPAKE dialog is probably fine too...  the downside is that it's more disruptive - you can't just switch back to a different awesomescreen panel.  (Maybe instead of hiding the awesomescreen as we do now, we should show the JPAKE dialog over the awesomescreen.)

Comment 12

[:Atoll]

Can the jpake dialog be shown inside a tab in the awesomescreen?

Comment 13

[Wesley Johnston (:wesj)]

> (Maybe instead of hiding the awesomescreen as we do now, we should show
> the JPAKE dialog over the awesomescreen.)

This is Bug 640971

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #12)
> Can the jpake dialog be shown inside a tab in the awesomescreen?

I don't think we want to do that. The list is a list. Popping the dialog up over the list is fine.

Comment 15

[Matt Brubeck (:mbrubeck)]

Filed bug 644424 for the idea of showing sync status or connect / sync-now buttons in the remote tabs panel.

Comment 16

[Matt Brubeck (:mbrubeck)]

Wes, do you think we could take this patch (or something similar) for Firefox 5, to improve the behavior in comment 0 and comment 2?  Then for Fx6 we can work on the approaches discussed in the later comments.

Comment 17

[Wesley Johnston (:wesj)]

I'm not sure exactly what case this is catching. Looks like you are checking if we have the wrong username, and we actually do need to show some sort of error? The check here should look more like the ones here:

http://mxr.mozilla.org/mobile-browser/source/chrome/content/sync.js#435

i.e. if sync is configured and enabled and set to autologin, then it doesn't matter we haven't logged in yet, we will soon... or something like that?

Comment 18

[Matt Brubeck (:mbrubeck)]

Attachment: patch v2

Updated based on Wes's comment.

Comment 19

[Matt Brubeck (:mbrubeck)]

I found a problem with this patch; if we open the remote tabs list while tabs are still empty, then it never refreshes when sync later completes.

Comment 20

[Matt Brubeck (:mbrubeck)]

Attachment: patch v3

This will let the spinner keep spinning until login completes, then it will show the tabs.  There's one problem, though:  If it shows the JPAKE connection screen, and then the user presses cancel, the spinner will keep spinning forever.

Comment 21

[Matt Brubeck (:mbrubeck)]

Attachment: patch v4

Show the spinner until sync setup/login completes or is aborted.

Comment 22

[Matt Brubeck (:mbrubeck)]

The remote tabs list is broken by an unrelated regression (bug 659777), so this patch cannot be properly tested without the fix from that bug.

Comment 23

[Wesley Johnston (:wesj)]

Comment on attachment 535225 [details] [diff] [review]
patch v4

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

Looks good and works great. Thanks Matt!

::: mobile/chrome/content/bindings.xml
@@ +1384,4 @@
>            this.setAttribute("loading", "true");
>  
> +          if (Weave.Service.isLoggedIn) {
> +            setTimeout(function() self._loadChildren());

Hmm. We don't need zero here anymore?

Comment 24

[Matt Brubeck (:mbrubeck)]

Pushed to cedar: http://hg.mozilla.org/projects/cedar/rev/3d845eea88e8

NOTE: This cannot be verified until this patch *and* bug 659777 are both merged to mozilla-central.


(In reply to comment #23)
> > +          if (Weave.Service.isLoggedIn) {
> > +            setTimeout(function() self._loadChildren());
> 
> Hmm. We don't need zero here anymore?

Oops, I left out the 0 by accident.  It actually seems to work fine that way, but I added it on checkin for clarity.

Comment 25

[Matt Brubeck (:mbrubeck)]

Comment on attachment 535225 [details] [diff] [review]
patch v4

Requesting approval-mozilla-aurora to land this for Firefox 6 after it has been in Nightly for a while.  This is a highly visible polish issue (it affects anyone who tries to view desktop tabs immediately after Fennec starts) that has been triaged as blocking-fennec:2.0next+ and then tracking-fennec:6+.

The fix touches mobile files only, so there is no risk for desktop.

Comment 26

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/3d845eea88e8

Comment 27

[Matt Brubeck (:mbrubeck)]

Pushed to Aurora for Firefox 6:
http://hg.mozilla.org/releases/mozilla-aurora/rev/90cd0558fc93

Comment 28

[Bret Reckard]

Awesome, thanks for taking my bug seriously! You guys rock!

Comment 29

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Mozilla/5.0 (Android; Linux armv71; rv6.0a2) Gecko/20110623 Firefox/6.0a2 Fennec/6.0a2
Device: HTC Flyer
OS: Android 2.3