* SUMMARY Dragging a timeline ruler handle when both handles clamped is broken. Overlapping handles are also styled incorrectly. When only one handle is clamped it should be styled white with a gray border (currently works), but when both are clamped the handle background color should match the ruler header. * STEPS TO REPRODUCE 1. Record a timeline 2. Make a ruler selection near the beginning of the graph 3. Scroll graph until selection falls off the left edge and both handles are clamped 4. Click and drag the ruler handle to the right => The expected behavior is that the right handle was selected and that dragging will modify the selection end time. Instead the left handle is selected and selection behavior is erratic.
Created attachment 258781 [details] [Patch] Proposed Fix
Comment on attachment 258781 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=258781&action=review > Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:591 > + let endTimeClamped = this._selectionEndTime > this._endTime || this._selectionEndTime < this._startTime; NIT: I would switch the order of these two expressions so that the "<" and ">" match on either side of the "||" compared to the line above. > Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:598 > + let newLeftPosition = Math.max(0, Math.min(1, (this._selectionStartTime - this._startTime) / duration)); We actually have a function called "clamp" in Utilities for this exact purpose. > Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:607 > + let newRightPosition = 1 - Math.max(0, Math.min(1, (this._selectionEndTime - this._startTime) / duration)); See line 598.
Created attachment 258788 [details] [Patch] Proposed Fix
Created attachment 258790 [details] [Video] New behavior
Comment on attachment 258788 [details] [Patch] Proposed Fix Clearing flags on attachment: 258788 Committed r188328: <http://trac.webkit.org/changeset/188328>
All reviewed patches have been landed. Closing bug.
Comment on attachment 258781 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=258781&action=review >> Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:598 >> + let newLeftPosition = Math.max(0, Math.min(1, (this._selectionStartTime - this._startTime) / duration)); > > We actually have a function called "clamp" in Utilities for this exact purpose. Number.constrain((this._selectionStartTime - this._startTime) / duration, 0, 1);
(In reply to comment #7) > Comment on attachment 258781 [details] > [Patch] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=258781&action=review > > >> Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:598 > >> + let newLeftPosition = Math.max(0, Math.min(1, (this._selectionStartTime - this._startTime) / duration)); > > > > We actually have a function called "clamp" in Utilities for this exact purpose. > > Number.constrain((this._selectionStartTime - this._startTime) / duration, 0, > 1); We should remove clamp and adopt Number.constrain.
> We should remove clamp and adopt Number.constrain. https://bugs.webkit.org/show_bug.cgi?id=147952