Bug 27658

Summary: Webkit ignores PgUp/PgDown/Home/End in SELECT tag objects
Product: WebKit Reporter: Neil Rhodes <nrhodes>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, bksening, commit-queue, honten, isherman, levin, tkent
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Fixes bug 27658
levin: review-
badselect.html
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Neil Rhodes 2009-07-24 09:09:51 PDT
Overview: A SELECT object displaying a list (non-popup) ignores the PgUp/PgDown/Home/End keys. Instead the keys scroll the page.

Steps to Reproduce:

    1) Open badselect.html (attached).

    2) Click on the "General" list item.

    3) Press one of Page Up/Page Down/Home/End

Actual Results: 

    If the window is small enough, the window scrolls.

Expected Results: 

    The Page Up/PageDown/Home/End keys should select "Build config"/"Bookmarks & History"/"Location Bar and Autocomplete"/"Toolbar" respectively.


This is has been forked from:
   Bug 22784: [Chromium] Home and End buttons do not do anything in drop down lists
   https://bugs.webkit.org/show_bug.cgi?id=22784
because that was actually demonstrating two bugs, of which this was one.
Comment 1 Neil Rhodes 2009-07-24 09:10:15 PDT
I'm working on a patch for this bug.
Comment 2 Neil Rhodes 2009-07-24 16:22:36 PDT
Created attachment 33476 [details]
Fixes bug 27658
Comment 3 Neil Rhodes 2009-08-14 07:34:19 PDT
Created attachment 34840 [details]
badselect.html
Comment 4 David Levin 2009-08-24 10:33:09 PDT
Comment on attachment 33476 [details]
Fixes bug 27658


> 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.
Comment 5 Blake Sening 2010-10-08 05:31:16 PDT
Is this still a problem in WebKit?

It seems in the latest nightlies SELECT tags do respond to PgUp/PgDown/Home/End.
Comment 6 Blake Sening 2010-10-08 09:52:55 PDT
Sorry, it still happens with the included reduction "badselect.html".

Rather, the title of the bug needs to be appended with "... when SIZE attribute is specified".
Comment 7 Naoki Takano 2011-03-30 13:52:19 PDT
Hi, this problem is not taken care of by anybody. So I want to try to fix it.

I have a couple of questions, before I start.

