Bug 211177 - Web Inspector: Tabs jiggle on click
Summary: Web Inspector: Tabs jiggle on click
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: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-29 03:52 PDT by Nikita Vasilyev
Modified: 2020-05-04 17:04 PDT (History)
3 users (show)

See Also:


Attachments
[Video] Bug (561.85 KB, video/quicktime)
2020-04-29 03:52 PDT, Nikita Vasilyev
no flags Details
Patch (3.26 KB, patch)
2020-04-29 04:30 PDT, Nikita Vasilyev
hi: review+
Details | Formatted Diff | Diff
[Video] With patch applied (941.98 KB, video/quicktime)
2020-04-29 04:33 PDT, Nikita Vasilyev
no flags Details
Patch (4.74 KB, patch)
2020-05-04 16:42 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2020-04-29 03:52:34 PDT
Created attachment 397953 [details]
[Video] Bug

When clicking on tabs they often jiggle a little. This happens because it's very easy to trigger tab dragging. We should increase the number of pixel required the mouse cursor to move before we start dragging tabs.
Comment 1 Nikita Vasilyev 2020-04-29 04:30:37 PDT
Created attachment 397955 [details]
Patch
Comment 2 Nikita Vasilyev 2020-04-29 04:33:20 PDT
Created attachment 397956 [details]
[Video] With patch applied
Comment 3 Radar WebKit Bug Importer 2020-04-29 04:34:03 PDT
<rdar://problem/62590810>
Comment 4 Devin Rousso 2020-04-29 07:54:24 PDT
Comment on attachment 397955 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:70
> +        this._mouseDownPageX = undefined;

NIT: the only time we should be using `undefined` is for timeout IDs and of we need a tri-state (e.g. `true`, `false`, "unset"), so in this case, since we always expect a number, I'd use `NaN` and then check for `isNaN`

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:739
> +        this._mouseDownPageX = event.pageX;

We should use this _instead_ of `_mouseIsDown` and avoid another property.

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:800
> +        if (this._mouseOffset === undefined)
> +            this._mouseOffset = event.pageX - this._selectedTabBarItem.element.totalOffsetLeft;

Why did this move?

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:804
> +            const draggingSensitivity = 12;

Is this something you estimated/observed by eye or looked at some source code to determine?

NIT: I'd call this `dragThreshold` as "sensitivity" is a bit ambiguous

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:824
>  

NIT: if the below is removed (see :799) then i'd also remove this line
Comment 5 Nikita Vasilyev 2020-04-29 13:29:40 PDT
Comment on attachment 397955 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/TabBar.js:70
>> +        this._mouseDownPageX = undefined;
> 
> NIT: the only time we should be using `undefined` is for timeout IDs and of we need a tri-state (e.g. `true`, `false`, "unset"), so in this case, since we always expect a number, I'd use `NaN` and then check for `isNaN`

I don't think it's important, but I'd rather never use NaN intentionally, if it was up to me. If we did that, seeing NaN would mean there was a type error somewhere.

Also, I find it rather arbitrary that we use undefined for timers. The 1st setTimeout/setInterval call returns 1. It isn't going to change because of web compatibility issues. We could've used 0 is the default value there.

>> Source/WebInspectorUI/UserInterface/Views/TabBar.js:739
>> +        this._mouseDownPageX = event.pageX;
> 
> We should use this _instead_ of `_mouseIsDown` and avoid another property.

Yeah, I was contemplating between clarity and saving on one variable.

>> Source/WebInspectorUI/UserInterface/Views/TabBar.js:800
>> +            this._mouseOffset = event.pageX - this._selectedTabBarItem.element.totalOffsetLeft;
> 
> Why did this move?

So dragging start from the clicked pixel and not the clicked pixel + dragThreshold.

>> Source/WebInspectorUI/UserInterface/Views/TabBar.js:804
>> +            const draggingSensitivity = 12;
> 
> Is this something you estimated/observed by eye or looked at some source code to determine?
> 
> NIT: I'd call this `dragThreshold` as "sensitivity" is a bit ambiguous

Just observed.
I like `dragThreshold`!
Comment 6 Nikita Vasilyev 2020-05-04 16:42:47 PDT
Created attachment 398435 [details]
Patch
Comment 7 EWS 2020-05-04 17:03:59 PDT
Committed r261127: <https://trac.webkit.org/changeset/261127>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398435 [details].