Closed Bug 385374 Opened 13 years ago Closed 12 years ago

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


(Toolkit :: XUL Widgets, defect, P2)






(Reporter: robert.strong.bugs, Assigned: dao)



(Keywords: access, regression)


(1 file, 4 obsolete files)

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
Flags: blocking-firefox3?
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.
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.
Attachment #269389 - Flags: review?(enndeakin)
Why would we want to fire two key events. Capturing listeners would think that the key was pressed twice, no?
(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.
Assignee: nobody → zeniko
Attachment #269389 - Attachment is obsolete: true
Attachment #269508 - Flags: review?(enndeakin)
Attachment #269389 - Flags: review?(enndeakin)
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.
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.
Flags: blocking-firefox3? → blocking-firefox3+
Target Milestone: --- → Firefox 3 M8
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.
Attachment #269508 - Flags: review?(enndeakin)
Assignee: zeniko → nobody
Target Milestone: Firefox 3 M8 → Firefox 3 M9
Target Milestone: Firefox 3 M9 → Firefox 3 M10
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 ?
OS: Windows Vista → All
Priority: -- → P1
Target Milestone: Firefox 3 M10 → Firefox 3 M11
Priority: P1 → P3
Duplicate of this bug: 406739
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?
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.
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?)
Moving over to Toolkit -> XUL Widgets per comment #10 and comment #12
Component: Extension/Theme Manager → XUL Widgets
Flags: blocking-firefox3+
Product: Firefox → Toolkit
QA Contact: extension.manager → xul.widgets
Summary: After performing an operation to an add-on the selected item is no longer focused → After performing an operation to the selected item is no longer focused
Target Milestone: Firefox 3 beta3 → ---
This was already blocking Firefox 3.0 so requesting blocking 1.9
Flags: blocking1.9?
Duplicate of this bug: 416392
Flags: blocking1.9? → blocking1.9+
Summary: After performing an operation to the selected item is no longer focused → After performing an operation to the selected richlistitem, it is no longer focused
Flags: tracking1.9+ → blocking1.9+
Keywords: access
Priority: P3 → P2
Attached patch patch as per comment 6 (obsolete) — Splinter Review
Attachment #269508 - Attachment is obsolete: true
Attachment #310085 - Flags: review?(neil)
Attachment #310085 - Attachment is obsolete: true
Attachment #310088 - Flags: review?(neil)
Attachment #310085 - Flags: review?(neil)
Assigning to Dao based on the fact he's actually doing the work.  Thanks for the patch, Dao.
Assignee: nobody → dao
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.
Attachment #310088 - Flags: review?(neil) → review+
Do you know of specific widgets that wouldn't be ready? I didn't find any in toolkit/content/widgets/.
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.
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.
Attachment #310088 - Attachment is obsolete: true
Attachment #311128 - Flags: review?(neil)
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.
Attachment #311128 - Flags: review?(neil) → review+
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
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
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
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta5
Target Milestone: mozilla1.9beta5 → mozilla1.9
You need to log in before you can comment on or make changes to this bug.