Bug 211177 – Web Inspector: Tabs jiggle on click

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

Created attachment 397955 [details]
Patch

[Nikita Vasilyev]

Created attachment 397956 [details]
[Video] With patch applied

[Radar WebKit Bug Importer]

<rdar://problem/62590810>

[Devin Rousso]

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

[Nikita Vasilyev]

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`!

[Nikita Vasilyev]

Created attachment 398435 [details]
Patch

[EWS]

Committed r261127: <https://trac.webkit.org/changeset/261127>

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