[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 #8 from David Levin <levin at chromium.org>  2011-03-30 14:14:28 PST ---
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.)

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