Bug 147013 – Web Inspector: Clicking a frame in the Rendering Frames timeline should select the corresponding grid row

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

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

[Matt Baker]

<rdar://problem/21223523>

[Matt Baker]

Created attachment 257346 [details]
[Patch] Proposed Fix

[Matt Baker]

Created attachment 257357 [details]
[Image] UI - selected frame

[Joseph Pecoraro]

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?

[Matt Baker]

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

[Matt Baker]

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

[Matt Baker]

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.

[Simon Fraser (smfr)]

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.

[Matt Baker]

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

[Timothy Hatcher]

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

[Timothy Hatcher]

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

[Brian Burg]

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

[Timothy Hatcher]

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.

[Matt Baker]

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

[Matt Baker]

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.

[Matt Baker]

Created attachment 257379 [details]
[Image] short frame selected, no arrow

[Matt Baker]

Created attachment 257380 [details]
[Image] tall frame selected, no arrow

Very hard to tell the tall frame is selected.

[Matt Baker]

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.

[Timothy Hatcher]

Comment on attachment 257385 [details]
[Image] alternative frame markers

I like A or B here.

[Matt Baker]

Created attachment 257555 [details]
[Patch] Proposed Fix

[Timothy Hatcher]

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.

[Matt Baker]

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.

[Timothy Hatcher]

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.

[Matt Baker]

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

[Timothy Hatcher]

Got it. Makes sense now! Proceed.

[Matt Baker]

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

[Matt Baker]

Disregard that last comment. Must have been leftover comment junk in the Review Patch page.

[WebKit Commit Bot]

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

Clearing flags on attachment: 257555

Committed r187468: <http://trac.webkit.org/changeset/187468>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.