Web Inspector: Layers visualization shouldn't select on mousedown

• Component: Web Inspector
• Status: RESOLVED FIXED

[Ross Kirsling]

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.

[Ross Kirsling]

Created attachment 344710 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Matt Baker]

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?

[Ross Kirsling]

Created attachment 345023 [details]
Patch

[Ross Kirsling]

(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. :)

[Ross Kirsling]

Created attachment 345045 [details]
Patch

[Ross Kirsling]

(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...

[EWS Watchlist]

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

[EWS Watchlist]

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

[Matt Baker]

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;
}

[Ross Kirsling]

(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).

[Ross Kirsling]

Created attachment 345423 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 345423 [details]
Patch for landing

Clearing flags on attachment: 345423

Committed r234021: <https://trac.webkit.org/changeset/234021>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42414466>