Closed
Bug 953625
Opened 10 years ago
Closed 10 years ago
wherever the focus is, make sure the up and down arrow change the selected account
Categories
(Instantbird Graveyard :: Account manager, enhancement)
Instantbird Graveyard
Account manager
Tracking
(Not tracked)
RESOLVED
FIXED
0.2b1
People
(Reporter: florian, Assigned: romain)
References
Details
Attachments
(1 file, 3 obsolete files)
5.90 KB,
patch
|
florian
:
review+
|
Details | Diff | Splinter Review |
*** 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.
Assignee | ||
Comment 1•10 years ago
|
||
*** 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.
Assignee | ||
Comment 2•10 years ago
|
||
*** 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.
Attachment #8351898 -
Flags: review?(florian)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → romain
Status: NEW → ASSIGNED
Reporter | ||
Comment 3•10 years ago
|
||
*** 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.
Reporter | ||
Comment 4•10 years ago
|
||
*** 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.
Attachment #8351900 -
Flags: review?(romain)
Assignee | ||
Comment 5•10 years ago
|
||
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 ***
Attachment #8351900 -
Flags: review?(romain) → review+
Assignee | ||
Comment 6•10 years ago
|
||
*** 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.
Attachment #8351902 -
Flags: review?(florian)
Assignee | ||
Comment 7•10 years ago
|
||
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 ***
Attachment #8351898 -
Attachment is obsolete: true
Attachment #8351898 -
Flags: review?(florian)
Assignee | ||
Comment 8•10 years ago
|
||
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 ***
Attachment #8351900 -
Attachment is obsolete: true
Reporter | ||
Comment 9•10 years ago
|
||
*** 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 ;).
Assignee | ||
Comment 10•10 years ago
|
||
*** 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 :).
Assignee | ||
Comment 11•10 years ago
|
||
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 ***
Attachment #8351902 -
Attachment is obsolete: true
Attachment #8351902 -
Flags: review?(florian)
Reporter | ||
Comment 12•10 years ago
|
||
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 ***
Attachment #8351903 -
Flags: review+
Reporter | ||
Comment 13•10 years ago
|
||
*** 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.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: 0.2a1 → 0.2b1
You need to log in
before you can comment on or make changes to this bug.
Description
•