[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 #87671|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #21 from David Levin <levin at chromium.org>  2011-04-01 14:43:05 PST ---
(From update of attachment 87671)
View in context: https://bugs.webkit.org/attachment.cgi?id=87671&action=review

There are a few issues I mentioned above which would be nice to fix up.

Also if you fix any bugs, please consider increasing the test coverage to have caught them. (For example, I think for the PageDown case, the value passed into nextSelectableListIndexPageAway is incorrect. Also it feels like PageDown resulting in selecting an item above the currently selected item is wrong. Ditto for PageUp selecting an item above the current item.)

> Source/WebCore/dom/SelectElement.cpp:78
> +// Returns the closest valid index far |skip| items from |listIndex| in direction |direction|.

How about:

// Returns the 1st valid item |skip| items from |listIndex| in direction |direction| if there is one.
// Otherwise, it returns the valid item closest to that boundary which is past |listIndex| if there is one.
// Otherwise, it returns |listIndex|.
// Valid means that it is enabled and an option element.

> Source/WebCore/dom/SelectElement.cpp:81
> +{

ASSERT(direction == -1 || direction == 1);

> Source/WebCore/dom/SelectElement.cpp:96
> +static int nextSelectableListIndexPageAway(SelectElementData& data, Element* element, int startIndex, SkipDirection direction)

I think startIndex should be startOptionIndex to clarify that this startIndex isn't the same as the other start indexes.

> Source/WebCore/dom/SelectElement.cpp:97
> +{

ASSERT(direction == -1 || direction == 1);

> Source/WebCore/dom/SelectElement.cpp:100
> +    int pageSize = static_cast<RenderListBox*>(element->renderer())->size() - 1; // -1 so we still show context

Use toRenderListBox instead of the cast.

How do you know that the renderer is a RenderListBox?

> Source/WebCore/dom/SelectElement.cpp:104
> +    int indexOnePageAway = SelectElement::optionToListIndex(data, element, max(0, min(listSize - 1, startIndex + direction * pageSize)));

Why is this offset going through this function as opposed to converting the startIndex to a list index and then just doing this:
   return nextValidIndex(items, startListIndex, direction, pageSize);

> Source/WebCore/dom/SelectElement.cpp:107
> +    // It means we try to find the index in (startIndex + page * direction, direction == SkipForward ? MAXINT : 0].

s/page/pageSize/

> Source/WebCore/dom/SelectElement.cpp:108
> +    int index = nextValidIndex(items, indexOnePageAway + (direction == SkipForwards ? -1 : 1), direction, 1);

It seems simpler to not do this "(direction == SkipForwards ? -1 : 1)"

> Source/WebCore/dom/SelectElement.cpp:112
> +        index = nextValidIndex(items, indexOnePageAway, direction == SkipForwards ? SkipBackwards : SkipForwards, 1);

This could actually end up being before startIndex which seems weird.

> Source/WebCore/dom/SelectElement.cpp:161
> +    const Vector<Element*>& items = data.listItems(element);

Consider:

int index = nextValidIndex(items, items.size(), SkipBackwards, MAXINT);
if (static_cast<unsigned>(index) == items.size())
  return -1;
return index;

seems simpler and doesn't repeat logic. (slightly less efficient but this won't be used that often and probably not for a list long enough where the efficiency will matter).

> Source/WebCore/dom/SelectElement.cpp:172
> +    const Vector<Element*>& items = data.listItems(element);

return nextValidIndex(items, -1, SkipForwards, MAXINT);

as before.

> Source/WebCore/dom/SelectElement.cpp:174
> +     while (index >= 0 && static_cast<unsigned>(index) < items.size() && (!isOptionElement(items[index]) || items[index]->disabled()))

indenting is messed up here.

> Source/WebCore/dom/SelectElement.cpp:183
>          startIndex = items.size();

Consider rewriting in terms of nextValidIndex

> Source/WebCore/dom/SelectElement.cpp:811
> +                endIndex = nextSelectableListIndexPageAway(data, element, lastSelectedListIndex(data, element), SkipForwards);

Is it correct to pass in lastSelectedListIndex for startOptionIndex given that the function is expect an option index?

> Source/WebCore/dom/SelectElement.cpp:835
> +        if (endIndex >= 0 && (keyIdentifier == "Down" || keyIdentifier == "Up" || keyIdentifier == "Home" ||  keyIdentifier == "End" || keyIdentifier == "PageDown" || keyIdentifier == "PageUp")) {

Hmmm this if got a bit ugly and seems fragile.

Consider (I'm not sure if these are better or not or how easy they are to do so use your judgement):
1. use a bool handled to indicate that the endIndex should be set. I think there is another function in this file that does that. OR
2. can endIndex have a value of -1 and only get set to >= 0 if it should be used in here.

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