[Webkit-unassigned] [Bug 3248] Mouse events on OPTION element seem to be ignored

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=3248





------- Comment #11 from rwlbuis at gmail.com  2009-02-06 13:24 PDT -------
Hi Darin,

Thanks for the speedy review!

(In reply to comment #10)
> (From update of attachment 27357 [review])
> > +        * html/HTMLSelectElement.cpp:
> > +        (WebCore::HTMLSelectElement::listBoxDefaultEventHandler):
> > +        * rendering/RenderListBox.cpp:
> > +        (WebCore::RenderListBox::nodeAtPoint):
> > +        * rendering/RenderListBox.h:
>
>
> Per-function and per-file comments are preferred.

Done.

> > @@ -1743,6 +1760,7 @@
> >          (WebCore::RenderLayer::setHasCompositingDescendant):
> >          Maintain a bit to tell if this layer has composited descendants.
> >  
> > +
> >  2009-02-03  Simon Fraser  <simon.fraser at apple.com>
> 
> Why add a blank line here?

Git merge leftover, fixed.

> > +        // convert to coords relative to the list box if needed
> 
> Sentence-form comments with capital letters and periods are preferred.

Fixed.

> >          MouseEvent* mEvt = static_cast<MouseEvent*>(evt);
> 
> The name mouseEvent seems much better than "mEvt"; maybe you should change it
> while modifying this code.

Agreed, fixed.

> > +        Node* targ = evt->target()->toNode();
> 
> How about "target" rather than "targ"?

Agreed, fixed.

> > +        if (targ != this) {
> > +            FloatPoint absPos = renderer()->absoluteToLocal(FloatPoint(offsetX, offsetY), false, true);
> > +            offsetX = absPos.x();
> > +            offsetY = absPos.y();
> > +        }
> 
> Do we really need to optimize the "targ == this" case? Is there a downside to
> always calling absoluteToLocal?

This may be the most tricky part of the patch. It took me a lot of time to find
out that MouseRelatedEvent::receivedTarget does this translation when there is
a renderer. Ofcourse there
is no renderer when the target is HTMLOptionElement, so the conversion is not
done, therefore the
focus updating in default handler of HTMLSelectElement is not performed. I am
afraid that targ != this test is not restrictive enough. Also doing the
conversion twice for HTMLSelectElement is wrong, so there must be an if check
AFAICS. One option is converting first to Element, then using
hasLocalName(optiionElement) as the test. This sounds like a few more tests,
but may be the best possible check?

> > +bool RenderListBox::nodeAtPoint(const HitTestRequest& request, HitTestResult& result, int _x, int _y, int _tx, int _ty, HitTestAction hitTestAction)
> 
> Why the underscores in the argument names here? Just "x" and "y" would be
> better than "_x" and "_y". And it's bizarre to have both "_tx" and "tx" as
> names in the same function.

x is not used because it clashes with x(). Ofcourse this->x() can be used, not
sure
whether that is optimized away(I guess so) and/or it looks better. I removed
_tx, _ty for now.

> > +    bool ret = RenderBlock::nodeAtPoint( request, result, _x, _y, _tx, _ty, hitTestAction );
> > +    if ( !ret )
> > +        return false;
> 
> Extra spaces here. The name "ret" is too short. Maybe just put the nodeAtPoint
> call inside the if instead of including a local variable.

Agreed, fixed.

> > +    int size = numItems();
> 
> The right type for the size of a vector is size_t. Why are we calling
> numItems() rather than calling size() on the vector? I worry that we might walk
> off the end of the vector.

I just did that since it is done like that in this file. numItems calls size().
I can use size_t and
explicitly call size() if needed....

> > +    for (int listIndex = 0;listIndex < size;++listIndex) {
> 
> The rights size of a vector index is size_t. Need spaces after semicolons here.
> The name listIndex seems kind of long. How about just "i"? It's strange that
> for the index you chose a long name, but for the rectangle you chose "r".

Copy and pasted code, now I use i, fixed.

> > +            HTMLElement *node = listItems[listIndex];
> 
> The * needs to go next to the type name, not the variable name.

Fixed (left after copy and paste I think).

I'll post the patch now which fixes these problems in the code department. I
plan to extend the test so it also adds listeners to the <select> elements and
try to do some boundary hit tests, for instance testing what happens when a
part is clicked of the listbox that has no item. Anyway is free to review the
code changes or wait until the test is updated(which might need code changes),
I don't plan to make that take too long before the test is final.
Cheers,

Rob.


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