Closed Bug 250091 Opened 16 years ago Closed 5 years ago

xul:menulist doesn't support the Page Up and Page Down keys

Categories

(Core :: User events and focus handling, defect, minor)

x86
Windows XP
defect
Not set
minor
Points:
3

Tracking

()

RESOLVED FIXED
mozilla41
Iteration:
41.1 - May 25
Tracking Status
e10s m7+ ---
firefox41 --- fixed

People

(Reporter: richard.corbin, Assigned: enndeakin)

References

Details

(Keywords: access, helpwanted)

Attachments

(1 file, 2 obsolete files)

The dropdown boxes to choose the headers & footers combo in the 'File > Page
Setup ' dialog do not support the use of the Home, End, Page Up / PgUp, and Page
Down / PgDn keys for quick usage.

Steps to reproduce:
1. File > Page Setup
2. Select Margins & Header/Footer tab
3. In the 'Headers & Footers' section select a dropdown.
4a. Use End key
4b. Use Home key
4c. Use Page Down key
4d. Use Page Up key

Actual Results:
4a,b,c,d. Nothing happens

Expected Results:
4a. Taken to End/Bottom of dropdown list
4b. Taken to Home/Top of dropdown list
4c. Taken down a page in dropdown list (in this case, due to small dropdown
should be taken to bottom of list)
4d. Taken up a page in dropdown list (in this case, due to small dropdown should
be taken to top of list)


Using: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040704
Firefox/0.9.0+
Apologies, step 3 should read:

3. In the 'Headers & Footers' tab to select a dropdown.

Stick to keyboard. (If you click with mouse the dropdown opens and then the
Home/End/PgUp/PgDn keys work).
Assignee: firefox → aaronleventhal
Component: General → Keyboard Navigation
QA Contact: firefox.general → jruderman
- True for all combo boxes in Mozilla
- Home and End do currently work when the dropdown is open, but not when it's closed
- PageUp/PageDown never work
Keywords: helpwanted
Summary: Page Setup Headers and Footers dropdowns don't support the Home, End, PgUp, PgDn keys → Combo boxes don't support the Home, End, PgUp, PgDn keys
"Home" and "End" now working for me in Firefox 1.5 beta 1.

Not sure if PageUp and PageDn ever need to be fixed?
Everything is working for me now except pageup/pagedown in a closed list. They
work when the list is dropped down.
QA Contact: jruderman → keyboard.navigation
Adjusted description to reflect current state of this bug.
Summary: Combo boxes don't support the Home, End, PgUp, PgDn keys → Combo boxes don't support the Page Up and Page Down keys
Assignee: aaronleventhal → nobody
Component: Keyboard Navigation → Keyboard: Navigation
Keywords: access
Product: Firefox → Core
QA Contact: keyboard.navigation → keyboard.navigation
Summary: Combo boxes don't support the Page Up and Page Down keys → Closed xul:menulist doesn't support the Page Up and Page Down keys
Assignee: nobody → enndeakin
My comment 4 is wrong, it's broken in open menulists as well
Summary: Closed xul:menulist doesn't support the Page Up and Page Down keys → xul:menulist doesn't support the Page Up and Page Down keys
What would pageup/pagedown do? Go to the start/end of the list?
I suggest looking at the code for the HTML combo and/or playing with the implementation in combo with a lot of items to see how it works.

The impl is based on what the combo box would look like if it were open. The first page up or dn goes to the first or last visible item at the current time. If you're already at the top or bottom of the visible list, the next one moves by the number of rows in the popup.
Version: unspecified → Trunk
This is a chrome bug.
No longer blocks: 1118089
Blocks: 1118089
Assignee: enndeakin → gwright
tracking-e10s: --- → m7+
This is chrome bug that break the current (new) way we use popups in e10s, hence it blocks the project.
Assignee: gwright → enndeakin
Status: NEW → ASSIGNED
Iteration: --- → 41.1 - May 25
Points: --- → 3
Flags: in-testsuite+
Attachment #8600434 - Attachment is obsolete: true
Attachment #8602200 - Flags: review?(neil)
Comment on attachment 8602200 [details] [diff] [review]
Patch with fixed up coordinate issues

>+    // Only scroll by page within menulists.
Would be nice if this worked for closed menulists too, but I guess we don't know the height?

>+  nsMenuFrame* newMenu = NULL;
Nit: nullptr

>+    // Get the position of the current item and add or subtract one popup's height
>+    // to or from it.
I'm worried that this makes too many assumptions about the layout. Would it be possible to use the scroll frame's height instead?

>+    // Look for the next child which is just past the target position. This child
>+    // will need to be selected.
The <select> code looks as if it selects the last visible child, rather than the first invisible child. (So for example, after pressing Page Up, pressing a single Page Down selects the last visible item, and then the next Page Down scrolls.) This is probably easily fixed by setting newMenu after the break condition.

r=me with the above fixed.
Attachment #8602200 - Flags: review?(neil) → review+
Attached patch menulistbypageSplinter Review
> Would be nice if this worked for closed menulists too, but I guess we don't know the height?

We do know the height since menulists have sizetopopup set, but we would need to update the menulist value as well. This can be a separate bug.

> This is probably easily fixed by setting newMenu after the break condition.

I added some additional code to handle when newMenu is disabled, so that we skip over any disabled items at the end of the page.
Attachment #8602200 - Attachment is obsolete: true
Attachment #8604821 - Flags: review?(neil)
(In reply to Neil Deakin from comment #14)
> > Would be nice if this worked for closed menulists too, but I guess we don't know the height?
> 
> We do know the height since menulists have sizetopopup set

When I tried the following code:
@@ -106,2 +106,8 @@ bool MenuBoxObject::HandleKeyPress(Keybo
   switch (keyCode) {
+    case nsIDOMKeyEvent::DOM_VK_PAGE_UP:
+    case nsIDOMKeyEvent::DOM_VK_PAGE_DOWN:
+    {
+      popupFrame->ChangeByPage(keyCode == nsIDOMKeyEvent::DOM_VK_PAGE_UP);
+      return true;
+    }
     case nsIDOMKeyEvent::DOM_VK_UP:
Page Up and Page Down simply selected the first or last item unless the menu had been open at least once. Perhaps it needs a call to SetPopupPosition?

> but we would need to update the menulist value as well.
Code for that already exists in menulist.xml

> I added some additional code to handle when newMenu is disabled, so that we
> skip over any disabled items at the end of the page.
Seems to work OK.
Comment on attachment 8604821 [details] [diff] [review]
menulistbypage

>+    nsIScrollableFrame *scrollframe = do_QueryFrame(nsBox::GetChildBox(this));
Please could you use GetScrollFrame(this) instead? It works in an edge case where this code doesn't.
Attachment #8604821 - Flags: review?(neil) → review+
https://hg.mozilla.org/mozilla-central/rev/d192c8d0ddb6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Component: Keyboard: Navigation → User events and focus handling
You need to log in before you can comment on or make changes to this bug.