Bug 22784 – [Chromium] Home, End, PageUp and PageDown buttons do not do anything in drop down lists

• Component: New Bugs
• Status: RESOLVED FIXED

[jasneet]

I Steps:
Go to attached testcase

II Issue:
Home and End buttons do not do anything in drop down lists

III Other Browsers:
IE7: ok
FF3: ok

IV Nightly tested: 39088

Bug in Chromium : http://code.google.com/p/chromium/issues/detail?id=4576

[jasneet]

Created attachment 25920 [details]
testcase

[Jens Alfke]

Created attachment 32577 [details]
Fixes bug 22784

[Mark Rowe (bdash)]

Comment on attachment 32577 [details]
Fixes bug 22784

> Index: WebCore/dom/SelectElement.cpp
> ===================================================================
> --- WebCore/dom/SelectElement.cpp	(revision 45713)
> +++ WebCore/dom/SelectElement.cpp	(working copy)
> @@ -47,7 +47,9 @@
>  #include "WMLSelectElement.h"
>  #endif
>  
> -#if PLATFORM(MAC)
> +// On Mac (Safari or Chrome), pop up the menu if the user presses the arrow keys
> +// while a Select element has focus. On other platforms, just change the selection.
> +#if PLATFORM(DARWIN) && !PLATFORM(IPHONE)

This isn't the appropriate check.  PLATFORM(DARWIN) evaluates to true for any port building on Mac OS X, including Qt, GTK and wx.  It indicates the presence of the BSD-level functionality of Darwin, and says nothing about the behavior of higher levels of the stack.

[Jens Alfke]

Here's a fix. 

* I added support for "Home", "End", "PgUp" and "PgDown" keys. To reduce
duplicated code between all these cases (and the arrow keys) I factored out the
list-traversal into a new static function.

* This bug affects non-Mac platforms. I found that it also affects the Mac
version of Chromium, because in Chromium PLATFORM(MAC) has to be false. But Mac
Chromium should follow the Mac HI guidelines the way Safari does, so I changed
the tests for defining ARROW_KEYS_POP_MENU so that it will also be true in Mac
Chromium. The logic gets a little weird, but I did what I could with the
existing platform flags. Maybe we need a new flag
PLATFORM(MAC_INCLUDING_CHROMIUM) ;-)

N.b. this is also being tracked as
http://code.google.com/p/chromium/issues/detail?id=4576

[Jens Alfke]

Mark -- I agree the test is kludgy. What do you suggest using instead? I am told that Chromium cannot turn on PLATFORM(MAC) because it enables all sorts of assumptions throughout WebKit that it's running inside an AppKit view hierarchy, which is definitely not true in Chromium.
If you think a new flag is necessary, we can do that; I didn't want to propose it myself since it seemed kind of a wide-ranging change for a newbie to make!

[Mark Rowe (bdash)]

Comment on attachment 32577 [details]
Fixes bug 22784

> -#if PLATFORM(MAC)
> +// On Mac (Safari or Chrome), pop up the menu if the user presses the arrow keys
> +// while a Select element has focus. On other platforms, just change the selection.
> +#if PLATFORM(DARWIN) && !PLATFORM(IPHONE)
>  #define ARROW_KEYS_POP_MENU 1
>  #else
>  #define ARROW_KEYS_POP_MENU 0

Another minor comment: Mac WebKit is more than just Safari, so it doesn't makes too much sense to mention Safari here.

> +#if !ARROW_KEYS_POP_MENU
> +// Returns the index of the next valid list item in direction |dir| past |listIndex|.
> +static int nextValidIndex(int listIndex, int dir, const Vector<Element*>& listItems) {
> +    ASSERT(dir!=0);
> +    int size = listItems.size();
> +    for (listIndex += dir; listIndex >= 0 && listIndex < size; listIndex += dir) {
> +        if (!listItems[listIndex]->disabled() && isOptionElement(listItems[listIndex]))
> +            return listIndex;
> +    }
> +    return -1;
> +}
> +#endif

There are some minor style issues here (brace on same line as function definition, lack of whitespace around operators).  We also prefer not to unnecessarily abbreviate variable names such as "dir".  It may be clearer to use an enum rather than magic values such as -1 and 1 to represent up and down as the directions.

[Mark Rowe (bdash)]

