Bug 49308 – Handle Page up/down and home/end keys in list box controls

• Component: Forms
• Status: UNCONFIRMED

[Carlos Garcia Campos]

While working on bug #15816 I realized that page up/down and home/end keys are not handled by SelectElement for list boxes.

[Carlos Garcia Campos]

Created attachment 73469 [details]
Patch to handle page up/down and home/end keys in list boxes

[Alexey Proskuryakov]

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.

[Carlos Garcia Campos]

(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.

[Alexey Proskuryakov]

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.

[Carlos Garcia Campos]

hmm, I see your point, well, it currently happens with combo boxes too.

[Carlos Garcia Campos]

Created attachment 73880 [details]
Updated patch now including tests

It's the same patch but with tests.

[Darin Adler]

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.

[Carlos Garcia Campos]

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.

[David Levin]

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.