| Summary: | REGRESSION (r48446): While a <select> popup menu is open, the rest of the WebView doesn't respond to mouse move events | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Adam Roben (:aroben) <aroben> | ||||||
| Component: | Platform | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | andersca | ||||||
| Priority: | P2 | Keywords: | InRadar, PlatformOnly, Regression | ||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | PC | ||||||||
| OS: | Windows XP | ||||||||
| URL: | 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 | ||||||||
| Attachments: |
|
||||||||
Created attachment 39778 [details]
Patch v1
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+ 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. 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. (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! Created attachment 39780 [details]
Patch v1
|
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.