Bug 126923 - Web Inspector: support handling mouse events on the inspector page overlay
Summary: Web Inspector: support handling mouse events on the inspector page overlay
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-01-13 11:38 PST by BJ Burg
Modified: 2016-12-13 15:37 PST (History)
4 users (show)

See Also:


Attachments
[WIP] (134.53 KB, patch)
2015-01-28 21:44 PST, Brian Burg
no flags Details | Formatted Diff | Diff
WIP (140.89 KB, patch)
2015-02-01 14:28 PST, Brian Burg
no flags Details | Formatted Diff | Diff
WIP (178.67 KB, patch)
2015-02-05 07:46 PST, Brian Burg
no flags Details | Formatted Diff | Diff
Patch (146.32 KB, patch)
2015-02-11 06:52 PST, Brian Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2014-01-13 11:38:52 PST
This will allow interaction with the overlay, such as showing interactive highlights of shapes, regions, rectangle selection, etc.
Comment 1 Timothy Hatcher 2014-01-13 12:56:32 PST
<rdar://problem/15806447>
Comment 2 Brian Burg 2015-01-28 21:44:14 PST
Created attachment 245608 [details]
[WIP]
Comment 3 Brian Burg 2015-01-29 22:13:10 PST
WIP notes:

Current idea is to add a WKWebView for the overlay page rather than using a PageOverlay. Specific highlight methods can be moved over to this overlay as part of the patch or in a follow-up.

As for page lifetime, the patch pairs together the setup and teardown of the overlay page and the inspector page. The overlay page is not used unless the inspector is active. (This may be split into two messages if necessary to make IPC work out.)

The point of making this overlay a real WebPage is so that we can add buttons, tooltips, direct manipulation, and other interactive features without further shoehorning PageOverlay to do things it wasn't designed for.

To me, it doesn't make much sense to put iOS (i.e., CALayer based) paint highlighting through this overlay page because 1) paint flashing requires low latency, which in practice is hard given the message-passing nature of Inspector and Overlay pages; 2) paint flashes aren't interactive, so they get no benefit from being rendered to an interactive overlay page.

As for other ports, I assume they can easily create an overlay page and composite it over the inspected page when the inspector opens. That's what a WKWebView approach does for mac/ios.
Comment 4 Brian Burg 2015-02-01 14:28:52 PST
Created attachment 245844 [details]
WIP
Comment 5 Timothy Hatcher 2015-02-02 14:55:54 PST
Comment on attachment 245844 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=245844&action=review

> Source/WebKit2/UIProcess/WebInspectorProxy.h:138
> +    static bool isOverlayPage(WebPageProxy&);

isInspectorOverlayPage(WebPageProxy&)?
Comment 6 Brian Burg 2015-02-05 07:46:03 PST
Created attachment 246101 [details]
WIP

Latest patch can show and hide the overlay WKWebView. Does not forward unhandled user inputs yet. Does not recieve highlight commands.
Comment 7 Timothy Hatcher 2015-02-05 08:18:26 PST
Comment on attachment 246101 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=246101&action=review

> Source/WebCore/inspector/InspectorDOMAgent.h:112
> +    virtual void didCreateFrontendAndBackend(Inspector::InspectorFrontendChannel*, Inspector::OverlayChannel*, Inspector::InspectorBackendDispatcher*) override;

We should rename Inspector::InspectorFrontendChannel to something like Inspector::FrontendChannel or Inspector::UserInterfaceChannel.

> Source/WebKit2/UIProcess/WebInspectorProxy.cpp:502
> +void WebInspectorProxy::eagerlyCreateOverlayPage()

Not sure we need to eagerly do this for the overlay. I did it for the fronted because it shaves ~500ms of load time opening the Inspector. Though if there is a noticeable delay in the overlay, maybe we should.

> Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:947
> +    [[NSNotificationCenter defaultCenter] addObserver:m_inspectorProxyObjCAdapter.get() selector:@selector(inspectedViewFrameDidChange:) name:NSViewFrameDidChangeNotification object:inspectedView];

