Mouse events on OPTION element seem to be ignored

• Component: Forms
• Status: RESOLVED FIXED

[Dave Hyatt]

* SUMMARY
With Safari 2.0 or 1.2, Mouse events such as ommouseup, omousedown, onclick, and ondblclick seem 
to be ignored with OPTION element

* STEPS TO REPRODUCE
1. Open the attached test case called "option_onmouseup.html" in Safari 2.0 (412)
2. Click on the <option> items called 'kiwi'. Verify that no alert dialog is displayed after a mouse event.
3. Open this same test case in Firefox 1.0.3. Notice that a alert dialog is displayed for each event.


* REGRESSION
No, this problem also occurs in Safari 1.2

[Dave Hyatt]

Apple Bug: rdar://4099606/

[Stuart Morgan]

Created attachment 2254 [details]
test case

The test case mentioned above.

[Eric Seidel (no email)]

This should be a pretty simple fix.  And we might actually get this for free when hyatt/beth move this form 
control off of AppKit and onto our own engine.

However someone could also probably make this work with the existing form controls with under an hour 
worth of effort.

[Alexey Proskuryakov]

*** Bug 4341 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

*** Bug 19782 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

Wow.  This still doesn't work in Safari 3.1.  Huh.

[Sriram]

Created attachment 24856 [details]
Patch to fix the firing of mouse down

This patch only fixes the firing of the mouse down event.  Is this the correct place to fix this problem?  The issue in this bug is that the hit test result only returns the HTMLSelectElement as the node under mouse.  Another way to fix it would be to implement nodeAtPoint for both HTMLSelectElement and HTMLOptionElement.

[Adele Peterson]

Comment on attachment 24856 [details]
Patch to fix the firing of mouse down

I don't think this is the right approach- but I'm not sure the nodeatpoint approach is right either.  The tricky thing is that the option elements don't have renderers, so real hit testing will never happen.  Maybe you can implement htmlselectelement::nodeatpoint and fake the option hittesting

[Rob Buis]

Created attachment 27357 [details]
First attempt

This patch implements mouse events for option elements in list boxes.
Cheers,

Rob.

[Darin Adler]

Comment on attachment 27357 [details]
First attempt

> +        * html/HTMLSelectElement.cpp:
> +        (WebCore::HTMLSelectElement::listBoxDefaultEventHandler):
> +        * rendering/RenderListBox.cpp:
> +        (WebCore::RenderListBox::nodeAtPoint):
> +        * rendering/RenderListBox.h:

Per-function and per-file comments are preferred.

> @@ -1743,6 +1760,7 @@
>          (WebCore::RenderLayer::setHasCompositingDescendant):
>          Maintain a bit to tell if this layer has composited descendants.
>  
> +
>  2009-02-03  Simon Fraser  <[e-mail]>

Why add a blank line here?

> +        // convert to coords relative to the list box if needed

Sentence-form comments with capital letters and periods are preferred.

>          MouseEvent* mEvt = static_cast<MouseEvent*>(evt);

The name mouseEvent seems much better than "mEvt"; maybe you should change it while modifying this code.

> +        Node* targ = evt->target()->toNode();

How about "target" rather than "targ"?

> +        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?

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

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

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

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

> +            HTMLElement *node = listItems[listIndex];

The * needs to go next to the type name, not the variable name.

Enough things need fixing that I'll say review-.

[Rob Buis]

Hi Darin,

Thanks for the speedy review!

(In reply to comment #10)
> (From update of attachment 27357 [details] [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  <[e-mail]>
> 
> 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.

[Rob Buis]

Created attachment 27416 [details]
Code fixes

Fixes for the code review comments. Test is unchanged and will be improved in a follow-up patch, which I plan to post somewhen this weekend, but apart from that the patch should be reviewable, just maybe not in a landeable state because of the improvements the test needs to get first.
Cheers,

Rob.

[Rob Buis]

Created attachment 27444 [details]
Improved testcase

The testcase now also tests that the parent select event listeners are processed and verifies the event target. The last test tests that clicking in the list box below the final item does not deliver the event to the option element and that the event target is set to the select.
I now consider the patch and test in a final state.
Cheers,

Rob.

[Darin Adler]

Comment on attachment 27444 [details]
Improved testcase

I still think that size_t and calling size() on listItems directly would be slightly better in RenderListBox::nodeAtPoint. I also would not bother with a local variable for the result of itemBoundingBoxRect and I would have put the declaration of "node" inside the if statement, and called it element rather than node. But these are tiny nits. Good new test!

I also think it's unfortunate that we use offsetX/Y at all, since they have a crazy definition. I bet we could make the code easier to understand if we used pageX/Y instead some day. For now, fix is great, r=me!

r=me

[Brent Fulgham]

Committed in http://trac.webkit.org/changeset/40763

[Marc-André Decoste]

Created attachment 27675 [details]
A proposal to modify the LayoutTest that has been created to validate the fix of this bug.

I'm not sure if this is the appropriate procedure to reopen the bug or if I should open a new one instead... Please warn me if it is the latter...

Thanks!

BYE
MAD

[Marc-André Decoste]

Is it OK to just add a new patch attachment to propose changes for the layout test validating this fix, or should I create a new bug?

The reason I propose this change is that the layout test fails on Chrome (and also WebKit on Windows) because the list boxes don't render with the exact same size as on the Mac. So I added absolute positions for the select elements and adapted the test coordinates accordingly... Hope it's OK...

[Alexey Proskuryakov]

Comment on attachment 27675 [details]
A proposal to modify the LayoutTest that has been created to validate the fix of this bug.

Please file a new bug. I considered just giving a review and checking this in, but got confused by your statement about this test failing on Windows. That doesn't seem to be confirmed by build.webkit.org, so I now wonder if it's even OK for ports to change the size of list boxes - someone more familiar with rendering will need to weigh in.

[phone]  mad  <[e-mail]>

This is not a strict formal requirement, but most contributors use full names in ChangeLogs. Also, please add a bug URL to ChangeLog.

[Marc-André Decoste]

OK, created https://bugs.webkit.org/show_bug.cgi?id=24006 to fix the Layout Test.

Thanks!