Closed Bug 955518 Opened 8 years ago Closed 8 years ago

Better handling of navigation keys between the filterbox and listbox

Categories

(Instantbird :: Other, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

References

Details

(Whiteboard: [1.5-blocking])

Attachments

(1 file, 4 obsolete files)

*** 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.
Blocks: 955452
Attached patch Patch (obsolete) — Splinter Review
*** 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)
Attachment #8354430 - Flags: review?(aleth)
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Depends on: 955515
Attached patch Patch 2 (obsolete) — Splinter Review
*** 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)
Attachment #8354432 - Flags: review?(aleth)
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)
*** 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.
Attached patch Patch 3 (obsolete) — Splinter Review
*** 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)
Attachment #8354433 - Flags: review?(aleth)
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)
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+
Whiteboard: [checkin-needed]
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-
Whiteboard: [checkin-needed]
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
Whiteboard: [1.5-blocking]
*** 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.
No longer blocks: 955452
Blocks: 955013
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 2081 as attmnt 3059 at 2013-11-16 19:01:00 UTC ***

This one WFM.
Attachment #8354840 - Flags: review?(benediktp)
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
Whiteboard: [1.5-blocking] → [1.5-blocking][needs review Mic]
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+
Attached patch Patch8Splinter Review
*** 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)
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: nhnt11 → aleth
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+
*** Original post on bio 2081 at 2013-12-02 22:55:43 UTC ***

http://hg.instantbird.org/instantbird/rev/52161c10ae08
Status: ASSIGNED → RESOLVED
Closed: 8 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.