Closed
Bug 955518
Opened 11 years ago
Closed 11 years ago
Better handling of navigation keys between the filterbox and listbox
Categories
(Instantbird Graveyard :: Other, defect)
Instantbird Graveyard
Other
Tracking
(Not tracked)
RESOLVED
FIXED
1.5
People
(Reporter: aleth, Assigned: aleth)
References
Details
(Whiteboard: [1.5-blocking])
Attachments
(1 file, 4 obsolete files)
4.14 KB,
patch
|
florian
:
review+
|
Details | Diff | Splinter Review |
*** Original post on bio 2081 at 2013-07-30 18:19:00 UTC ***
As pressing 'down' from the filterbox selects the first row, and it should be possible to undo this the intuitive way.
Comment 1•11 years ago
|
||
*** Original post on bio 2081 as attmnt 2661 at 2013-07-30 22:24:00 UTC ***
Better handling of navigation keys between the filterbox and listbox.
Attachment #8354430 -
Flags: review?(benediktp)
Updated•11 years ago
|
Attachment #8354430 -
Flags: review?(aleth)
Updated•11 years ago
|
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Comment 2•11 years ago
|
||
*** Original post on bio 2081 as attmnt 2663 at 2013-07-30 22:51:00 UTC ***
This handles Page up in addition to the up key.
Attachment #8354432 -
Flags: review?(benediktp)
Updated•11 years ago
|
Attachment #8354432 -
Flags: review?(aleth)
Comment 3•11 years ago
|
||
Comment on attachment 8354430 [details] [diff] [review]
Patch
*** Original change on bio 2081 attmnt 2661 at 2013-07-30 22:51:14 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354430 -
Attachment is obsolete: true
Attachment #8354430 -
Flags: review?(benediktp)
Attachment #8354430 -
Flags: review?(aleth)
Assignee | ||
Comment 4•11 years ago
|
||
*** Original post on bio 2081 at 2013-07-31 10:49:12 UTC ***
Comment on attachment 8354432 [details] [diff] [review] (bio-attmnt 2663)
Patch 2
This no longer calls listbox.focus() if (listbox.selectedIndex < 0 && listbox.firstChild). Is that intentional?
It might be a good idea to call listbox.ensureIndexIsVisible(0); in the if (listbox.selectedIndex == 0... case too just in case someone used the scrollbar in an odd way.
Comment 5•11 years ago
|
||
*** Original post on bio 2081 as attmnt 2664 at 2013-07-31 21:04:00 UTC ***
This ensures the listbox is focused and also does the ensureIndexIsVisible as aleth suggested.
Attachment #8354433 -
Flags: review?(benediktp)
Updated•11 years ago
|
Attachment #8354433 -
Flags: review?(aleth)
Comment 6•11 years ago
|
||
Comment on attachment 8354432 [details] [diff] [review]
Patch 2
*** Original change on bio 2081 attmnt 2663 at 2013-07-31 21:04:30 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354432 -
Attachment is obsolete: true
Attachment #8354432 -
Flags: review?(benediktp)
Attachment #8354432 -
Flags: review?(aleth)
Assignee | ||
Comment 7•11 years ago
|
||
Comment on attachment 8354433 [details] [diff] [review]
Patch 3
*** Original change on bio 2081 attmnt 2664 at 2013-08-01 09:23:12 UTC ***
Thanks!
Attachment #8354433 -
Flags: review?(aleth) → review+
Assignee | ||
Updated•11 years ago
|
Whiteboard: [checkin-needed]
Comment 8•11 years ago
|
||
Comment on attachment 8354433 [details] [diff] [review]
Patch 3
*** Original change on bio 2081 attmnt 2664 at 2013-08-02 08:38:01 UTC ***
This doesn't work for me when I'm using "Page Down" from the input box. The list scrolls down a page immediately while the first item is being selected and not visible.
You can check that with DOMi or e.g. by using using the scrollbar to scroll to top without moving the mouse over an item, ofcourse, as this would change the selection.
Attachment #8354433 -
Flags: review?(benediktp) → review-
Updated•11 years ago
|
Whiteboard: [checkin-needed]
Updated•11 years ago
|
Summary: Pressing the up key in the top row of the newtab should refocus the filterbox → Better handling of navigation keys between the filterbox and listbox
Assignee | ||
Updated•11 years ago
|
Whiteboard: [1.5-blocking]
Assignee | ||
Comment 9•11 years ago
|
||
*** Original post on bio 2081 at 2013-11-06 16:28:48 UTC ***
Something must have changed since this bug was filed, as pressing 'down' currently selects the second item, not the first.
Assignee | ||
Comment 10•11 years ago
|
||
*** Original post on bio 2081 as attmnt 3059 at 2013-11-16 19:01:00 UTC ***
This one WFM.
Attachment #8354840 -
Flags: review?(benediktp)
Assignee | ||
Comment 11•11 years ago
|
||
Comment on attachment 8354433 [details] [diff] [review]
Patch 3
*** Original change on bio 2081 attmnt 2664 at 2013-11-16 19:01:49 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354433 -
Attachment is obsolete: true
Updated•11 years ago
|
Whiteboard: [1.5-blocking] → [1.5-blocking][needs review Mic]
Comment 12•11 years ago
|
||
Comment on attachment 8354840 [details] [diff] [review]
Patch
*** Original change on bio 2081 attmnt 3059 at 2013-12-01 18:13:55 UTC ***
>diff --git a/instantbird/content/newtab.xml b/instantbird/content/newtab.xml
> const modifierKeyCodes = [KeyEvent.DOM_VK_ALT, KeyEvent.DOM_VK_META,
> KeyEvent.DOM_VK_SHIFT, KeyEvent.DOM_VK_CONTROL];
>
> // If the key is a modifier, don't do anything.
> if (modifierKeyCodes.indexOf(event.keyCode) != -1)
> return;
>+ if (event.altKey || event.metaKey || event.shiftKey || event.ctrlKey)
>+ return;
Why are both these checks needed?
The patch looks reasonable. If I end up stealing the review, I'll want to try it, so just giving feedback+ for now.
Attachment #8354840 -
Flags: feedback+
Assignee | ||
Comment 13•11 years ago
|
||
*** Original post on bio 2081 as attmnt 3107 at 2013-12-01 18:43:00 UTC ***
> > // If the key is a modifier, don't do anything.
> > if (modifierKeyCodes.indexOf(event.keyCode) != -1)
> > return;
> >+ if (event.altKey || event.metaKey || event.shiftKey || event.ctrlKey)
> >+ return;
>
> Why are both these checks needed?
Indeed.
Attachment #8354891 -
Flags: review?(benediktp)
Assignee | ||
Comment 14•11 years ago
|
||
Comment on attachment 8354840 [details] [diff] [review]
Patch
*** Original change on bio 2081 attmnt 3059 at 2013-12-01 18:43:07 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354840 -
Attachment is obsolete: true
Attachment #8354840 -
Flags: review?(benediktp)
Assignee | ||
Updated•11 years ago
|
Assignee: nhnt11 → aleth
Comment 15•11 years ago
|
||
Comment on attachment 8354891 [details] [diff] [review]
Patch8
*** Original change on bio 2081 attmnt 3107 at 2013-12-02 22:42:30 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354891 -
Flags: review?(benediktp) → review+
Comment 16•11 years ago
|
||
*** Original post on bio 2081 at 2013-12-02 22:55:43 UTC ***
http://hg.instantbird.org/instantbird/rev/52161c10ae08
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [1.5-blocking][needs review Mic] → [1.5-blocking]
Target Milestone: --- → 1.5
You need to log in
before you can comment on or make changes to this bug.
Description
•