[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 #23 from David Levin <levin at chromium.org>  2011-04-01 17:58:03 PST ---
(From update of attachment 87671)
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.

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