Last Comment Bug 639711 - Selecting "Tabs from other computers" on home screen directs to Preferences
: Selecting "Tabs from other computers" on home screen directs to Preferences
Status: VERIFIED FIXED
: polish, regression, uiwanted
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: ARM Android
: P2 normal (vote)
: Firefox 6
Assigned To: Matt Brubeck (:mbrubeck)
:
:
Mentors:
: 642575 (view as bug list)
Depends on: 640971 644424 659777
Blocks: 612607
  Show dependency treegraph
 
Reported: 2011-03-07 18:18 PST by Bret Reckard
Modified: 2011-06-23 13:46 PDT (History)
13 users (show)
mounir: in‑testsuite?
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (744 bytes, patch)
2011-03-18 12:20 PDT, Matt Brubeck (:mbrubeck)
no flags Details | Diff | Splinter Review
patch v2 (1.33 KB, patch)
2011-04-08 07:59 PDT, Matt Brubeck (:mbrubeck)
wjohnston2000: review+
Details | Diff | Splinter Review
patch v3 (3.86 KB, patch)
2011-05-20 17:42 PDT, Matt Brubeck (:mbrubeck)
no flags Details | Diff | Splinter Review
patch v4 (5.65 KB, patch)
2011-05-25 16:24 PDT, Matt Brubeck (:mbrubeck)
wjohnston2000: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Bret Reckard 2011-03-07 18:18:26 PST
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) 2011-03-08 07:33:45 PST
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 [:kbrosnan] 2011-03-14 09:26:28 PDT
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 3 Matt Brubeck (:mbrubeck) 2011-03-17 13:38:31 PDT
*** Bug 642575 has been marked as a duplicate of this bug. ***
Comment 4 Matt Brubeck (:mbrubeck) 2011-03-17 13:41:35 PDT
Nominating for 4.next.
Comment 5 Richard Soderberg [:atoll] 2011-03-17 13:42:55 PDT
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) 2011-03-18 12:20:51 PDT
Created attachment 520275 [details] [diff] [review]
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) 2011-03-18 13:07:24 PDT
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) 2011-03-18 13:09:47 PDT
(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] 2011-03-18 13:16:49 PDT
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] 2011-03-18 13:17:56 PDT
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) 2011-03-18 13:19:50 PDT
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 Richard Soderberg [:atoll] 2011-03-18 13:51:01 PDT
Can the jpake dialog be shown inside a tab in the awesomescreen?
Comment 13 Wesley Johnston (:wesj) 2011-03-18 14:00:55 PDT
> (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?) 2011-03-18 15:05:09 PDT
(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) 2011-03-23 17:39:12 PDT
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) 2011-04-07 22:01:01 PDT
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) 2011-04-08 07:27:55 PDT
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) 2011-04-08 07:59:21 PDT
Created attachment 524628 [details] [diff] [review]
patch v2

Updated based on Wes's comment.
Comment 19 Matt Brubeck (:mbrubeck) 2011-04-08 12:27:45 PDT
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) 2011-05-20 17:42:38 PDT
Created attachment 534170 [details] [diff] [review]
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) 2011-05-25 16:24:58 PDT
Created attachment 535225 [details] [diff] [review]
patch v4

Show the spinner until sync setup/login completes or is aborted.
Comment 22 Matt Brubeck (:mbrubeck) 2011-05-25 16:26:16 PDT
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) 2011-05-27 14:56:35 PDT
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) 2011-05-27 15:21:45 PDT
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) 2011-05-27 15:24:57 PDT
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) 2011-05-30 05:58:10 PDT
Pushed:
http://hg.mozilla.org/mozilla-central/rev/3d845eea88e8
Comment 27 Matt Brubeck (:mbrubeck) 2011-06-01 12:29:42 PDT
Pushed to Aurora for Firefox 6:
http://hg.mozilla.org/releases/mozilla-aurora/rev/90cd0558fc93
Comment 28 Bret Reckard 2011-06-02 12:06:14 PDT
Awesome, thanks for taking my bug seriously! You guys rock!
Comment 29 Naoki Hirata :nhirata (please use needinfo instead of cc) 2011-06-23 13:46:22 PDT
Mozilla/5.0 (Android; Linux armv71; rv6.0a2) Gecko/20110623 Firefox/6.0a2 Fennec/6.0a2
Device: HTC Flyer
OS: Android 2.3

Note You need to log in before you can comment on or make changes to this bug.