Last Comment Bug 748400 - Filelink add account dialog's "Learn More" looks strange when no more accounts can be added.
: Filelink add account dialog's "Learn More" looks strange when no more account...
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: Thunderbird 15.0
Assigned To: Mike Conley (:mconley) - (Needinfo me!)
:
Mentors:
Depends on:
Blocks: BigFiles
  Show dependency treegraph
 
Reported: 2012-04-24 09:14 PDT by Mike Conley (:mconley) - (Needinfo me!)
Modified: 2012-05-10 08:21 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed
+
fixed


Attachments
Screenshot of the problem (25.52 KB, image/png)
2012-04-24 09:14 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details
Patch v1 (5.58 KB, patch)
2012-05-01 11:54 PDT, Mike Conley (:mconley) - (Needinfo me!)
bwinton: review+
bwinton: ui‑review-
Details | Diff | Splinter Review
WIP Patch (6.17 KB, patch)
2012-05-02 11:36 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
WIP Patch 2 (7.06 KB, patch)
2012-05-02 11:48 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
Patch v2 (8.35 KB, patch)
2012-05-02 12:02 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
Patch v3 (7.30 KB, patch)
2012-05-02 12:47 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
Patch v4 (7.27 KB, patch)
2012-05-02 12:50 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
Patch v5 (7.11 KB, patch)
2012-05-02 12:54 PDT, Mike Conley (:mconley) - (Needinfo me!)
richard.marti: review+
bwinton: review-
bwinton: ui‑review-
Details | Diff | Splinter Review
Patch v6 (9.26 KB, patch)
2012-05-08 08:19 PDT, Mike Conley (:mconley) - (Needinfo me!)
bwinton: review+
bwinton: ui‑review+
Details | Diff | Splinter Review
Patch v7 - now with tests! (35.36 KB, patch)
2012-05-08 13:29 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
Patch v8 (34.70 KB, patch)
2012-05-08 13:31 PDT, Mike Conley (:mconley) - (Needinfo me!)
bwinton: review+
Details | Diff | Splinter Review
Patch v9 (carrying over r+ from bwinton) (34.73 KB, patch)
2012-05-09 13:26 PDT, Mike Conley (:mconley) - (Needinfo me!)
mozilla: approval‑comm‑aurora+
mozilla: approval‑comm‑beta+
Details | Diff | Splinter Review

Description Mike Conley (:mconley) - (Needinfo me!) 2012-04-24 09:14:20 PDT
Created attachment 617895 [details]
Screenshot of the problem

See screenshot.
Comment 1 Mike Conley (:mconley) - (Needinfo me!) 2012-05-01 11:54:27 PDT
Created attachment 620007 [details] [diff] [review]
Patch v1

Ok, so now the "Learn More..." link is beneath and right aligned with the "You can't add more than one account" text.

I also noticed that the "Set up account" button was still enabled in this state.  We now disable when we're in this state.
Comment 2 Blake Winton (:bwinton) (:☕️) 2012-05-02 09:08:38 PDT
Comment on attachment 620007 [details] [diff] [review]
Patch v1

Seems to fix it, although clicking "Learn more" opens up a compose window on Mac.  So, ui-r-.  ;)

(But ui-r=me with that fixed.)

