[Webkit-unassigned] [Bug 27658] Webkit ignores PgUp/PgDown/Home/End in SELECT tag objects

[bugzilla-daemon at webkit.org]

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


David Levin <levin at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #87998|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #26 from David Levin <levin at chromium.org>  2011-04-04 11:47:06 PST ---
(From update of attachment 87998)
View in context: https://bugs.webkit.org/attachment.cgi?id=87998&action=review

Nicely done!

We're in the final stretch and the next patch may very well be the last.

Thanks for getting this into shape and addressing all feedback well!

> LayoutTests/fast/events/select-element.html:165
> +function testPageDownWithDisabledElements() {

Why is there no test in this function start starts at 0 or 7?

> LayoutTests/fast/events/select-element.html:173
> +function testPageUpWithDisabledElements() {

Why is there no test in this function start starts at 0 or 7?

> Source/WebCore/dom/SelectElement.cpp:134
> +    int edgeIndex = (direction== SkipForwards) ? 0 : (items.size() - 1);

Need a space after direction.

> Source/WebCore/dom/SelectElement.cpp:789
>                  endIndex = nextSelectableListIndex(data, element, lastSelectedListIndex(data, element));

For down, the start index is lastSelectedListIndex(data, element)
For up, the start index is optionToListIndex(data, element, selectedIndex(data, element))

Consider pulling this out of the clauses:

if (keyIdentifier == "Down" || keyIdentifier == "PageDown") {
    int startIndex = lastSelectedListIndex(data, element)
    handled = true;
    if (keyIdentifier == "Down")
        endIndex = nextSelectableListIndex(data, element, startIndex);
    else
        endIndex = nextSelectableListIndexPageAway(data, element, startIndex, SkipForwards);    
} else if (keyIdentifier == "Up" || keyIdentifier == "PageUp")  {
    int startIndex = optionToListIndex(data, element, selectedIndex(data, element))
    handled = true;
    ... similar to above...
}

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