While working on bug #15816 I realized that page up/down and home/end keys are not handled by SelectElement for list boxes.
Created attachment 73469 [details] Patch to handle page up/down and home/end keys in list boxes
This bug doesn't explain why. Does this better match other browsers? Or some/all common platforms? Note that right now, PageUp/PageDown/Home/End aren't just ignored - they are handled by outer object, e.g. for scrolling the whole page.
(In reply to comment #2) > This bug doesn't explain why. Does this better match other browsers? Or some/all common platforms? I don't know other browsers, but the patch does the same than firefox, and I don't know other platforms either, but gtk widgets implement this behaviour too. > Note that right now, PageUp/PageDown/Home/End aren't just ignored - they are handled by outer object, e.g. for scrolling the whole page. Sure, this is just when the list box has the focus.
In the common case of a small list box, this change would make behavior worse, because one wouldn't be able to scroll the page with keyboard when focus is on the box. This is why I'm questioning it.
hmm, I see your point, well, it currently happens with combo boxes too.
Created attachment 73880 [details] Updated patch now including tests It's the same patch but with tests.
Comment on attachment 73880 [details] Updated patch now including tests View in context: https://bugs.webkit.org/attachment.cgi?id=73880&action=review Thanks for tackling this. Looks pretty good. There are a few improvements I’d like to see before you land. > WebCore/ChangeLog:6 > + Handle Page up/down and home/end keys in list box controls > + https://bugs.webkit.org/show_bug.cgi?id=49308 This patch says its adding new keys, but that misrepresents it. It also changes the behavior of existing keys; you need to explain why the new algorithm is better. It's possible you are now doing the right thing for some platforms and the wrong thing for others. > WebCore/dom/SelectElement.cpp:762 > + endIndex = nextSelectableListIndex(data, element, > + (data.activeSelectionEndIndex() < 0) ? > + lastSelectedListIndex(data, element) : > + data.activeSelectionEndIndex()); Multiple-line function calls should not be indented so the arguments line up with the parenthesis opening the argument list in WebKit code. Multiple line if statement bodies need braces in WebKit coding style. It’s based on lines of source code, not statements, so braces are needed here. I suggest making helper functions so you don’t have to put these long ? : expressions here. > WebCore/dom/SelectElement.cpp:776 > + int widowSize = toRenderListBox(element->renderer())->numVisibleItems() - 1; > + int nextIndex = (data.activeSelectionEndIndex() < 0) ? lastSelectedListIndex(data, element) : data.activeSelectionEndIndex(); > + > + endIndex = nextSelectableListIndex(data, element, min((int) listItems.size() - 1, nextIndex + widowSize - 1)); This code needs some “why” comments. The term “widow size” is also not all that clear. Is it the size of a “widow”? I don’t think so, so the name is not great. I suggest making some helper functions so the code is easier to read. We do not use C-style casts, so you’ll want to use static_cast or find some way to avoid the typecasting. > WebCore/dom/SelectElement.cpp:789 > - if (keyIdentifier == "Down" || keyIdentifier == "Up") { > + if (endIndex != -1) { Using a special value of -1 is not a great way to handle this. A separate boolean is probably a better idea. Or if you do want a special value, then please use a name for it rather than just -1.
Created attachment 78680 [details] Updated patch I've added helper funtions to make the code easier and fixed the typo 'widowSize' using pageSize instead since it actually represents the size of the page used for Page up/down.
Comment on attachment 78680 [details] Updated patch Hmm. it looks like I just approved a similar change here: https://bugs.webkit.org/show_bug.cgi?id=27658 If folks object, that could be rolled out. Also, cgarcia feel free to look over that code and if you see improvements, submit a patch for them.