Web Inspector: Dragging a timeline ruler handle when both handles clamped is broken

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

* 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.

[Matt Baker]

Created attachment 258781 [details]
[Patch] Proposed Fix

[Devin Rousso]

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.

[Matt Baker]

Created attachment 258788 [details]
[Patch] Proposed Fix

[Matt Baker]

Created attachment 258790 [details]
[Video] New behavior

[WebKit Commit Bot]

Comment on attachment 258788 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 258788

Committed r188328: <http://trac.webkit.org/changeset/188328>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[BJ Burg]

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);

[Timothy Hatcher]

(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.

[Matt Baker]

> We should remove clamp and adopt Number.constrain.

https://bugs.webkit.org/show_bug.cgi?id=147952