Summary: | Web Inspector: Tabs jiggle on click | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nikita Vasilyev <nvasilyev> | ||||||||||
Component: | Web Inspector | Assignee: | Nikita Vasilyev <nvasilyev> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | hi, inspector-bugzilla-changes, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Created attachment 397955 [details]
Patch
Created attachment 397956 [details]
[Video] With patch applied
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]. |
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.