We should consider using constraints here (and for the Inspector). They will allow us to auto magically follow the frame. I plan to look at this soon (for other reasons) if you don't want to try it. It might require switching other views to auto layout and sprinkling translatesAutoresizingMaskIntoConstraints = NO.

Roughly what you should need:

    [NSLayoutConstraint constraintWithItem:overlayView attribute:NSLayoutAttributeTop relatedBy:NSLayoutRelationEqual toItem:inspectedView attribute:NSLayoutAttributeTop multiplier:1 constant:0].active = YES;
    [NSLayoutConstraint constraintWithItem:overlayView attribute:NSLayoutAttributeBottom relatedBy:NSLayoutRelationEqual toItem:inspectedView attribute:NSLayoutAttributeBottom multiplier:1 constant:0].active = YES;
    [NSLayoutConstraint constraintWithItem:overlayView attribute:NSLayoutAttributeLeft relatedBy:NSLayoutRelationEqual toItem:inspectedView attribute:NSLayoutAttributeLeft multiplier:1 constant:0].active = YES;
    [NSLayoutConstraint constraintWithItem:overlayView attribute:NSLayoutAttributeRight relatedBy:NSLayoutRelationEqual toItem:inspectedView attribute:NSLayoutAttributeRight multiplier:1 constant:0].active = YES;
Comment 8 Brian Burg 2015-02-05 09:05:33 PST
Comment on attachment 246101 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=246101&action=review

>> Source/WebCore/inspector/InspectorDOMAgent.h:112
>> +    virtual void didCreateFrontendAndBackend(Inspector::InspectorFrontendChannel*, Inspector::OverlayChannel*, Inspector::InspectorBackendDispatcher*) override;
> 
> We should rename Inspector::InspectorFrontendChannel to something like Inspector::FrontendChannel or Inspector::UserInterfaceChannel.

In general, this seems fine for things defined in Inspector namespace within JSC. For WebCore things, it might lead to some awkward filename clashes.

>> Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:947
>> +    [[NSNotificationCenter defaultCenter] addObserver:m_inspectorProxyObjCAdapter.get() selector:@selector(inspectedViewFrameDidChange:) name:NSViewFrameDidChangeNotification object:inspectedView];
> 
> We should consider using constraints here (and for the Inspector). They will allow us to auto magically follow the frame. I plan to look at this soon (for other reasons) if you don't want to try it. It might require switching other views to auto layout and sprinkling translatesAutoresizingMaskIntoConstraints = NO.
> 
> Roughly what you should need:
> 
>     [NSLayoutConstraint constraintWithItem:overlayView attribute:NSLayoutAttributeTop relatedBy:NSLayoutRelationEqual toItem:inspectedView attribute:NSLayoutAttributeTop multiplier:1 constant:0].active = YES;
>     [NSLayoutConstraint constraintWithItem:overlayView attribute:NSLayoutAttributeBottom relatedBy:NSLayoutRelationEqual toItem:inspectedView attribute:NSLayoutAttributeBottom multiplier:1 constant:0].active = YES;
>     [NSLayoutConstraint constraintWithItem:overlayView attribute:NSLayoutAttributeLeft relatedBy:NSLayoutRelationEqual toItem:inspectedView attribute:NSLayoutAttributeLeft multiplier:1 constant:0].active = YES;
>     [NSLayoutConstraint constraintWithItem:overlayView attribute:NSLayoutAttributeRight relatedBy:NSLayoutRelationEqual toItem:inspectedView attribute:NSLayoutAttributeRight multiplier:1 constant:0].active = YES;

Sounds fine to me (it might allow us to remove the ObjC proxy), though I probably won't have time to attempt that myself. The frame sizing code in this file is kinda scary.

> Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:950
> +    // FIXME: when the overlay is ready to handle user input, make first responder here.

Am still investigating how Cocoa or WebKit decides which WebPageProxy to send user input to. Without first responder here, the overlay still gets most events.
Comment 9 Brian Burg 2015-02-11 06:52:19 PST
Created attachment 246388 [details]
Patch

Latest patch implements overlay attach and detach. Does not yet delegate events to inspected page when not inactive.