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.
<rdar://problem/27553228>
This is pretty bad! Good find.
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.
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.
Created attachment 297467 [details] Patch
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
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
Created attachment 297488 [details] Patch
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.
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.
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.
Created attachment 297548 [details] Patch
Comment on attachment 297548 [details] Patch Clearing flags on attachment: 297548 Committed r210046: <http://trac.webkit.org/changeset/210046>
All reviewed patches have been landed. Closing bug.