The default bug view has changed. See this FAQ.

Filelink add account dialog's "Learn More" looks strange when no more accounts can be added.

RESOLVED FIXED in Thunderbird 15.0

Status

Thunderbird
Preferences
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

Trunk
Thunderbird 15.0
x86
All

Thunderbird Tracking Flags

(thunderbird13+ fixed, thunderbird14+ fixed)

Details

Attachments

(2 attachments, 10 obsolete attachments)

25.52 KB, image/png
Details
34.73 KB, patch
Details | Diff | Splinter Review
Created attachment 617895 [details]
Screenshot of the problem

See screenshot.
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.
Assignee: nobody → mconley
Status: NEW → ASSIGNED
Attachment #620007 - Flags: ui-review?(bwinton)
Attachment #620007 - Flags: review?(bwinton)
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.
Attachment #620007 - Flags: ui-review?(bwinton)
Attachment #620007 - Flags: ui-review-
Attachment #620007 - Flags: review?(bwinton)
Attachment #620007 - Flags: review+
Created attachment 620399 [details] [diff] [review]
WIP Patch
Attachment #620007 - Attachment is obsolete: true
Created attachment 620408 [details] [diff] [review]
WIP Patch 2

OSX and Ubuntu are styled - Windows is next...
Attachment #620399 - Attachment is obsolete: true
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
Attachment #620408 - Attachment is obsolete: true
Attachment #620416 - Flags: ui-review?(bwinton)
Attachment #620416 - Flags: review?(richard.marti)
Comment on attachment 620416 [details] [diff] [review]
Patch v2

I think I can clean this up some first.
Attachment #620416 - Flags: ui-review?(bwinton)
Attachment #620416 - Flags: review?(richard.marti)
Created attachment 620430 [details] [diff] [review]
Patch v3
Attachment #620416 - Attachment is obsolete: true
Attachment #620430 - Flags: ui-review?(bwinton)
Attachment #620430 - Flags: review?(richard.marti)
Created attachment 620432 [details] [diff] [review]
Patch v4

Was disabling an element twice.  Going to give this a once over before requesting review.
Attachment #620430 - Attachment is obsolete: true
Attachment #620430 - Flags: ui-review?(bwinton)
Attachment #620430 - Flags: review?(richard.marti)
Created attachment 620437 [details] [diff] [review]
Patch v5

Ok, this one is good for reviewing.
Attachment #620432 - Attachment is obsolete: true
Attachment #620437 - Flags: ui-review?(bwinton)
Attachment #620437 - Flags: review?(richard.marti)
Attachment #620437 - Flags: review?(bwinton)
Comment on attachment 620437 [details] [diff] [review]
Patch v5

The CSS is looking good.
Attachment #620437 - Flags: review?(richard.marti) → review+
Attachment #620437 - Flags: ui-review?(bwinton)
Attachment #620437 - Flags: ui-review-
Attachment #620437 - Flags: review?(bwinton)
Attachment #620437 - Flags: review-
(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.)
Created attachment 621985 [details] [diff] [review]
Patch v6
Attachment #620437 - Attachment is obsolete: true
Comment on attachment 621985 [details] [diff] [review]
Patch v6

Alright, I feel pretty good about this one.
Attachment #621985 - Flags: ui-review?(bwinton)
Attachment #621985 - Flags: review?(bwinton)
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.
Attachment #621985 - Flags: ui-review?(bwinton)
Attachment #621985 - Flags: ui-review+
Attachment #621985 - Flags: review?(bwinton)
Attachment #621985 - Flags: review+
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
Attachment #621985 - Attachment is obsolete: true
Created attachment 622122 [details] [diff] [review]
Patch v8

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

Ready for review.
Attachment #622120 - Attachment is obsolete: true
Attachment #622122 - Flags: review?(bwinton)
Try build finished, and nothing hairy popped up.
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.
Attachment #622122 - Flags: review?(bwinton) → review+
(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!
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.
Attachment #622122 - Attachment is obsolete: true
Attachment #622486 - Flags: approval-comm-beta?
Attachment #622486 - Flags: approval-comm-aurora?
comm-central: http://hg.mozilla.org/comm-central/rev/f29073e5269d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 15.0

Updated

5 years ago
Attachment #622486 - Flags: approval-comm-beta?
Attachment #622486 - Flags: approval-comm-beta+
Attachment #622486 - Flags: approval-comm-aurora?
Attachment #622486 - Flags: approval-comm-aurora+
tracking-thunderbird13: --- → ?
tracking-thunderbird14: --- → ?
tracking-thunderbird13: ? → +
tracking-thunderbird14: ? → +
comm-aurora: https://hg.mozilla.org/releases/comm-aurora/rev/af5a7be911d1
comm-beta: https://hg.mozilla.org/releases/comm-beta/rev/1f230f50d4e9
status-thunderbird13: --- → fixed
status-thunderbird14: --- → fixed
You need to log in before you can comment on or make changes to this bug.