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.
Created attachment 397955 [details] Patch
Created attachment 397956 [details] [Video] With patch applied
<rdar://problem/62590810>
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 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`!
Created attachment 398435 [details] Patch
Committed r261127: <https://trac.webkit.org/changeset/261127> All reviewed patches have been landed. Closing bug and clearing flags on attachment 398435 [details].