[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 #33476|review?                     |review-
               Flag|                            |




--- Comment #4 from David Levin <levin at chromium.org>  2009-08-24 10:33:09 PDT ---
(From update of attachment 33476)

> WebCore/dom/SelectElement.cpp	(working copy)
> ===================================================================
> +// Returns the index of the next valid list item |skip| items past |listIndex| in direction |direction|.
This isn't really correct as it could return listIndex or listIndex + 1, etc.
even if skip = 10.


> +// returns the index of the next valid item one page away from |startIndex| in direction |direction|.

(Many instances) Make comments looks like sentences. (Start with a capital and
end with a period.)


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

I can't make sense of this function name.  It seems like it should stat with
"next".  Perhaps "nextSelectableListIndexPageAway".

Also, after having read through this function it is unclear what it is trying
to do and if it is correct.  I think it attempts to find a "valid index" that
is in this range (startIndex, startIndex + pageSize * direction] but if that
fails, it tries to find a valid index (startIndex + pageSize * direction,
direction  == SkipForwards ? MAXINT : 0], so that's what I'll use as my guide.


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

1 space before end of line comments.

> + int index = nextValidIndex(items, indexOnePageAway + oppositeDirection, direction, 1);

Relying on the enum value to be -1, +1 seems like a bad practice here. I know
that "nextValidIndex" does but ideally it is only relied on there.


> +    if (index < 0 || index >= listSize ||(!isOptionElement(items[index]) || items[index]->disabled())) {

Add a space after || here: "||(!isOptionElement".
Unnecessary () around the last two items.

At a higher level, this isn't correct because "index" may be valid but not in
this range (startIndex, startIndex + pageSize * direction].

Add a test which exposes this issue.


> +int SelectElement::firstSelectableListIndex(SelectElementData& data, Element* element)
> +{
> +    const Vector<Element*>& items = data.listItems(element);
> +    int index = 0;
> +    while (index >= 0 && (unsigned) index < items.size() && (!isOptionElement(items[index]) || items[index]->disabled()))

(Multiple places) Use c++ style casts.


>  void SelectElement::menuListDefaultEventHandler(SelectElementData& data, Element* element, Event* event, HTMLFormElement* htmlForm)
>  {
...
> +        if (endIndex >= 0 && 

Please move the && to the next line.

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

The || should be aligned with the ( from the previous line.


> Index: WebCore/dom/SelectElement.h
> ===================================================================
> +    static int selectableListIndexPageFrom(SelectElementData& data, Element* element, int startIndex, SkipDirection direction);

Can this be moved out of the class and solely be defined within the
SelectElement.cpp file?



> Index: LayoutTests/fast/events/select-element.html
> ===================================================================
> +    function log(s)

Ideally use whole words for variables: "s"


> +    function sendKeyAndExpectIndex(selectId, key, initialIndex, expectedIndex) {
...
> +        if (select.selectedIndex != initialIndex) {
> +            log("can't set selectedIndex to " + initialIndex + ' (is '
> +            + select.selectedIndex + ')');

Would be nice to align with (

> +            return false;
> +        }
> +        if (window.layoutTestController)
> +            eventSender.keyDown(key);
> +        if (select.selectedIndex != expectedIndex) {
> +            log('selectedIndex should be ' + expectedIndex + ' (is ' +

In keeping with the moving the &&, etc to the beginning of the next line, move
the + to the next line would be nice.

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