953625 - wherever the focus is, make sure the up and down arrow change the selected account

Status: RESOLVED FIXED

(Instantbird Graveyard :: Account manager, enhancement)

Description

[Florian Quèze [:florian]]

*** Original post on bio 178 at 2009-04-24 20:40:00 UTC ***

When the account manager window has focus, the up/down arrow keys should always change the selected account, at least when the focused element is a inside the accounts list.

Bug 953624 (bio 177) is also focus-related.

Comment 1

[Romain BEZUT]

*** Original post on bio 178 at 2009-07-31 22:09:11 UTC ***

In order not to break the keyboard accessibility, this patch should include an approved patch at Bug 953624 (bio 177), so that they could be pushed together.

Comment 2

[Romain BEZUT]

Attachment: Patch

*** Original post on bio 178 as attmnt 154 at 2009-08-01 00:28:00 UTC ***

This patch includes the approved patch at Bug 953624 (bio 177).

I tried to apply the same rule as the one I apply for keypress (phase=caturing), but it does not work, so it is better to keep the hack for onselect.

In this patch the propagation of the key event if stopped, but the default action is kept, so that it changes the selected item only.

Need to be tested on Windows and Mac.

Comment 3

[Florian Quèze [:florian]]

*** Original post on bio 178 at 2009-08-01 11:56:50 UTC ***

Comment on attachment 8351898 [details] [diff] [review] (bio-attmnt 154)
Patch

>diff --git a/instantbird/base/content/instantbird/accounts.js b/instantbird/base/content/instantbird/accounts.js

>+  onKeyPress: function am_onKeyPress(event) {
>+    // Here we don't prevent default action (changing selected account)

I'm not sure I understand this comment right.
Do you mean that because we stop the propagation of the event, the default action of the richlistbox will be executed instead of the (default) event handler of the child elements?
If so, please rephrase the comment to make the meaning obvious :).

>+    if (event.keyCode == event.DOM_VK_DOWN) {
>+      if (this.selectedIndex < this.itemCount - 1)
>+        this.ensureIndexIsVisible(this.selectedIndex + 1);
>+      event.stopPropagation();
>+      return;
>+    }
maybe use an else instead of the return here? Or if you keep the return, add an empty line here to make it more visible.

>+    if (event.keyCode == event.DOM_VK_UP) {
>+      if (this.selectedIndex > 0)
>+        this.ensureIndexIsVisible(this.selectedIndex - 1);
>+      event.stopPropagation();
>+      return;
>+    }
This return statement looks really useless.

Comment 4

[Florian Quèze [:florian]]

Attachment: additionnal change

*** Original post on bio 178 as attmnt 156 at 2009-08-01 12:01:00 UTC ***

I tested this on my Mac and noticed that when connecting the account, the focus is kept by the 'Connect' button even when it's no longer visible. The focus should go to the 'Disconnect' button instead.
The attached patch attempts to fix this detail. I would like if you could check it.

This issue may be more related to bug 953624 (bio 177) though.

Comment 5

[Romain BEZUT]

Comment on attachment 8351900 [details] [diff] [review]
additionnal change

*** Original change on bio 178 attmnt 156 at 2009-08-01 12:40:01 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 6

[Romain BEZUT]

Attachment: Patch V3

*** Original post on bio 178 as attmnt 158 at 2009-08-01 12:45:00 UTC ***

> Do you mean that because we stop the propagation of the event, the default
> action of the richlistbox will be executed instead of the (default) event
> handler of the child elements?
Yes that is what I tried to explain
By preventing propagation, the default action is applied to the richlistbox, so that we don't need to change the selectedIndex "manually" in the function.


I applied the coding style fixes, and included your patch in this one.
I had the same behavior, after connecting an account, or disconnecting it, the old button had still the focus, and we were unable to move with arrows anymore.

I didn't pay attention to this point.

Comment 7

[Romain BEZUT]

Comment on attachment 8351898 [details] [diff] [review]
Patch

*** Original change on bio 178 attmnt 154 at 2009-08-01 12:45:11 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 8

[Romain BEZUT]

Comment on attachment 8351900 [details] [diff] [review]
additionnal change

*** Original change on bio 178 attmnt 156 at 2009-08-01 12:45:11 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 9

[Florian Quèze [:florian]]

*** Original post on bio 178 at 2009-08-01 13:25:50 UTC ***

(In reply to comment #5)
> Created an attachment (id=158) [details]
> Patch V3
> 
> > Do you mean that because we stop the propagation of the event, the default
> > action of the richlistbox will be executed instead of the (default) event
> > handler of the child elements?
> Yes that is what I tried to explain
> By preventing propagation, the default action is applied to the richlistbox, so
> that we don't need to change the selectedIndex "manually" in the function.

Rephrase the comment in the code please ;).

Comment 10

[Romain BEZUT]

Attachment: Patch V3

*** Original post on bio 178 as attmnt 159 at 2009-08-01 13:31:00 UTC ***

Changed the comment to make things clearer, I thought you was talking about the comment on Bugzilla :).

Comment 11

[Romain BEZUT]

Comment on attachment 8351902 [details] [diff] [review]
Patch V3

*** Original change on bio 178 attmnt 158 at 2009-08-01 13:31:30 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 12

[Florian Quèze [:florian]]

Comment on attachment 8351903 [details] [diff] [review]
Patch V3

*** Original change on bio 178 attmnt 159 at 2009-08-01 13:33:17 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 13

[Florian Quèze [:florian]]

*** Original post on bio 178 at 2009-08-02 21:56:24 UTC ***

I pushed attachment 8351903 [details] [diff] [review] (bio-attmnt 159) as 3 separate changesets:
https://hg.instantbird.org/instantbird/rev/2030a20dacf6
https://hg.instantbird.org/instantbird/rev/2ccaf97bc24c
https://hg.instantbird.org/instantbird/rev/6a3750daacc3

The first 2 are actually more related to bug 953624 (bio 177) than this bug.