[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


Naoki Takano <takano.naoki at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |takano.naoki at gmail.com




--- Comment #7 from Naoki Takano <takano.naoki at gmail.com>  2011-03-30 13:52:19 PST ---
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?

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