* SUMMARY Clicking a frame in the Rendering Frames timeline should select the corresponding grid row. Clicking a grid row should also select the corresponding frame. A marker should be added to the Rendering Frames overview graph identifying the currently selected frame. Additional behavior: 1. Clicking a frame outside the ruler selection should change the selection to include only the clicked frame. 2. If the ruler selection changes and no longer includes the selected frame, it should be deselected since it will no longer appear in the grid. * NOTES This should be implemented for the original Timelines view as well. However there isn't a 1-to-1 relationship between graph elements and timeline records there, so we may want to: a) Select the grid row corresponding to the first record when a bar representing multiple records is clicked. b) Extend DataGrid to allow multiple row selection.
<rdar://problem/21223523>
Created attachment 257346 [details] [Patch] Proposed Fix
Created attachment 257357 [details] [Image] UI - selected frame
Whoa! That arrow is kinda cool! It does however look off-center. Is that a 1x/2x only issue? Can we always get it centered?
> Whoa! That arrow is kinda cool! It does however look off-center. Is that a > 1x/2x only issue? Can we always get it centered? The arrow has margin-left: 1px. This was originally to adjust for the 1px of left padding on the frame element, but might not be necessary anymore. I'll play around with it a bit.
The arrow border dims slightly when the window is inactive (to match the ruler borders). The highlight color should probably do the same, and switch from light blue to light gray (something like #eee).
When a specific frame is selected, should the details sidebar show stats for that frame, rather than aggregate stats for the whole selection? If so there should probably be a way to deselect a frame, maybe by clicking it a second time. Currently the only way to deselect a frame is to change the ruler selection so that the frame falls outside the selection bounds.
Are you planning to add click support for events in the layout/painting and Javascript timelines as well? It will be really odd if frames work, but not these others.
(In reply to comment #8) > Are you planning to add click support for events in the layout/painting and > Javascript timelines as well? It will be really odd if frames work, but not > these others. Yes, this patch adds new methods to base classes TimelineOverview and TimelineOverviewGraph for just this reason.
Very cool. Though, I think we should drop the arrow. The selection color is enough. The color should also match the color we use in the console for selected log rows: background-color: hsl(210, 98%, 96%);
The selection color might also be covered if the frame is full. Maybe instead of just the selection color, also darken the segments? -webkit-filter: brightness(90%);
(In reply to comment #11) > The selection color might also be covered if the frame is full. Maybe > instead of just the selection color, also darken the segments? > > -webkit-filter: brightness(90%); We could use an effect like the overview timeline view used to have, where the pills look like silhouettes over the selection.
That would work too.(In reply to comment #12) > (In reply to comment #11) > > The selection color might also be covered if the frame is full. Maybe > > instead of just the selection color, also darken the segments? > > > > -webkit-filter: brightness(90%); > > We could use an effect like the overview timeline view used to have, where > the pills look like silhouettes over the selection. That would work too.
(In reply to comment #10) > Very cool. Though, I think we should drop the arrow. The selection color is > enough. The color should also match the color we use in the console for > selected log rows: > > background-color: hsl(210, 98%, 96%); I like the arrow, but I'll try some of the other suggestions.
I like the new highlight color, however decreasing the brightness doesn't do much to make tall frames stand out. I think the arrow (or some kind of marker) is needed here. I'll add some screenshots to illustrate.
Created attachment 257379 [details] [Image] short frame selected, no arrow
Created attachment 257380 [details] [Image] tall frame selected, no arrow Very hard to tell the tall frame is selected.
Created attachment 257385 [details] [Image] alternative frame markers Two more variations. A) adds a marker to the ruler, similar to the vertical rule we place by selected console messages. B) extends the highlight color into the ruler.
Comment on attachment 257385 [details] [Image] alternative frame markers I like A or B here.
Created attachment 257555 [details] [Patch] Proposed Fix
Comment on attachment 257555 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=257555&action=review > Source/WebInspectorUI/UserInterface/Views/RenderingFrameTimelineOverviewGraph.css:56 > + background: -webkit-linear-gradient(left, hsl(210, 100%, 49%), hsl(210, 100%, 49%)) no-repeat 1px 0; Does this need to be a linear-gradient? Can it just be background-color? > Source/WebInspectorUI/UserInterface/Views/RenderingFrameTimelineOverviewGraph.css:62 > + background-image: -webkit-linear-gradient(left, hsl(0, 0%, 60%), hsl(0, 0%, 60%)); Ditto. > Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.css:53 > + background: -webkit-linear-gradient(left, hsl(210, 98%, 96%), hsl(210, 98%, 96%)) no-repeat 1px -1px; background-color? > Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.css:57 > + background: -webkit-linear-gradient(left, hsl(0, 0%, 96%), hsl(0, 0%, 96%)) no-repeat 1px -1px; Ditto.
1px left padding is used to separate frame elements, since a 1px margin would change the width of the element and complicate the overview graph's positioning and scaling logic. Dropping the left padding altogether and spacing the frames during graph layout also adds additional complexity (and has its own problems). The linear gradient is the simplest way to shift the highlight 1px to the right, and prevent the padding between frame elements from being highlighted.
Margin does not affect the width, padding and border do. box-sizing: content-box; will exclude padding and border from the width. We use border-box typically.
(In reply to comment #23) > Margin does not affect the width, padding and border do. > > box-sizing: content-box; will exclude padding and border from the width. We > use border-box typically. I think I'm explaining this poorly. Frame elements have an integral pixel width, since the graph's seconds per pixel value snaps to pixel boundaries while zooming. We want border-box here, since the frame borders/padding need to included in the width determined by the overview graph. Adding a margin between frame elements or switching to content-box causes the graph elements to get out of synch with the ruler dividers.
Got it. Makes sense now! Proceed.
Comment on attachment 257555 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=257555&action=review >> Source/WebInspectorUI/UserInterface/Views/RenderingFrameTimelineOverviewGraph.css:56 >> + background: -webkit-linear-gradient(left, hsl(210, 100%, 49%), hsl(210, 100%, 49%)) no-repeat 1px 0; > > Does this need to be a linear-gradient? Can it just be background-color? This is the cleanest way to highlight the background. Since frame
Disregard that last comment. Must have been leftover comment junk in the Review Patch page.
Comment on attachment 257555 [details] [Patch] Proposed Fix Clearing flags on attachment: 257555 Committed r187468: <http://trac.webkit.org/changeset/187468>
All reviewed patches have been landed. Closing bug.