>+++ b/mail/components/cloudfile/content/addAccountDialog.js
>@@ -52,45 +52,49 @@ let addAccountDialog = {
>+    if (!canAddAccounts)
>+      return;
[snip…]
>     // Hook up the default "Learn More..." link to the appropriate link.
>     let learnMore = this._settings
>                         .contentDocument
>                         .querySelector('#learn-more > a[href=""]');
>     if (learnMore)
>-      learnMore.href = Services.prefs
>-                               .getCharPref("mail.cloud_files.learn_more_url");
>+      this._hookUpLearnMore(learnMore);
>+

So, I think we might want to do this even if we can't add accounts, which could explain the previous complaint.  But you do that later, so, no, I guess.

>@@ -143,26 +147,43 @@ let addAccountDialog = {
>+    // This block should go away when bug 748437 gets fixed, since we'll
>+    // be able to add an arbitrary number of accounts for each account type.
>     if (this._accountType.itemCount == 0) {
>+      // Hook up the default "Learn More..." link to the appropriate link.
>+      let learnMore = this._noAccountText
>+                          .querySelector('#learn-more');
>+
>+      if (learnMore) {
>+        this._hookUpLearnMore(learnMore);
>+        learnMore.addEventListener('click', this.onClickLink);

A ha!  That  might be it.  In the general case, we're looking at
    let learnMore = this._settings
                        .contentDocument
                        .querySelector('#learn-more > a[href=""]');
But in this case, we have 
    let learnMore = this._noAccountText
                        .querySelector('#learn-more');

So, I don't know why that's different, but I'm guessing that's the problem.

Apart from that, it seems fine to me, so I'm going to say r=me once that's fixed.

Later,
Blake.
Comment 3 Mike Conley (:mconley) - (Needinfo me!) 2012-05-02 11:36:43 PDT
Created attachment 620399 [details] [diff] [review]
WIP Patch
Comment 4 Mike Conley (:mconley) - (Needinfo me!) 2012-05-02 11:48:18 PDT
Created attachment 620408 [details] [diff] [review]
WIP Patch 2

OSX and Ubuntu are styled - Windows is next...
Comment 5 Mike Conley (:mconley) - (Needinfo me!) 2012-05-02 12:02:36 PDT
Created attachment 620416 [details] [diff] [review]
Patch v2

Ok, found the problem and a solution.

Paenglab: am I using -moz-margin-start / -moz-margin-end correctly here?

-Mike
Comment 6 Mike Conley (:mconley) - (Needinfo me!) 2012-05-02 12:42:06 PDT
Comment on attachment 620416 [details] [diff] [review]
Patch v2

I think I can clean this up some first.
Comment 7 Mike Conley (:mconley) - (Needinfo me!) 2012-05-02 12:47:55 PDT
Created attachment 620430 [details] [diff] [review]
Patch v3
Comment 8 Mike Conley (:mconley) - (Needinfo me!) 2012-05-02 12:50:52 PDT
Created attachment 620432 [details] [diff] [review]
Patch v4

Was disabling an element twice.  Going to give this a once over before requesting review.
Comment 9 Mike Conley (:mconley) - (Needinfo me!) 2012-05-02 12:54:26 PDT
Created attachment 620437 [details] [diff] [review]
Patch v5

Ok, this one is good for reviewing.
Comment 10 Richard Marti (:Paenglab) 2012-05-02 13:14:16 PDT
Comment on attachment 620437 [details] [diff] [review]
Patch v5

The CSS is looking good.
Comment 11 Blake Winton (:bwinton) (:☕️) 2012-05-08 07:34:46 PDT
(Huh, that might have been nicer with a comment…  ;)

The "Set up Account" button doesn't get disabled, so r-, and ui-r-.

Mike's already working on it.)
Comment 12 Mike Conley (:mconley) - (Needinfo me!) 2012-05-08 08:19:54 PDT
Created attachment 621985 [details] [diff] [review]
Patch v6
Comment 13 Mike Conley (:mconley) - (Needinfo me!) 2012-05-08 08:28:59 PDT
Comment on attachment 621985 [details] [diff] [review]
Patch v6

Alright, I feel pretty good about this one.
Comment 14 Blake Winton (:bwinton) (:☕️) 2012-05-08 10:56:17 PDT
Comment on attachment 621985 [details] [diff] [review]
Patch v6

Aside from the problem I saw in http://dl.dropbox.com/u/2301433/Screenshots/OpenALink.png I like it, so ui-r=me with that fixed.

>+++ b/mail/components/cloudfile/content/addAccountDialog.js
>@@ -206,31 +218,36 @@ let addAccountDialog = {
>   onInput: function AAD_onInput() {
>     // Let's see if we have everything we need to make OK enabled...
>-    addAccountDialog._accept.disabled = !addAccountDialog.checkValidity();
>+    if (this._accountType.selectedIndex == -1) {
>+      // We have the "Select a service provider" menuitem selected, so we
>+      // shouldn't be able to click "Set up account"
>+      this._accept.disabled = true;
>+    }
>+    else
>+      this._accept.disabled = !this.checkValidity();

Nit: If there are brackets around the if, we need them around the else, too.

And aside from that, I'm happy with the code, so r=me, too.
Comment 15 Mike Conley (:mconley) - (Needinfo me!) 2012-05-08 13:29:33 PDT
Created attachment 622120 [details] [diff] [review]
Patch v7 - now with tests!

I've added some tests to check on the "add an account" dialog logic, wrt to accept button enabling / disabling.

This was tougher than it sounds - I had to go monkeying around a bit in how we spawn the dialog (if we didn't, we risk permanent orange on our Mozmill testing machines), and had to change bits and pieces of our testing helpers.

I've pushed a version of this patch up to try to make sure I didn't blow anything up:  https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=a590b8cc6226
Comment 16 Mike Conley (:mconley) - (Needinfo me!) 2012-05-08 13:31:55 PDT
Created attachment 622122 [details] [diff] [review]
Patch v8

Some style cruft slipped in, but shouldn't affect the previous try build.

Ready for review.
Comment 17 Mike Conley (:mconley) - (Needinfo me!) 2012-05-09 06:38:14 PDT
Try build finished, and nothing hairy popped up.
Comment 18 Blake Winton (:bwinton) (:☕️) 2012-05-09 12:52:41 PDT
Comment on attachment 622122 [details] [diff] [review]
Patch v8

>+++ b/mail/test/mozmill/cloudfile/test-cloudfile-add-account-dialog.js
>@@ -0,0 +1,215 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+ * You can obtain one at http://mozilla.org/MPL/2.0/. */

Love me some MPL2!

>+let cfh, gOldProviders = {};

As later, do we really need "cfh" here, in we're installing into ourselves?

>+function teardownModule(module) {
>+  // Put the old entries back
>+  for (let [key, value] in Iterator(gOldProviders))
>+    gCategoryMan.addCategoryEntry(kCategory, key, value, false, true);

Should we clear out any entries we added before we put the old entries back?

>+++ b/mail/test/mozmill/shared-modules/test-cloudfile-helpers.js
>@@ -7,50 +7,43 @@ let Cc = Components.classes;
[snip…]
>-var cfh, fdh, gMockCloudfileComponent;
>+var fdh, moh;
> 
> function setupModule(module) {
>   fdh = collector.getModule("folder-display-helpers");
>   fdh.installInto(module);
> 
>-  let moh = collector.getModule("mock-object-helpers");
>-
>-  gMockCloudfileComponent = new moh.MockObjectRegisterer(
>-      kMockContractID,
>-      kMockCID,
>-      MockCloudfileAccount);
>+  moh = collector.getModule("mock-object-helpers");
>+  moh.installInto(module);

If we're installing into the module, do we really need the global vars?

Aside from those, I like it.  r=me!

Thanks,
Blake.
Comment 19 Mike Conley (:mconley) - (Needinfo me!) 2012-05-09 13:21:51 PDT
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #18)
> >-var cfh, fdh, gMockCloudfileComponent;
> >+var fdh, moh;
> > 
> > function setupModule(module) {
> >   fdh = collector.getModule("folder-display-helpers");
> >   fdh.installInto(module);
> > 
> >-  let moh = collector.getModule("mock-object-helpers");
> >-
> >-  gMockCloudfileComponent = new moh.MockObjectRegisterer(
> >-      kMockContractID,
> >-      kMockCID,
> >-      MockCloudfileAccount);
> >+  moh = collector.getModule("mock-object-helpers");
> >+  moh.installInto(module);
> 
> If we're installing into the module, do we really need the global vars?
> 

Normally, you'd be right, but since we're inside a helper, we can't really load other helpers into our global scope.  The best we can do is load them into the object returned by the collector (this pattern can be seen in a few places in shared-modules - with useful functions like assert_true).

> Aside from those, I like it.  r=me!

Cool, I'll have that other stuff fixed in a jif.  Thanks for the review!
Comment 20 Mike Conley (:mconley) - (Needinfo me!) 2012-05-09 13:26:46 PDT
Created attachment 622486 [details] [diff] [review]
Patch v9 (carrying over r+ from bwinton)

I think we'll want this UI polish bug for beta / aurora - it fixes some rather dumb behaviour in our "add an account" dialog.
Comment 21 Mike Conley (:mconley) - (Needinfo me!) 2012-05-09 13:47:48 PDT
comm-central: http://hg.mozilla.org/comm-central/rev/f29073e5269d
Comment 22 Mike Conley (:mconley) - (Needinfo me!) 2012-05-10 08:21:10 PDT
comm-aurora: https://hg.mozilla.org/releases/comm-aurora/rev/af5a7be911d1
comm-beta: https://hg.mozilla.org/releases/comm-beta/rev/1f230f50d4e9

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