[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





--- Comment #22 from Naoki Takano <takano.naoki at gmail.com>  2011-04-01 15:44:13 PST ---
David,

Thank you for your support!!

(In reply to comment #21)
> (From update of attachment 87671 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=87671&action=review
> 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.)

Ok, I try to add more tests and also make sure PageDown/PageUp selecting is correct.

> > 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?

So do you mean like following?

int pageSize = 0;
if (element->renderer()->isListBox())
    pageSize = toRenderListBox(renderer)->size() - 1; // -1 so we still show context

> > 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)"

I will do as following,

int offset = direction == SkipForwards ? -1 : 1;
int index = nextValidIndex(items, indexOnePageAway + offset, direction, 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.

Yes, but when the default selected option is invalid, and all the items specified direction are invalid, we have to move the closest valid option backwards. That's the purpose.

I can write the test code.

> > 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?
If we don't call optionToListIndex() in nextSelectableListIndexPageAway(), it is Ok. This is my misunderstanding.

So,
1, I don't change the name |startIndex| as param of nextSelectableListIndexPageAway().
2, Replace optionToListIndex() to nextValidIndex().

What do you think?

> > 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.
I'll choose 1, like menuListDefaultEventHandler().

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