Bug 29332 – REGRESSION (r48446): While a <select> popup menu is open, the rest of the WebView doesn't respond to mouse move events

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Adam Roben (:aroben)]

To reproduce:

1. Go to data:text/html,%3Cstyle%3Ea:hover%20{%20color:%20green%20}%3C/style%3E%3Cselect%3E%3Coption%3E1%3C/select%3E%3Ca%20href=%22#%22%3Ea%20link%3C/a%3E
2. Click on the <select> to open its popup menu
3. Hover over the link

The mouse cursor doesn't change to a pointing hand, and the link doesn't turn green like it should. This only happens in Safari; Firefox and IE behave correctly.

[Adam Roben (:aroben)]

<rdar://problem/7231652>

[Anders Carlsson]

Created attachment 39778 [details]
Patch v1

[Darin Adler]

Comment on attachment 39778 [details]
Patch v1

> +static void translatePoint(LPARAM& lParam, HWND from, HWND to) {

Please put the brace in a second line.

> +    // Protect the popup menu in case its owner is destroyed while we're running the message pump.
> +    RefPtr<PopupMenu> protect(this);

I hate "protect" -- if there's any other way to make the lifetime work I would prefer it. It's so hard to know when you need to protect something -- once we add one of these it becomes voodoo code you can never remove. But it's better than explicit ref/deref, that's for sure.

> +    if (event->type == NPCocoaEventMouseUp) {
> +        NSBeep();
> +    }

Did you mean to land this?

>  			buildSettings = {
> -				ARCHS = "$(ARCHS_STANDARD_32_64_BIT_PRE_XCODE_3_1)";
> +				ARCHS = "$(ONLY_ACTIVE_ARCH_PRE_XCODE_3_1)";
>  				ARCHS_STANDARD_32_64_BIT_PRE_XCODE_3_1 = "ppc i386 ppc64 x86_64";
>  				GCC_WARN_ABOUT_RETURN_TYPE = YES;
>  				GCC_WARN_UNUSED_VARIABLE = YES;
> +				ONLY_ACTIVE_ARCH_PRE_XCODE_3_1 = "$(NATIVE_ARCH)";
>  				PREBINDING = NO;
> -				SDKROOT = /Developer/SDKs/MacOSX10.5.sdk;
> +				SDKROOT = "";
>  			};

Did you mean to land this?

I'm going to say review- because of those example changes, but it's really close to a review+

[Adam Roben (:aroben)]

Comment on attachment 39778 [details]
Patch v1

> +    pt.x = (short)LOWORD(lParam);
> +    pt.y = (short)HIWORD(lParam);    

GET_X_LPARAM and GET_Y_LPARAM would be better here.

[Sam Weinig]

Comment on attachment 39778 [details]
Patch v1

> +static void translatePoint(LPARAM& lParam, HWND from, HWND to) {
> +    POINT pt;
> +    pt.x = (short)LOWORD(lParam);
> +    pt.y = (short)HIWORD(lParam);    
> +    ::MapWindowPoints(from, to, &pt, 1);
> +    lParam = MAKELPARAM(pt.x, pt.y);
> +}

The { should be on a new line.

[Anders Carlsson]

(In reply to comment #3)
> (From update of attachment 39778 [details])
> > +static void translatePoint(LPARAM& lParam, HWND from, HWND to) {
> 
> Please put the brace in a second line.
> 

Whoops, fixed.

> > +    // Protect the popup menu in case its owner is destroyed while we're running the message pump.
> > +    RefPtr<PopupMenu> protect(this);
> 
> I hate "protect" -- if there's any other way to make the lifetime work I would
> prefer it. It's so hard to know when you need to protect something -- once we
> add one of these it becomes voodoo code you can never remove. But it's better
> than explicit ref/deref, that's for sure.
> 

I couldn't think of a way to do this without having to redesign a lot of the popup menu API.

> Did you mean to land this?
> 

Nope, I blame bugzilla-tool...

> Did you mean to land this?
> 

Nope, same here.

> I'm going to say review- because of those example changes, but it's really
> close to a review+

OK, I'll upload a new patch. Thanks!

[Anders Carlsson]

Created attachment 39780 [details]
Patch v1

[Anders Carlsson]

http://trac.webkit.org/changeset/48529