Bug 160207 – Web Inspector: Window resize causes TimelineOverview graph elements to be repositioned

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

Created attachment 284622 [details]
[Video] Timeline overview resize bug

Summary:
Window resize causes TimelineOverview graph elements to be repositioned. Graph elements and ruler markers are shifted horizontally during resize once the window is too narrow to show the current time marker (see attachment).

Steps to Reproduce:
1. Record a timeline
2. Zoom (wheel) the overview graph until the current time marker is just before the right edge of the window
3. Resize window, decreasing width until current time marker should no longer fit
Expected:
  => Current time marker becomes hidden
Actual:
  => Current time marker moves with the window edge, and is always visible. Graph elements shift as well.

[Radar WebKit Bug Importer]

<rdar://problem/27553228>

[Timothy Hatcher]

This is pretty bad! Good find.

[Matt Baker]

Investigating this for a *very* long time uncovered a subtle bug in TimelineRuler.prototype.layout which made it possible for dividers and labels to remain fixed while the ruler start time is changing.

So, it turns out the issue here isn't that graph elements are being repositioned, but that ruler's elements *aren't* being repositioned.

[Matt Baker]

Created attachment 297451 [details]
[Video] TimelineRuler divider calculation

The way ruler dividers are calculated leads to the bug. The video shows the dividerData calculation updating in real-time, as the window is resized.

Note that resizing the window doesn't cause dividers to be added or removed, or for their positions to change. The logged values (Inspector² console) show that the data is changing however, apparently because `dividerData.lastTime` holds the actual end time and not the calculated time of the last visible divider. A related point: the divider count is shows as 7, when only 5 dividers are visible.

[Matt Baker]

Created attachment 297467 [details]
Patch

[Build Bot]

Comment on attachment 297467 [details]
Patch

Attachment 297467 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2755902

New failing tests:
imported/w3c/web-platform-tests/IndexedDB/interfaces.worker.html

[Build Bot]

Created attachment 297482 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Matt Baker]

Created attachment 297488 [details]
Patch

[Blaze Burg]

Comment on attachment 297488 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=297488&action=review

r=me

> Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:453
> +        let firstDividerTime = (Math.ceil((this._startTime - this._zeroTime) / sliceTime) * sliceTime) + this._zeroTime;

I had trouble understanding the derivation of this. My understanding is that we just start placing slices starting from zero time, then use the first one that's greater than startTime. Can you put this into a comment, as the formula is kind of inscrutable (I had to diagram it on a whiteboard).

It might be more readable to use the iterative solution rather than the closed formula, but if there's a comment explaining they are equivalent, then this can stay as-is.

[Matt Baker]

Comment on attachment 297488 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=297488&action=review

>> Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:453
>> +        let firstDividerTime = (Math.ceil((this._startTime - this._zeroTime) / sliceTime) * sliceTime) + this._zeroTime;
> 
> I had trouble understanding the derivation of this. My understanding is that we just start placing slices starting from zero time, then use the first one that's greater than startTime. Can you put this into a comment, as the formula is kind of inscrutable (I had to diagram it on a whiteboard).
> 
> It might be more readable to use the iterative solution rather than the closed formula, but if there's a comment explaining they are equivalent, then this can stay as-is.

The `firstDividerTime` formula hasn't changed, just `lastDividerTime`. This was being set equal to the end time, not the time position of the last divider. The effect was that the check for Object.shallowEqual would fail as long as the end time changed between layouts, ensuring the ruler is updated as the view scrolls/zooms. But when the overview start time changes while the end time remains fixed (the original steps to reproduce), the Object.shallowEqual check will pass and early return, so long as the first divider doesn't change.

[Blaze Burg]

Comment on attachment 297488 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=297488&action=review

>>> Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:453
>>> +        let firstDividerTime = (Math.ceil((this._startTime - this._zeroTime) / sliceTime) * sliceTime) + this._zeroTime;
>> 
>> I had trouble understanding the derivation of this. My understanding is that we just start placing slices starting from zero time, then use the first one that's greater than startTime. Can you put this into a comment, as the formula is kind of inscrutable (I had to diagram it on a whiteboard).
>> 
>> It might be more readable to use the iterative solution rather than the closed formula, but if there's a comment explaining they are equivalent, then this can stay as-is.
> 
> The `firstDividerTime` formula hasn't changed, just `lastDividerTime`. This was being set equal to the end time, not the time position of the last divider. The effect was that the check for Object.shallowEqual would fail as long as the end time changed between layouts, ensuring the ruler is updated as the view scrolls/zooms. But when the overview start time changes while the end time remains fixed (the original steps to reproduce), the Object.shallowEqual check will pass and early return, so long as the first divider doesn't change.

Oh. You should have put this comment into the changelog, as it actually tells me why the changes are necessary. "Update ruler layout to calculate accurate divider count and divider times," kinda goes without saying.

[Matt Baker]

Created attachment 297548 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 297548 [details]
Patch

Clearing flags on attachment: 297548

Committed r210046: <http://trac.webkit.org/changeset/210046>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.