[Webkit-unassigned] [Bug 22784] [Chromium] Home and End buttons do not do anything in drop down lists

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=22784


Mark Rowe (bdash) <mrowe at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #32582|review?(mrowe at apple.com)    |review-
               Flag|                            |




--- Comment #16 from Mark Rowe (bdash) <mrowe at apple.com>  2009-07-10 15:43:37 PDT ---
(From update of attachment 32582)
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 45720)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,24 @@
> +2009-07-10  Jens Alfke  <snej at google.com>
> +
> +        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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.