(In reply to comment #5)
> Mark -- I agree the test is kludgy. What do you suggest using instead?

I don't know what you should use.  I just know that PLATFORM(DARWIN) is not it.  You'd need to speak with someone familiar with the Chromium port to find out the right define for your needs.

[Jens Alfke]

Looking at Platform.h, a better check might be
    PLATFORM(DARWIN) && !PLATFORM(GTK) && !PLATFORM(WX)
which is based on the same logic used to compute PLATFORM(MAC) when
BUILDING_CHROMIUM__ is not turned on. Does that seem, if not reasonable, than
at least acceptable since we don't have the right flag yet?

Will change the reference to Safari.

[Jens Alfke]

Created attachment 32580 [details]
2nd patch

[Jens Alfke]

Just noticed you added "[Chromium]" to the title. This bug doesn't only affect Chromium; it affects WebKit itself on all non-Mac platforms.

[Mark Rowe (bdash)]

Comment on attachment 32580 [details]
2nd patch

> Index: WebCore/dom/SelectElement.cpp
> ===================================================================
> --- WebCore/dom/SelectElement.cpp	(revision 45713)
> +++ WebCore/dom/SelectElement.cpp	(working copy)
> @@ -47,7 +47,10 @@
>  #include "WMLSelectElement.h"
>  #endif
>  
> -#if PLATFORM(MAC)
> +// On Mac OS, pop up the menu if the user presses the arrow keys while a Select element has focus.
> +// On other platforms, just change the selection.
> +// (Cannot just test PLATFORM(MAC), because that is false in Chromium due to its AppKit assumptions.)
> +#if PLATFORM(DARWIN) && !PLATFORM(GTK) && !PLATFORM(WX)
>  #define ARROW_KEYS_POP_MENU 1
>  #else
>  #define ARROW_KEYS_POP_MENU 0

At the very least this is not correct because it omits Qt.  Thinking about this a little more, I suspect what you really want is PLATFORM(MAC) || (PLATFORM(CHROMIUM) && PLATFORM(DARWIN))

Marking as r- due to this and the other unaddressed comments.

[Mark Rowe (bdash)]

(In reply to comment #10)
> Just noticed you added "[Chromium]" to the title. This bug doesn't only affect
> Chromium; it affects WebKit itself on all non-Mac platforms.

In that case, the changes to the #if's (Pop-up menus in Chromium don't behave like Mac pop-ups) seem to be a different issue from what this bug talks about (Home and end don't do anything in select elements on non-Mac platforms).

[Jens Alfke]

Created attachment 32582 [details]
updated patch

[Jens Alfke]

New patch should address the existing issues.

I don't see the Mac stuff as a separate issue. I kept the original title someone else created for this bug, but in fixing it what I did was make keyboard navigation of Select elements work correctly, both on non-Macs and in Mac Chromium. (Basically I fixed the reported problem first, then noticed that the behavior on my Mac doesn't match the behavior of Safari, which I think is described by HI Guidelines, so I fixed that too.)

If you think I need to split that one line out as a separate bug I can do that, but it would just consume another couple man-hours of people's time for a separate submission and review; doesn't seem worth it to me. This whole thing is just a minor fix.

[Mark Rowe (bdash)]

(In reply to comment #14)
> New patch should address the existing issues.
> 
> I don't see the Mac stuff as a separate issue. I kept the original title
> someone else created for this bug, but in fixing it what I did was make
> keyboard navigation of Select elements work correctly, both on non-Macs and in
> Mac Chromium. (Basically I fixed the reported problem first, then noticed that
> the behavior on my Mac doesn't match the behavior of Safari, which I think is
> described by HI Guidelines, so I fixed that too.)
>
> If you think I need to split that one line out as a separate bug I can do that,
> but it would just consume another couple man-hours of people's time for a
> separate submission and review; doesn't seem worth it to me. This whole thing
> is just a minor fix.

It would take a matter of minutes to split it in to a separate patch, not hours.  If you're not willing to do that then that's fine, but you should retitle the bug to describe what it is you're actually fixing.

[Mark Rowe (bdash)]

Comment on attachment 32582 [details]
updated patch

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 45720)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,24 @@
> [phone]  Jens Alfke  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Bug 22784: Home/End and PageUp/PageDn buttons do not do anything in drop down lists,
> +		on non-Mac platforms (nor in the Mac build of Chromium.)
> +		https://bugs.webkit.org/show_bug.cgi?id=22784
> +		http://code.google.com/p/chromium/issues/detail?id=4576
> +
> +
> +        No new tests; affects only user input handling, not layout or rendering.
> +
> +        * dom/SelectElement.cpp:
> +		Changed definition of ARROW_KEYS_POP_MENU to make it true in Mac Chromium,
> +		so it will behave compatibly with Mac HI guidelines on pop-up menus.
> +		It's not possible to have PLATFORM(MAC) be true in the Mac build of Chromium.
> +        (WebCore::nextValidIndex): 
> +		New utility fn for traversing items of a select's list.
> +        (WebCore::SelectElement::menuListDefaultEventHandler):
> +		Added code to handle Home/End and PageUp/PageDn when ARROW_KEYS_POP_MENU is false.

There are lots of tabs in this ChangeLog entry.  Our SVN pre-commit hook will be unhappy with these, so it would be best to remove them.

> +#if !ARROW_KEYS_POP_MENU
> +// Returns the index of the next valid list item before or after |listIndex|.
> +static int nextValidIndex(int listIndex, bool forward, const Vector<Element*>& listItems)
> +{
> +    int skip = forward ? 1 : -1;

Is there a reason that you switched to a boolean argument rather than the enum that I suggested?  At the call site you don't have the context of the argument name to indicate what the true or false represent. An enum value is understandable at a glance, even at the call site.

The rest of the patch looks good.  It should be landable when these two minor issues are addressed.

[Jens Alfke]

Bugzilla won't let me retitle the bug; maybe because my account doesn't have the right privileges.

[Jens Alfke]

Created attachment 32590 [details]
new patch

[Jens Alfke]

Sorry about the tab characters! I'm on a new machine and hadn't yet fixed the tab settings in TextMate.

>Is there a reason that you switched to a boolean argument rather than the enum
>that I suggested?

Yes: because I felt that was a better change. IMHO defining a special enum for this one very local subroutine is overkill.

I've improved my change to add a second (inlined) function, so the calls now look like "nextValidIndex" and "previousValidIndex", without a direction parameter. Hopefully that should be clearer.

[Neil Rhodes]

Created attachment 32594 [details]
Proposed fix for bug 22784

It's not clear to me that Jens's patch actually handles paging up and down a page at a time.

In any case, this patch provides a test for the behavior of PageUp/PageDown/Home/End behavior for select elements.

[Jens Alfke]

It does handle page up/down. Note that this code runs only when the Select is _not_ popped up -- otherwise the platform's menu code is tracking the events.

Since only a single item is showing, the current 'page size' is 1, so I made page up/down jump by one element.

[Jens Alfke]

nrhodes' patch and mine are orthogonal -- they fix different issues. You should probably take both of them.

This made me notice that there's a disconnect between this bug's title and its description/test-case. The title (like Chromium bug 4576 which links to this one) refers to a 'drop down list', i.e. a pop-up menu. However, the description and test case here say that the problem is with list boxes. 

The original test case from 12/10/08 does include a pop-up, but incorrectly says it has no problems; actually on non-Mac platforms, or in Mac Chromium, the home/end/page keys do _not_ work when it's not popped up; that's what bug 4576 describes. (Whoever wrote the test case must only have tried it on a Mac, or only tried keystrokes while the menu was popped up?)

So in summary:
* My patch implements proper behavior of the home/end/page keys in a pop-up Select that has focus but is not currently popped up, on non-Mac platforms.
* My patch also gives Mac Chromium the Mac-like behavior of popping up the menu in response to the spacebar or home/end/page/arrow keys, as PLATFORM_MAC already does.
* nrhodes' patch implements proper key behavior in a _list-box_ Select.

(I noticed the bug was still unassigned, so I've taken the liberty of assigning it to myself.)

[Jens Alfke]

*ping* Any ETA for further review of the currently pending patches?

[David Levin]

It would be good if you had separate bugs with titles hat reflected what you said in the comments because it tends to get confusing with just a few comments (even when all the patches are coming from just one author).

[David Levin]

Comment on attachment 32590 [details]
new patch

It is best not to assign your patch to be reviewed by a particular person.  You'll get more attention usually if your patch is in the general queue.

There were a few things that Mark suggested which would allow you to get closer to getting this checked in faster:

1. If you had a patch and a bug just for the define change for ARROW_KEYS_POP_MENU.  I'd r+ it right now and it would be a small focused patch that didn't attempt to do more than one thing.  This is a good practice in general. As it makes reviews go quicker and keeping patches focused also helps with future maintainence when people look at the history and try to understand what patches were trying to accomplish, etc. so while it may be ten minutes more work for you. It is a good thing.

2. WebKit is adopting a new style guide (that isn't yet in the written guidelines) to avoid bool parameters to functions. This is a really good thing for several reasons: 
a. It keeps the code more readable (What does "true/false" mean when you see it at a calling site?)
b. It helps with future changes to avoid breakages.  (Once when we picked up a new revision of WebKit code in chromium things broke because a new bool parameter was inserted in the middle of other bool parameters.  Now clearly this doesn't seem like the best thing to do but if they had been enums the problem would have been quickly detected and corrected instead of taking hours to track down.)

While your function seems small and you may feel that it isn't worth it to add an enum for this function, it isn't hard, and it could help with future maintenance.



> +static int nextValidIndex(const Vector<Element*>& listItems, int listIndex, bool forward = true)

Change bool forward to an enum as noted above.

> +        if (keyIdentifier == "Down" || keyIdentifier == "Right" || keyIdentifier == "PageDown") {
> +            listIndex = nextValidIndex(listItems, listIndex);

Making "PageDown" go down only one item seems incorrect.

> +        } else if (keyIdentifier == "Up" || keyIdentifier == "Left" || keyIdentifier == "PageUp") {
> +            listIndex = previousValidIndex(listItems, listIndex);
> +            handled = true;

Making "PageUp" go up only one item seems incorrect.

[David Levin]

Comment on attachment 32594 [details]
Proposed fix for bug 22784


1. Please create a separate bug for your patch (or else this is going to get confusing fast).
2. This needs a changelog.  Run prepare-ChangeLog --bug=YourNewBugNumber.
3. It would be great if you used the functions that are being created in the other patch on this bug rather than have basically the same code duplicated here.
4. Please make comments look like sentences.  Start with a capital and end with a period.
5. It looks like you added several static functions to the class. It doesn't look like they need to be exposed on the class and could instead be static functions in the file. 

> Index: WebCore/dom/SelectElement.cpp

> +int SelectElement::selectableListIndexPageFrom(SelectElementData& data, Element* element, int startIndex, bool pageDown)

Avoid bool parameters. Use an enum instead. See my comment for the other patch on this bug.

> +{
> +    int pageSize = static_cast<RenderListBox*>(element->renderer())->size() - 1;  // -1 so we still show context

1 space before end of line comments.

> +    int indexOnePageAway = optionToListIndex(data, element, max(0, min(listSize -1, startIndex +  (pageDown ? pageSize : -pageSize))));

There is no space after "listSize -"
There are two spaces after "startIndex + "

> +    while (index >= 0 && index < listSize && (!isOptionElement(items[index]) || items[index]->disabled()))
> +        pageDown ? ++index : --index;

Ideally this would use the function from the other patch -- perhaps wait until after that patch is checked in.


> +    if (index < 0 || (unsigned) index == items.size()) {

Use c++ style casts.

> +        while (index >= 0 && index < listSize && (!isOptionElement(items[index]) || items[index]->disabled()))
> +            pageDown ? --index : ++index;

Ideally this would use the function from the other patch.


> +int SelectElement::firstSelectableListIndex(SelectElementData& data, Element* element)

Use the function from the other patch.

> +int SelectElement::lastSelectableListIndex(SelectElementData& data, Element* element)

Use the function from the other patch.

>              else if (keyIdentifier == "Up")
>                  endIndex = previousSelectableListIndex(data, element, optionToListIndex(data, element, selectedIndex(data, element)));
...
> +            else if (keyIdentifier == "PageUp")
> +                endIndex = selectableListIndexPageFrom(data, element, selectedIndex(data, element), false);

Why is this using selectedIndex(data, element) instead of what "Up" does: optionToListIndex(data, element, selectedIndex(data, element)) ?

> +        if (endIndex >= 0 && 
Boolean expressions at the same nesting level that span multiple lines should have their operators on the left side of the line instead of the right side.

So please move the && to the next line.

> +		        (keyIdentifier == "Down" || keyIdentifier == "Up" || keyIdentifier == "Home"
> +                ||  keyIdentifier == "End" || keyIdentifier == "PageDown" || keyIdentifier == "PageUp")) {

You have some TABs here which is why the indentation looks so bad.

[Jens Alfke]

I've split out the redefinition of ARROW_KEYS_POP_MENU into a new bug 27448 and provided a patch (which is just a subset of my previous patch here.)

I'll update the remaining part of my patch and attach that here in a few minutes.

** Neil, please file a new report for the bug that you fixed. After my patch for this bug has landed, merge your stuff with it (consider re-using or enhancing the static function I added) and submit the patch against that new bug. Thanks!

[Jens Alfke]

Created attachment 33096 [details]
Updated, more focused patch

This is an update of my prior patch for the home/end/pageup/pagedown behavior:
* Removes the redefinition of the space key behavior on Mac; that's broken into a separate bug/patch.
* Uses an enum instead of boolean for controlling the up/down mode of nextValidIndex().

Regarding the question of how far page up/down should scroll: I settled on one item, because since the list is not popped up at the time, the current 'page size' is 1.

[David Levin]

Comment on attachment 33096 [details]
Updated, more focused patch



> Index: WebCore/ChangeLog
> +        No new tests; affects only user input handling.

(Now that I understand this better) It seems that a layout test could be added for this functionality.  For example, see this test that simulates user input: LayoutTests/fast/forms/input-text-enter.html

> Index: WebCore/dom/SelectElement.cpp
> +enum SkipDirection {
> +    kSkipBackwards = -1,
> +    kSkipForwards = 1
> +};
> +

Enum members should user InterCaps with an initial capital letter.  (No leading "k". Also, just fyi, even for constants WebKit doesn't prefix them with "k".  They follow the standard naming convention.)

[Jens Alfke]

Created attachment 33224 [details]
Updated patch with test case

This revised patch adds a layout test. (Note that the test checks the non-Mac behavior, so it is expected to report failure on Mac; I've added a Mac-specific expected output file reflecting this.)
Page Up/Down now jump by three, as in Neil Rhodes' related patch.
It also fixes the enum naming.

[David Levin]

Comment on attachment 33224 [details]
Updated patch with test case

A few small changes and this will be done :)

> --- LayoutTests/fast/forms/select-popup-pagekeys-expected.txt	(revision 0)

> +This test verifies that the Home/End/PageUp/PageDown keys work correctly for pop-up <select> elements that have focus but are not currently popped-up.
> +

The tests typically note here what one should see if running it in the browser.  (Some thing like.  Below you should see some information about the key pressed and whether a test passed followed by a single "SUCCESS".)


The expected output here doesn't match what I would expect from the html. (It is missing "NOTE: This test will fail on Mac OS, on which these keys are NOT supposed to change the selection of a focused pop-up."


> \ No newline at end of file

Please add a newline (I guess fix the html file to ensure that there is one.)


> Property changes on: LayoutTests/fast/forms/select-popup-pagekeys-expected.txt
> ___________________________________________________________________
> Name: svn:eol-style:native

Please set to LF.


> Index: LayoutTests/fast/forms/select-popup-pagekeys.html
> \ No newline at end of file

Please add a newline.

> Index: LayoutTests/platform/mac/fast/forms/select-popup-pagekeys-expected.txt
> ___________________________________________________________________
> Name: svn:eol-style:native

Please set to LF.

[Jens Alfke]

Created attachment 33306 [details]
Final(?) patch

This patches addresses the above review comments. It's also been updated to WebKit r46229.

[David Levin]

Comment on attachment 33306 [details]
Final(?) patch

I think the conversion to LF was over aggressive (when it was done to existing files -- I didn't mean to suggest that.  Anyway, I'll take care of this all on landing).

[David Levin]

Assigned to levin for landing.

[David Levin]

While landing, I noticed a bug in the layout test:

passed = testPageDownNoDisabledElements();
  should be
passed = testPageDownNoDisabledElements() && passed;

(or "passed &&" could come first but then the first failure prevents anything else from running).

Anyway, I'll fix this while landing.

[David Levin]

Committed as http://trac.webkit.org/changeset/46259

[David Levin]

Follow up fix: http://trac.webkit.org/changeset/46261

[David Levin]

One more follow up fix http://trac.webkit.org/changeset/46262