Summary: | [Mac] Tooltips do not honor some types of obscuring windows | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brent Fulgham <bfulgham> | ||||||||||||
Component: | Layout and Rendering | Assignee: | Brent Fulgham <bfulgham> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | bfulgham, buildbot, commit-queue, rniwa, simon.fraser, thorton | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Brent Fulgham
2016-01-19 17:55:02 PST
Created attachment 269323 [details]
Patch
Attachment 269323 [details] did not pass style-queue:
ERROR: Source/WebKit2/UIProcess/Cocoa/WebPageProxyCocoa.mm:29: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 1 in 4 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Does WebKit1 have the same bug? Comment on attachment 269323 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269323&action=review > Source/WebKit2/UIProcess/Cocoa/WebPageProxyCocoa.mm:67 > + NSInteger eventWindowNumber = [NSWindow windowNumberAtPoint:[NSEvent mouseLocation] belowWindowWithWindowNumber:0]; This is not Cocoa code, it's either AppKit or Mac code. Also, we have this big ViewState mechanism. Does it make any sense to make this a part of that? Comment on attachment 269323 [details] Patch Attachment 269323 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/714926 Number of test failures exceeded the failure limit. Created attachment 269325 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 269323 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269323&action=review >> Source/WebKit2/UIProcess/Cocoa/WebPageProxyCocoa.mm:67 >> + NSInteger eventWindowNumber = [NSWindow windowNumberAtPoint:[NSEvent mouseLocation] belowWindowWithWindowNumber:0]; > > This is not Cocoa code, it's either AppKit or Mac code. This also may not be cheap. It's probably a round trip with the WindowServer. I suspect that there are also legitimate times when other windows are on top but we still need to show tooltips. Why isn't this just an AppKit bug? Created attachment 269360 [details]
Patch
Comment on attachment 269360 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269360&action=review > Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:599 > +bool WebPageProxy::viewIsObscured() The name of this function... > Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:602 > + NSInteger eventWindowNumber = [NSWindow windowNumberAtPoint:[NSEvent mouseLocation] belowWindowWithWindowNumber:0]; ...doesn't agree with the implementation, which is looking at the global mouse location. I don't think we should have a function whose behavior depends implicitly on the mouse location. Created attachment 269399 [details]
Patch
Comment on attachment 269399 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269399&action=review > Source/WebKit/mac/WebView/WebHTMLView.mm:2138 > + Whoops! Extra space. Comment on attachment 269399 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269399&action=review > Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:608 > + NSPoint eventLocalWindowPosition = event.nativeEvent().locationInWindow; > + NSRect eventScreenPosition = [m_pageClient.platformWindow() convertRectToScreen:NSMakeRect(eventLocalWindowPosition.x, eventLocalWindowPosition.y, 0, 0)]; > + > + NSInteger currentWindowNumber = [m_pageClient.platformWindow() windowNumber]; > + NSInteger eventWindowNumber = [NSWindow windowNumberAtPoint:eventScreenPosition.origin belowWindowWithWindowNumber:0]; > + > + return currentWindowNumber != eventWindowNumber; Feels like this code should live up in WKWebView or somewhere. The name could also be improved; something like windowIsFrontWindowUnderMouse: (In reply to comment #14) > Comment on attachment 269399 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=269399&action=review > > > Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:608 > > + NSPoint eventLocalWindowPosition = event.nativeEvent().locationInWindow; > > + NSRect eventScreenPosition = [m_pageClient.platformWindow() convertRectToScreen:NSMakeRect(eventLocalWindowPosition.x, eventLocalWindowPosition.y, 0, 0)]; > > + > > + NSInteger currentWindowNumber = [m_pageClient.platformWindow() windowNumber]; > > + NSInteger eventWindowNumber = [NSWindow windowNumberAtPoint:eventScreenPosition.origin belowWindowWithWindowNumber:0]; > > + > > + return currentWindowNumber != eventWindowNumber; > > Feels like this code should live up in WKWebView or somewhere. WebViewImpl > The name could also be improved; something like > windowIsFrontWindowUnderMouse: Created attachment 269525 [details]
Patch
Comment on attachment 269525 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269525&action=review > Source/WebKit2/UIProcess/WebPageProxy.cpp:1671 > + setToolTip(""); I think we prefer String() to "". Committed r195451: <http://trac.webkit.org/changeset/195451> |