385374 - After performing an operation to the selected richlistitem, it is no longer focused

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect, P2)

Description

[Robert Strong (they/them - no direct email)]

This only affects trunk.

Steps to reproduce:
1) Select an extension in the extensions view.
2) Use arrow up or down to verify adjacent extensions can be selected by using these keys
3) Click enable or disable for the selected extension
4) Use arrow up or down to select an adjacent extension

Actual results:
entire list is scrolled if a scroll bar is displayed

Expected results:
adjacent extension is selected

Comment 1

[Simon Bünzli]

Regression from bug 377686? If so, the question is whether to add keyboard navigation handlers for the bubble phase to richlistbox.xml in general or just to the EM binding in particular.

Comment 2

[Simon Bünzli]

Attachment: react to bubbling navigation key events

The same issue also exists with the Downloads Manager, so other implementors might expect this behavior as well. This patch restores the previous behavior with the slight difference that focus is returned to the richlistbox on navigation so that it doesn't get lost unexpectedly.

Note that hitting [Left] or [Right] still allows the focus to leave the richlistbox, but that's another bug if not expected behavior.

Comment 3

[Neil Deakin]

Why would we want to fire two key events. Capturing listeners would think that the key was pressed twice, no?

Comment 4

[Simon Bünzli]

Attachment: react to bubbling navigation key events (take 2)

(In reply to comment #3)
> Why would we want to fire two key events.

The patch was missing an event.stopPropagation(); so that we'd just have replaced one event with another for best code-reuse - but during the capture phase there'd indeed have been two events instead of one.

So we can either just copy the handlers from listbox.xml, adding

  if (event.originalTarget != this) { this.focus(); ... }

to each of them, or use one common handler for all cases as this patch proposes.

Comment 5

[Neil Deakin]

I'll need to think about this more. Maybe reverting the change to combine  the handlers between the two bindings. Maybe another Neil has some thoughts.

Comment 6

[[e-mail]]

Well, listboxes have the same issue (I used the mail filter dialog ... create a few rows so that a scrollbar appears, then tab to a central + button and hit down and up but be careful not to hit the same key twice otherwise focus goes elsewhere). At the very least I suggest button.xml calls event.preventDefault() when advancing/rewinding focus. Once that is done it should be safe to remove phase="target" from listbox.xml and check event.getPreventDefault() instead. Note that this doesn't actually allow an adjacent extension to be selected.

Comment 7

[Neil Deakin]

Comment on attachment 269508 [details] [diff] [review]
react to bubbling navigation key events (take 2)

Removing review request as the other Neil's idea seems possible.

Comment 8

[Przemyslaw Bialik]

I can add that the focus is lost also when clicking on uninstall or options buttons.
This might be out of scope of this bug ? but when switching between e.g. extensions and themes tab the last selected item should get focus as it was the case with FF 2.0, currently it is selected but without focus.
Also surprisingly after having selected one item under plugins tab and relaunching EM - no item is selected so focus-last-selected-item feature does not work here, got ideas why ?

Comment 10

[Shawn Wilsher :sdwilsh]

We also hit this with the download manager when searching, and the external protocol handling dialog hits it all the time (at least on the mac for both of these).

I think we should probably up the priority on this?

Comment 11

[Robert Strong (they/them - no direct email)]

Shawn, are there bugs for this for the download manager and the protocol handling dialog? Also, there should probably be a widget bug to cover all the consumers of the richlistbox. The last time I worked around these types of issues in extensions.xml late in the game for Firefox 2.0 which was not ideal and it would be a 'good thing' to fix this in richlistbox.xml this time around.

Comment 12

[Shawn Wilsher :sdwilsh]

No - I was talking Dave about this and figured I'd comment to indicate that there is probably a larger problem with the widget and not the consumers (maybe move the bug to Toolkit:Widgets?)

Comment 13

[Robert Strong (they/them - no direct email)]

Moving over to Toolkit -> XUL Widgets per comment #10 and comment #12

Comment 14

[Robert Strong (they/them - no direct email)]

This was already blocking Firefox 3.0 so requesting blocking 1.9

Comment 16

[Dão Gottwald [:dao]]

Attachment: patch as per comment 6

Comment 17

[Dão Gottwald [:dao]]

Attachment: patch as per comment 6 (typo fixed)

Comment 18

[Damon Sicore (:damons)]

Assigning to Dao based on the fact he's actually doing the work.  Thanks for the patch, Dao.

Comment 19

[[e-mail]]

Comment on attachment 310088 [details] [diff] [review]
patch as per comment 6 (typo fixed)

r=me on the button.xml change which is good to stop the unexpected listbox scrolling but the rest of the widget set isn't ready for the listbox.xml change.

Comment 20

[Dão Gottwald [:dao]]

Do you know of specific widgets that wouldn't be ready? I didn't find any in toolkit/content/widgets/.

Comment 21

[[e-mail]]

The one I noticed was that a menulist handles up/down but doesn't seem to call preventDefault; I think textboxes (well, textareas) might call preventDefault but that will be in the system group so you will need group="system" too.

Comment 22

[Dão Gottwald [:dao]]

Attachment: also fix menulists and textboxes

yeah, group=system makes it work for multiline textboxes, although I don't know what it's doing exactly. Doesn't seem to be documented.

Comment 23

[[e-mail]]

Comment on attachment 311128 [details] [diff] [review]
also fix menulists and textboxes

OK, this works with all of the (rich|)listboxes I could think of.

Comment 24

[Reed Loden [:reed]]

Checking in toolkit/content/widgets/button.xml;
/cvsroot/mozilla/toolkit/content/widgets/button.xml,v  <--  button.xml
new revision: 1.22; previous revision: 1.21
done
Checking in toolkit/content/widgets/listbox.xml;
/cvsroot/mozilla/toolkit/content/widgets/listbox.xml,v  <--  listbox.xml
new revision: 1.35; previous revision: 1.34
done
Checking in toolkit/content/widgets/menulist.xml;
/cvsroot/mozilla/toolkit/content/widgets/menulist.xml,v  <--  menulist.xml
new revision: 1.40; previous revision: 1.39
done