Bug 153263 - [Mac] Tooltips do not honor some types of obscuring windows
Summary: [Mac] Tooltips do not honor some types of obscuring windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-01-19 17:55 PST by Brent Fulgham
Modified: 2016-01-22 08:50 PST (History)
6 users (show)

See Also:


Attachments
Patch (4.62 KB, patch)
2016-01-19 18:06 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (303.29 KB, application/zip)
2016-01-19 18:46 PST, Build Bot
no flags Details
Patch (3.84 KB, patch)
2016-01-20 10:45 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (5.86 KB, patch)
2016-01-20 16:17 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (8.50 KB, patch)
2016-01-21 17:55 PST, Brent Fulgham
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2016-01-19 17:55:02 PST
When a WebView is obscured by the Notification Center or Spotlight windows, tooltips continue to be displayed even though the mouse pointer is no longer in the WebView.

This is happening because these these types of applications (and perhaps others) do not become the foreground application, confusing WebKit.
Comment 1 Brent Fulgham 2016-01-19 17:55:34 PST
<rdar://problem/21423972>
Comment 2 Brent Fulgham 2016-01-19 18:06:53 PST
Created attachment 269323 [details]
Patch
Comment 3 WebKit Commit Bot 2016-01-19 18:08:20 PST
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.
Comment 4 Tim Horton 2016-01-19 18:10:43 PST
Does WebKit1 have the same bug?
Comment 5 Tim Horton 2016-01-19 18:11:44 PST
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.
Comment 6 Tim Horton 2016-01-19 18:12:32 PST
Also, we have this big ViewState mechanism. Does it make any sense to make this a part of that?
Comment 7 Build Bot 2016-01-19 18:45:56 PST
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.
Comment 8 Build Bot 2016-01-19 18:46:00 PST
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 9 Simon Fraser (smfr) 2016-01-19 19:02:22 PST
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?
Comment 10 Brent Fulgham 2016-01-20 10:45:09 PST
Created attachment 269360 [details]
Patch
Comment 11 Simon Fraser (smfr) 2016-01-20 11:16:50 PST
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.
Comment 12 Brent Fulgham 2016-01-20 16:17:35 PST
Created attachment 269399 [details]
Patch
Comment 13 Brent Fulgham 2016-01-21 09:53:37 PST
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 14 Simon Fraser (smfr) 2016-01-21 10:37:15 PST
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:
Comment 15 Tim Horton 2016-01-21 10:38:40 PST
(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:
Comment 16 Brent Fulgham 2016-01-21 17:55:54 PST
Created attachment 269525 [details]
Patch
Comment 17 Simon Fraser (smfr) 2016-01-21 20:01:27 PST
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 "".
Comment 18 Brent Fulgham 2016-01-22 08:50:25 PST
Committed r195451: <http://trac.webkit.org/changeset/195451>