Bug 27658 – Webkit ignores PgUp/PgDown/Home/End in SELECT tag objects

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Neil Rhodes]

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.

[Neil Rhodes]

I'm working on a patch for this bug.

[Neil Rhodes]

Created attachment 33476 [details]
Fixes bug 27658

[Neil Rhodes]

Created attachment 34840 [details]
badselect.html

[David Levin]

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.

[Blake Sening]

Is this still a problem in WebKit?

It seems in the latest nightlies SELECT tags do respond to PgUp/PgDown/Home/End.

[Blake Sening]

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

[Naoki Takano]

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?

[David Levin]

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

[Naoki Takano]

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,

[Naoki Takano]

Created attachment 87671 [details]
Patch

[Naoki Takano]

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,

[Alexey Proskuryakov]

It might be useful to add a test with (possibly disabled) option groups. These are tricky.

[Naoki Takano]

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,

[Alexey Proskuryakov]

I'm asking about <optgroup> elements (which can also be enabled or not).

[Naoki Takano]

Got it.

Thanks!!

(In reply to comment #14)
> I'm asking about <optgroup> elements (which can also be enabled or not).

[Naoki Takano]

David,

Do you have any comment about cpp part?

Thanks,

[Naoki Takano]

David,

Could you review my patch?

Or is there anybody who can review?

Thanks,

[Alexey Proskuryakov]

Could you add optgroup tests while waiting for review?

[Naoki Takano]

Sure,

(In reply to comment #18)
> Could you add optgroup tests while waiting for review?

[David Levin]

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

[David Levin]

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.

[Naoki Takano]

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

[David Levin]

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.

[Naoki Takano]

Created attachment 87998 [details]
Patch

[Naoki Takano]

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,

[David Levin]

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

[Naoki Takano]

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

[Naoki Takano]

Created attachment 88180 [details]
Patch

[Naoki Takano]

Comment on attachment 88180 [details]
Patch

Please review.

[David Levin]

Comment on attachment 88180 [details]
Patch

Great! Thanks.

[WebKit Commit Bot]

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

[David Levin]

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

[Naoki Takano]

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.

[Naoki Takano]

Created attachment 88355 [details]
Patch

[Naoki Takano]

Comment on attachment 88355 [details]
Patch

Commit again please.

[David Levin]

Comment on attachment 88355 [details]
Patch

This patch is missing lot (like the tests).

[Naoki Takano]

Sorry... I try again.

(In reply to comment #36)
> (From update of attachment 88355 [details])
> This patch is missing lot (like the tests).

[Naoki Takano]

Created attachment 88359 [details]
Patch

[Naoki Takano]

Comment on attachment 88359 [details]
Patch

Please review again.

[WebKit Commit Bot]

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

[Naoki Takano]

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

[Naoki Takano]

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

[Naoki Takano]

Created attachment 88472 [details]
Patch

[Naoki Takano]

Comment on attachment 88472 [details]
Patch

Fixed test failure problem.

Please review again.

[David Levin]

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.

[WebKit Commit Bot]

Comment on attachment 88472 [details]
Patch

Clearing flags on attachment: 88472

Committed r83096: <http://trac.webkit.org/changeset/83096>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Naoki Takano]

David,

Thank you for your support.

Finally I land it;-)

Thanks,

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