Closed Bug 953650 Opened 7 years ago Closed 7 years ago

Changing the selected account with up/down arrows doesn't work when the selected account is of an unknown protocol

Categories

(Instantbird :: Account manager, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: florian, Assigned: romain)

References

Details

Attachments

(1 file, 1 obsolete file)

1009 bytes, patch
florian
: review+
Details | Diff | Splinter Review
*** Original post on bio 204 at 2009-08-03 18:16:00 UTC ***

To reproduce this, make sure you have an account with an unknown protocol. For example, install the facebook plugin, create a facebook account and then disable or uninstall the facebook plugin.
Blocks: 953625
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 204 as attmnt 161 at 2009-08-04 00:58:00 UTC ***

More generally, if the button is disabled, then the richlistbox is selected instead.
Attachment #8351905 - Flags: review?(florian)
Assignee: nobody → romain
Status: NEW → ASSIGNED
Attached patch Patch V2Splinter Review
*** Original post on bio 204 as attmnt 165 at 2009-08-04 22:09:00 UTC ***

Coding style
Attachment #8351909 - Flags: review?(florian)
Comment on attachment 8351905 [details] [diff] [review]
Patch

*** Original change on bio 204 attmnt 161 at 2009-08-04 22:09:29 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351905 - Attachment is obsolete: true
Attachment #8351905 - Flags: review?(florian)
Comment on attachment 8351905 [details] [diff] [review]
Patch

*** Original change on bio 204 attmnt 161 at 2009-08-04 22:12:17 UTC ***

Here are the review comments I already gave by private IM:

>diff --git a/instantbird/base/content/instantbird/account.xml b/instantbird/base/content/instantbird/account.xml

>-        let focusElt = (this._account.disconnected) ? "connect" : "disconnect";
>-        document.getAnonymousElementByAttribute(this, "anonid", focusElt).focus();
>+        let focusedElt = this._account.disconnected ? "connect" : "disconnect";
>+        let button = document.getAnonymousElementByAttribute(this, "anonid",
>+                                                                focusedElt);
>+        if (!button.disabled)
>+          button.focus();
>+        else
>+          document.getElementById("accountlist").focus();
I don't really like the duplication of .focus() and the else here.
I'd prefer something like this:
   if (button.disabled)
     button = document.getElementById("accountlist");
   button.focus();

If you change this, the variable names aren't suitable, I propose 'action' and
'focusTarget' to replace 'focusElt' and 'button'.
Attachment #8351905 - Flags: review-
Comment on attachment 8351909 [details] [diff] [review]
Patch V2

*** Original change on bio 204 attmnt 165 at 2009-08-04 22:13:47 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351909 - Attachment description: Patch V3 → Patch V2
Attachment #8351909 - Flags: review?(florian) → review+
*** Original post on bio 204 at 2009-08-04 23:12:44 UTC ***

attachment 8351909 [details] [diff] [review] (bio-attmnt 165) pushed:
https://hg.instantbird.org/instantbird/rev/82cecab96020
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.2b1
You need to log in before you can comment on or make changes to this bug.