(In reply to comment #4)
> (From update of attachment 33476 [details])
> 
> > 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.
Do you mean the function should return more than or equal to listIndex + skip value?

> > +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.

I think this function operation means
1, Try to find (startIndex + pageSize * direction, direction  == SkipForwards ? MAXINT : 0]
2, And if we cannot find valid index, we have to find (startIndex, startIndex + pageSize * direction]

I guess your saying is opposite, right?

> > +    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.
If my former guess is right, we don't have to need the range (startIndex, startIndex + pageSize * direction] check here, right?
Comment 8 David Levin 2011-03-30 14:14:28 PDT
I'm answering base on just looking at my comments. the context has totally left me by now.

(In reply to comment #7)
> Hi, this problem is not taken care of by anybody. So I want to try to fix it.
> 
> I have a couple of questions, before I start.
> 
> (In reply to comment #4)
> > (From update of attachment 33476 [details] [details])
> > 
> > > 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.
> Do you mean the function should return more than or equal to listIndex + skip value?

I was simply saying that the comment was incorrect.  The comment said that it returned an index that was at least listIndex + skip and that wasn't correct.



> 
> > > +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.
> 
> I think this function operation means
> 1, Try to find (startIndex + pageSize * direction, direction  == SkipForwards ? MAXINT : 0]
> 2, And if we cannot find valid index, we have to find (startIndex, startIndex + pageSize * direction]
> 
> I guess your saying is opposite, right?

It looks like I was saying the opposite. I may have been wrong.


> 
> > > +    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.
> If my former guess is right, we don't have to need the range (startIndex, startIndex + pageSize * direction] check here, right?

I don't remember. If there is a new patch, I can try to review it and then figure all of this out again. (This isn't an area I deal with in general, so I don't have the context for all of this in my memory. I have to read lots of code and figure it out.)
Comment 9 Naoki Takano 2011-03-30 14:27:50 PDT
Thank you for your reply.

(In reply to comment #8)
> I was simply saying that the comment was incorrect.  The comment said that it returned an index that was at least listIndex + skip and that wasn't correct.

Ok, so I will revise the comment.

> I don't remember. If there is a new patch, I can try to review it and then figure all of this out again. (This isn't an area I deal with in general, so I don't have the context for all of this in my memory. I have to read lots of code and figure it out.)

Once I'm ready for the patch, I let you know.

Thanks,
Comment 10 Naoki Takano 2011-03-30 23:45:13 PDT
Created attachment 87671 [details]
Patch
Comment 11 Naoki Takano 2011-03-30 23:46:54 PDT
Comment on attachment 87671 [details]
Patch

Basically, the logic is the same as the original.

But I changed some format problems and applied to the latest source code.

Also I change the test, because it didn't work.

Please review it.

Thanks,
Comment 12 Alexey Proskuryakov 2011-03-31 12:19:06 PDT
It might be useful to add a test with (possibly disabled) option groups. These are tricky.
Comment 13 Naoki Takano 2011-03-31 17:23:02 PDT
Alexey,

(In reply to comment #12)
> It might be useful to add a test with (possibly disabled) option groups. These are tricky.
Thank you for your comment. But I cannot understand what you mean,  (possibly disabled) option groups.

I thought <option> tag has "disabled" attribute, but don't know "possible disabled".

Could you tell me more?

Thanks,
Comment 14 Alexey Proskuryakov 2011-03-31 17:29:41 PDT
I'm asking about <optgroup> elements (which can also be enabled or not).
Comment 15 Naoki Takano 2011-03-31 17:37:49 PDT
Got it.

Thanks!!

(In reply to comment #14)
> I'm asking about <optgroup> elements (which can also be enabled or not).
Comment 16 Naoki Takano 2011-03-31 18:57:09 PDT
David,

Do you have any comment about cpp part?

Thanks,
Comment 17 Naoki Takano 2011-04-01 12:57:50 PDT
David,

Could you review my patch?

Or is there anybody who can review?

Thanks,
Comment 18 Alexey Proskuryakov 2011-04-01 13:07:25 PDT
Could you add optgroup tests while waiting for review?
Comment 19 Naoki Takano 2011-04-01 13:22:44 PDT
Sure,

(In reply to comment #18)
> Could you add optgroup tests while waiting for review?
Comment 20 David Levin 2011-04-01 13:38:17 PDT
(In reply to comment #17)
> David,
> 
> Could you review my patch?
> 
> Or is there anybody who can review?
> 
> Thanks,

I've started looking (and a have some misc comments but nothing huge). It takes me longer being an area I'm not as familiar with. If someone else beats me to it, that is fine.
Comment 21 David Levin 2011-04-01 14:43:05 PDT
Comment on attachment 87671 [details]
Patch

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.
Comment 22 Naoki Takano 2011-04-01 15:44:13 PDT
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().
Comment 23 David Levin 2011-04-01 17:58:03 PDT
Comment on attachment 87671 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=87671&action=review

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

Well, that is one way of solving it. If you really have some way of knowing that the renderer will always be a listbox (and it may be), then just say how you know this. (I wasn't able to figure that out right off but it may be my lack of familiarity with this area of code.)

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

ok, just making sure that was the purpose.

But if that is the case, can't it be written in terms of one nextValidIndex call and get right of a lot of code in here?

For example,  something like this:
   int edgeIndex = (direction == SkipForward) ? 0 : (listSize - 1);
   int skipAmount = pageSize + (direction == SkipForward) ? startListIndex : (edgeIndex - startListIndex);
   return nextValidIndex(items, edgeIndex, direction, skipAmount);

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

> If we don't call optionToListIndex() in nextSelectableListIndexPageAway(), it is Ok.
Is it?

From how, I read optionToListIndex, it looks like the option index is very different from the list index. The option index only counts the number of option items (http://www.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/dom/SelectElement.cpp&q=optionToListIndex&exact_package=chromium&l=362) which is different from the number of items in the list.
Comment 24 Naoki Takano 2011-04-02 23:33:30 PDT
Created attachment 87998 [details]
Patch
Comment 25 Naoki Takano 2011-04-02 23:35:10 PDT
Comment on attachment 87998 [details]
Patch

Please review my patch.

Added optgroup test.
Fixed some bugs related to passing params for nextSelectableListIndexPageAway() which affects optgroup behavior.

Thanks,
Comment 26 David Levin 2011-04-04 11:47:06 PDT
Comment on attachment 87998 [details]
Patch

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...
}
Comment 27 Naoki Takano 2011-04-04 13:57:17 PDT
David,

Thank you for your review.

I can finish the revise tonight.

Hope the next is the last round;-)

(In reply to comment #26)
> (From update of attachment 87998 [details])
> 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...
> }
Comment 28 Naoki Takano 2011-04-04 18:51:08 PDT
Created attachment 88180 [details]
Patch
Comment 29 Naoki Takano 2011-04-04 18:51:37 PDT
Comment on attachment 88180 [details]
Patch

Please review.
Comment 30 David Levin 2011-04-04 21:29:42 PDT
Comment on attachment 88180 [details]
Patch

Great! Thanks.
Comment 31 WebKit Commit Bot 2011-04-05 02:17:39 PDT
Comment on attachment 88180 [details]
Patch

Rejecting attachment 88180 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'bu..." exit_code: 2

Last 500 characters of output:
gkcp/WebCorePrefix.h -c /Projects/CommitQueue/Source/WebCore/rendering/RenderListBox.cpp -o /Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/RenderListBox.o

** BUILD FAILED **


The following build commands failed:
WebCore:
	CompileC /Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/SelectElement.o /Projects/CommitQueue/Source/WebCore/dom/SelectElement.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
(1 failure)


Full output: http://queues.webkit.org/results/8335252
Comment 32 David Levin 2011-04-05 07:04:31 PDT
/Projects/CommitQueue/Source/WebCore/dom/SelectElement.cpp:98: warning: no previous prototype for 'int WebCore::nextSelectableListIndex(WebCore::SelectElementData&, WebCore::Element*, int)'
/Projects/CommitQueue/Source/WebCore/dom/SelectElement.cpp:103: warning: no previous prototype for 'int WebCore::previousSelectableListIndex(WebCore::SelectElementData&, WebCore::Element*, int)'
/Projects/CommitQueue/Source/WebCore/dom/SelectElement.cpp:108: warning: no previous prototype for 'int WebCore::firstSelectableListIndex(WebCore::SelectElementData&, WebCore::Element*)'
/Projects/CommitQueue/Source/WebCore/dom/SelectElement.cpp:117: warning: no previous prototype for 'int WebCore::lastSelectableListIndex(WebCore::SelectElementData&, WebCore::Element*)'

You need to prefix these functions with static.
Comment 33 Naoki Takano 2011-04-05 09:01:59 PDT
Yeah, I forgot to add static when I move the functions from private...

I'll do that.

(In reply to comment #32)
> /Projects/CommitQueue/Source/WebCore/dom/SelectElement.cpp:98: warning: no previous prototype for 'int WebCore::nextSelectableListIndex(WebCore::SelectElementData&, WebCore::Element*, int)'
> /Projects/CommitQueue/Source/WebCore/dom/SelectElement.cpp:103: warning: no previous prototype for 'int WebCore::previousSelectableListIndex(WebCore::SelectElementData&, WebCore::Element*, int)'
> /Projects/CommitQueue/Source/WebCore/dom/SelectElement.cpp:108: warning: no previous prototype for 'int WebCore::firstSelectableListIndex(WebCore::SelectElementData&, WebCore::Element*)'
> /Projects/CommitQueue/Source/WebCore/dom/SelectElement.cpp:117: warning: no previous prototype for 'int WebCore::lastSelectableListIndex(WebCore::SelectElementData&, WebCore::Element*)'
> 
> You need to prefix these functions with static.
Comment 34 Naoki Takano 2011-04-05 19:52:23 PDT
Created attachment 88355 [details]
Patch
Comment 35 Naoki Takano 2011-04-05 19:52:44 PDT
Comment on attachment 88355 [details]
Patch

Commit again please.
Comment 36 David Levin 2011-04-05 20:10:17 PDT
Comment on attachment 88355 [details]
Patch

This patch is missing lot (like the tests).
Comment 37 Naoki Takano 2011-04-05 20:47:21 PDT
Sorry... I try again.

(In reply to comment #36)
> (From update of attachment 88355 [details])
> This patch is missing lot (like the tests).
Comment 38 Naoki Takano 2011-04-05 20:50:21 PDT
Created attachment 88359 [details]
Patch
Comment 39 Naoki Takano 2011-04-05 20:50:59 PDT
Comment on attachment 88359 [details]
Patch

Please review again.
Comment 40 WebKit Commit Bot 2011-04-05 23:07:07 PDT
Comment on attachment 88359 [details]
Patch

Rejecting attachment 88359 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'bu..." exit_code: 2

Last 500 characters of output:
........................................................................................................................................................................................................................................................................
fast/forms/listbox-deselect-scroll.html -> failed

Exiting early after 1 failures. 8647 tests run.
207.49s total testing time

8646 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
4 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/8345059
Comment 41 Naoki Takano 2011-04-05 23:09:50 PDT
It seems like failed...

I look into it.

(In reply to comment #40)
> (From update of attachment 88359 [details])
> Rejecting attachment 88359 [details] from commit-queue.
> 
> Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'bu..." exit_code: 2
> 
> Last 500 characters of output:
> ........................................................................................................................................................................................................................................................................
> fast/forms/listbox-deselect-scroll.html -> failed
> 
> Exiting early after 1 failures. 8647 tests run.
> 207.49s total testing time
> 
> 8646 test cases (99%) succeeded
> 1 test case (<1%) had incorrect layout
> 4 test cases (<1%) had stderr output
> 
> Full output: http://queues.webkit.org/results/8345059
Comment 42 Naoki Takano 2011-04-06 09:02:47 PDT
Interestingly, 'SelectAll' command in DumpRenderTree doesn't work well. Manual select all works fine.

The problem is in previousSelectableListIndex and nextSelectableListIndex.

Still I'm looking into it...
Comment 43 Naoki Takano 2011-04-06 11:39:41 PDT
Created attachment 88472 [details]
Patch
Comment 44 Naoki Takano 2011-04-06 11:40:20 PDT
Comment on attachment 88472 [details]
Patch

Fixed test failure problem.

Please review again.
Comment 45 David Levin 2011-04-06 12:19:24 PDT
Comment on attachment 88472 [details]
Patch

So this was the fix:

 103static int previousSelectableListIndex(SelectElementData& data, Element* element, int startIndex)
 104{
 105    if (startIndex == -1)
 106        startIndex = data.listItems(element).size();

OK, that mimics what was there before.
Comment 46 WebKit Commit Bot 2011-04-06 13:52:36 PDT
Comment on attachment 88472 [details]
Patch

Clearing flags on attachment: 88472

Committed r83096: <http://trac.webkit.org/changeset/83096>
Comment 47 WebKit Commit Bot 2011-04-06 13:52:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 48 Naoki Takano 2011-04-06 15:27:30 PDT
David,

Thank you for your support.

Finally I land it;-)

Thanks,

(In reply to comment #47)
> All reviewed patches have been landed.  Closing bug.