Bug 187488 - Web Inspector: Layers visualization shouldn't select on mousedown
Summary: Web Inspector: Layers visualization shouldn't select on mousedown
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Ross Kirsling
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-07-09 16:07 PDT by Ross Kirsling
Modified: 2018-07-19 21:32 PDT (History)
5 users (show)

See Also:


Attachments
Patch (3.88 KB, patch)
2018-07-10 10:55 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews201 for win-future (12.96 MB, application/zip)
2018-07-10 20:29 PDT, EWS Watchlist
no flags Details
Patch (4.35 KB, patch)
2018-07-13 22:06 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (4.26 KB, patch)
2018-07-14 17:30 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.80 MB, application/zip)
2018-07-14 20:27 PDT, EWS Watchlist
no flags Details
Patch for landing (4.01 KB, patch)
2018-07-19 20:46 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ross Kirsling 2018-07-09 16:07:31 PDT
From a comment by Matt Baker (https://bugs.webkit.org/show_bug.cgi?id=185109#c3):

When one drags to rotate or pan the Layers tab visualization, the mousedown event which begins the drag also updates the selection.
This may come as a surprise to the user, if they were only intending to take a different look at the current selection.

Instead of updating the selection on mousedown, we should probably update on mouseup, just when the mousedown and mouseup targets are the same.
Comment 1 Ross Kirsling 2018-07-10 10:55:30 PDT
Created attachment 344710 [details]
Patch
Comment 2 EWS Watchlist 2018-07-10 20:29:34 PDT
Comment on attachment 344710 [details]
Patch

Attachment 344710 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8501051

New failing tests:
http/tests/security/canvas-remote-read-remote-video-localhost.html
Comment 3 EWS Watchlist 2018-07-10 20:29:45 PDT
Created attachment 344748 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 4 Matt Baker 2018-07-13 16:22:23 PDT
Comment on attachment 344710 [details]
Patch

This patch does what the bug describes: it only selects on mouseup, and only when the mouseup and mousedown targets match. However, I was expecting selection to not work *at all* when performing a pan or rotate. Just to clarify, what is the intended behavior?
Comment 5 Ross Kirsling 2018-07-13 22:06:04 PDT
Created attachment 345023 [details]
Patch
Comment 6 Ross Kirsling 2018-07-13 22:10:52 PDT
(In reply to Matt Baker from comment #4)
> Comment on attachment 344710 [details]
> Patch
> 
> This patch does what the bug describes: it only selects on mouseup, and only
> when the mouseup and mousedown targets match. However, I was expecting
> selection to not work *at all* when performing a pan or rotate. Just to
> clarify, what is the intended behavior?

Well, that is intentional in the sense that it's a fix that I'd intended to make half a year ago but evidently let slip. 😅 I decided to start there since I thought it might match the spirit of your concern -- the inspiration for that behavior is that of a button, which allows you to move away and back between mousedown and mouseup.

To match the specific behavior you mentioned, I've tried adding an extra condition that the mouse position be near-equal before and after. Let me know what you think. :)
Comment 7 Ross Kirsling 2018-07-14 17:30:02 PDT
Created attachment 345045 [details]
Patch
Comment 8 Ross Kirsling 2018-07-14 17:33:06 PDT
(In reply to Ross Kirsling from comment #6)
> To match the specific behavior you mentioned, I've tried adding an extra
> condition that the mouse position be near-equal before and after. Let me
> know what you think. :)

Derp. Realized it's simpler and more accurate to just clear the candidate selection via a one-shot mousemove handler...
Comment 9 EWS Watchlist 2018-07-14 20:27:12 PDT
Comment on attachment 345045 [details]
Patch

Attachment 345045 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8540266

New failing tests:
http/tests/security/canvas-remote-read-remote-video-localhost.html
Comment 10 EWS Watchlist 2018-07-14 20:27:24 PDT
Created attachment 345049 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 11 Matt Baker 2018-07-19 15:48:55 PDT
Comment on attachment 345045 [details]
Patch

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

Works great! r=me, with a few style suggestions to consider.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:166
>          this._mouse = new THREE.Vector2;

Does this really need to be part of the class state? It's only used in _findHoveredLayerGroup, and is recalculated each time.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:318
> +        this._candidateSelection = {layerGroup: this._findHoveredLayerGroup(event)};

Since layerGroup is the only property of _candidateSelection, I'd just store it directly:

this._candidateSelection = this._findHoveredLayerGroup(event.offsetX, event.offsetY);

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:342
> +    _findHoveredLayerGroup({offsetX, offsetY, target: {width, height}})

I think this method could be cleaned up a bit. Do we really need to pass the width and height from the event object? How about:

_findHoveredLayerGroup(offsetX, offsetY)
{
    let canvas = this._renderer.domElement;
    let x = (offsetX / canvas.offsetWidth) * 2 - 1;
    let y = -(offsetY / canvas.offsetHeight) * 2 + 1;

    this._raycaster.setFromCamera(new THREE.Vector2(x, y), this._camera);

    const recursive = true;
    let intersects = this._raycaster.intersectObjects(this._scene.children, recursive);
    return intersects.length ? intersects[0].object.parent : null;
}
Comment 12 Ross Kirsling 2018-07-19 16:00:56 PDT
(In reply to Matt Baker from comment #11)
> > Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:318
> > +        this._candidateSelection = {layerGroup: this._findHoveredLayerGroup(event)};
> 
> Since layerGroup is the only property of _candidateSelection, I'd just store
> it directly:
> 
> this._candidateSelection = this._findHoveredLayerGroup(event.offsetX,
> event.offsetY);

The reason for this one is because I'm distinguishing `{layerGroup: null}` (attempting to deselect by clicking on open space) from `null` (no attempt in progress).
Comment 13 Ross Kirsling 2018-07-19 20:46:41 PDT
Created attachment 345423 [details]
Patch for landing
Comment 14 WebKit Commit Bot 2018-07-19 21:29:28 PDT
Comment on attachment 345423 [details]
Patch for landing

Clearing flags on attachment: 345423

Committed r234021: <https://trac.webkit.org/changeset/234021>
Comment 15 WebKit Commit Bot 2018-07-19 21:29:29 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2018-07-19 21:32:02 PDT
<rdar://problem/42414466>