Bug 147389 – Web Inspector: Add the ability to filter out tasks in the Rendering Frames timeline

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

[Matt Baker]

* SUMMARY
Add the ability to filter out tasks in the frames timeline. Each task requires a column in the data grid, and as we add more categories (such as Canvas), the ability to filter will be important to reduce the noise.

I propose we add a filter "funnel" button to the left of the delete button in the navigation sidebar. Clicking the button would drop down a bar similar to the "find in page" bar (using an ease-in transition) used in the Resources & Elements tabs. The bar would contains checkboxes to toggle the frame tasks.

View behaviors:
 - Grid columns for filtered tasks are hidden.
 - Filtered tasks would still appear in the frames graph and doughnut chart.
 - Frame records for which the entire duration was spent in filtered tasks are hidden from the tree/grid.

Filter "funnel" button behavior:
 - Icon is an empty funnel if no tasks are filtered.
 - Icon is a partially filled funnel if one or filters are applied.

* NOTES
One concern I have is that we are now locating filtering in two places: in the existing filter text box at the bottom of the sidebar, and in the proposed task filter bar. We could move the filter text box to the new task filter bar (similar to Chrome), but this is contrary to the current UI paradigm of placing filter text boxes at the bottom of the navigation sidebar. The one exception I'm aware of is in the Console tab, which lacks a sidebar.

See mockups for icon states and filter bar UI.

[Radar WebKit Bug Importer]

<rdar://problem/22042544>

[Matt Baker]

Created attachment 257705 [details]
[Image] Mockup - filter icon

These mockups show a new Canvas task type, but I didn't bother adding it to the grid.

[Matt Baker]

Created attachment 257706 [details]
[Image] Mockup - filter bar toggled

The Done button is included for ease of use, but ESC and hitting the funnel button would also dismiss the filter bar.

[Matt Baker]

Created attachment 257708 [details]
[Image] Mockup - filter applied

Unchecking Canvas changes the funnel icon to the "partially filled" state, to indicate that a filter is active.

[Devin Rousso]

I think that this is a really awesome idea.  I do think, however, that adding another navbar is a bit cluttered.  Instead, you may consider moving the colored checkboxes (I really like these) to the key for the hollow pie chart on the left and allow the user to click the colored boxes (which will now be checkboxes) to filter.

[Matt Baker]

(In reply to comment #5)
> I think that this is a really awesome idea.  I do think, however, that
> adding another navbar is a bit cluttered.  Instead, you may consider moving
> the colored checkboxes (I really like these) to the key for the hollow pie
> chart on the left and allow the user to click the colored boxes (which will
> now be checkboxes) to filter.

I like the idea of repurposing the legend keys and reducing clutter. I'm open to this, just a few things to consider:

- The sidebar isn't always visible. Not a huge deal, as we place key UI in sidebars elsewhere.
- A user might expect to see the doughnut chart update when checking/unchecking legend items. The filters don't apply to the graph, but this UI would imply that they do.
- We're going to add a frame duration filter too (All, >= 1ms, >= 15ms), which would fit nicely in the filter bar (and familiar to users of Chrome).

[Devin Rousso]

(In reply to comment #6)
> - We're going to add a frame duration filter too (All, >= 1ms, >= 15ms),
> which would fit nicely in the filter bar (and familiar to users of Chrome).

Why not collapse all the filters (including the duration filters) into a scope bar next to "Records" similar to how Timelines > Timelines (instead of Timelines > Rendering Frames) does it?

[Matt Baker]

(In reply to comment #7)
> Why not collapse all the filters (including the duration filters) into a
> scope bar next to "Records" similar to how Timelines > Timelines (instead of
> Timelines > Rendering Frames) does it?

I considered this but wasn't sure what to display for the button text. It could be just "Filter", with the following dropdown menu:

[x] Script
[ ] Canvas
[x] Layout
[x] Paint
[x] Other
-------------
[o] All
[ ] >= 1 ms
[ ] >= 15 ms

(with the second group being radio-style checkboxes)

Another option could be to have the funnel icon bring up the dropdown menu, and move it to the left of the filter text box at the bottom of the sidebar. The icon could still have two states, indicating whether any filtering is active.

[Timothy Hatcher]

I like the colored checkboxes, and we had something like it in the first Timeline panel many years ago.

I do like making them part of the legend. Why not filter the graphs?

Also all other filtering happens in the sidebar, so anything we do should be in the sidebar so the user only has one place to look.

[Matt Baker]

(In reply to comment #9)
> I like the colored checkboxes, and we had something like it in the first
> Timeline panel many years ago.

I'm partial to the colored checkboxes too.

> I do like making them part of the legend. Why not filter the graphs?

We'd need to represent the filtered time somehow, at least in the frame graph. A hollow segment could be added to the top of the frame to account for filtered tasks.

> Also all other filtering happens in the sidebar, so anything we do should be
> in the sidebar so the user only has one place to look.

I agree with keeping all filtering in the sidebar. How about one of the following:

a) Making the doughnut legend item keys checkboxes
b) Adding a button (funnel icon) to the left of the filter text box. Button brings up popup menu described above.

I like a) because it's simple, and we get colored checkboxes. I like b) because it keeps all filtering UI together in one place.

[Matt Baker]

Filtering by frame duration is going under a separate bug: https://bugs.webkit.org/show_bug.cgi?id=147419

[Matt Baker]

Created attachment 258683 [details]
[Patch] Proposed Fix

[Matt Baker]

I scaled back the scope of this patch. Task filtering as implemented in this patch operates on the data grid only, and is limited to showing/hiding grid rows (like other filters: text, timeline ruler).

[Matt Baker]

Created attachment 258690 [details]
[Image] UI - no filtering

[Matt Baker]

Created attachment 258691 [details]
[Image] UI - filter Paint

[Matt Baker]

The checkbox colors look pretty light against the sidebar background, especially compared to the pie chart sections. I'll play with the filter a bit to get better contrast.

Unfortunately it's not as simple as changing the colors passed to the chart, since they're also used to draw the chart.

[Devin Rousso]

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

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

> Source/WebInspectorUI/UserInterface/Views/ChartDetailsSectionRow.css:74
> +.details-section > .content > .group > .row.chart > .chart-content > .legend > .legend-item > label > .color-key,

You could use :matches(.color-key, input[type="checkbox"]) here.

> Source/WebInspectorUI/UserInterface/Views/ChartDetailsSectionRow.js:136
> +            var filterElement = createSVGElement("filter");

We are now starting to use "let" instead of "var", so please replace them in your changes.

> Source/WebInspectorUI/UserInterface/Views/ChartDetailsSectionRow.js:164
> +        var hasCheckboxes = this._items.find(function(element) { return element.checkbox; });

You could probably add this to the loop below inside the (item.checkbox) loop in order to avoid looping through all the elements twice.  It would require another variable/check to make sure you don't add the stylesheet twice, but it could save some loops.

> Source/WebInspectorUI/UserInterface/Views/ChartDetailsSectionRow.js:189
> +                keyElement.__itemId = item.id;

I think we are now moving towards a Symbol based syntax instead of "__".  You could do something like:
WebInspector.ChartDetailsSectionRow.KeyItemIdSymbol = Symbol("chard-details-section-row-key-item-id")

> Source/WebInspectorUI/UserInterface/Views/ChartDetailsSectionRow.js:192
> +                styleSheet.insertRule(".details-section > .content > .group > .row.chart > .chart-content > .legend > .legend-item > label > input[type=checkbox]." + className + " { -webkit-filter: grayscale(1) url(#" + className + ") }", 0);

This feels very inefficient as we will recalculate this almost every time the data is set, which happens almost every time the timeline is refreshed.  It would make more sense to me if you separated this out into a member function that stores a list of existing classNames and if the current checkbox's className is not in the list, then create a new filter and style for that checkbox.  Since we only have 4 filters, although they can change, we shouldn't need to constantly recalculate and recreate these styles and svgFilters.

> Source/WebInspectorUI/UserInterface/Views/ChartDetailsSectionRow.js:196
> +                keyElement.className = "color-key";

We prefer using classList.

> Source/WebInspectorUI/UserInterface/Views/ChartDetailsSectionRow.js:203
> +            valueElement.className = "value";

Ditto from line 196.

> Source/WebInspectorUI/UserInterface/Views/ChartDetailsSectionRow.js:211
> +            legendItemElement.className = "legend-item";

Ditto from line 196.

> Source/WebInspectorUI/UserInterface/Views/ChartDetailsSectionRow.js:221
> +        this.dispatchEventToListeners(WebInspector.ChartDetailsSectionRow.Event.LegendItemChecked, {id: checkbox.__itemId, checked: checkbox.checked});

See line 189.

> Source/WebInspectorUI/UserInterface/Views/TimelineSidebarPanel.js:411
> +                if (!this._renderingFrameTaskFilter.has(taskType) && treeElement.record.durationForTask(taskType) > 0) {

I don't think you need the "> 0" as 0 would be falsy anyways.

[Matt Baker]

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

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

>> Source/WebInspectorUI/UserInterface/Views/ChartDetailsSectionRow.css:74
>> +.details-section > .content > .group > .row.chart > .chart-content > .legend > .legend-item > label > .color-key,
> 
> You could use :matches(.color-key, input[type="checkbox"]) here.

Inspector CSS files for views use absolute selectors to avoid collisions. We don't typically add sheets/rules dynamically, but the same principle applies.

>> Source/WebInspectorUI/UserInterface/Views/ChartDetailsSectionRow.js:164
>> +        var hasCheckboxes = this._items.find(function(element) { return element.checkbox; });
> 
> You could probably add this to the loop below inside the (item.checkbox) loop in order to avoid looping through all the elements twice.  It would require another variable/check to make sure you don't add the stylesheet twice, but it could save some loops.

The perf impact is likely negligible, but I think it reads better as a check in the for loop so I'm happy to move it back.

>> Source/WebInspectorUI/UserInterface/Views/TimelineSidebarPanel.js:411
>> +                if (!this._renderingFrameTaskFilter.has(taskType) && treeElement.record.durationForTask(taskType) > 0) {
> 
> I don't think you need the "> 0" as 0 would be falsy anyways.

Good catch.

[Matt Baker]

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

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

>> Source/WebInspectorUI/UserInterface/Views/ChartDetailsSectionRow.js:196
>> +                keyElement.className = "color-key";
> 
> We prefer using classList.

Seems verbose for simple cases, especially when the element's scope is quite limited, but I'm fine changing over for consistently if it's becoming standard practice.

[Matt Baker]

Created attachment 258757 [details]
[Patch] Proposed Fix

[Devin Rousso]

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

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

> Source/WebInspectorUI/UserInterface/Views/ChartDetailsSectionRow.js:128
> +            if (this._svgFiltersElement.childNodes[i].id === className)

This is really confusing.  I would either add a comment explaining why your variables are named this way or change className to filterElementType (or something like that).

> Source/WebInspectorUI/UserInterface/Views/ChartDetailsSectionRow.js:138
> +        filterElement.id = className;

See line 128.

All good otherwise.

[Matt Baker]

Created attachment 258782 [details]
[Patch] Proposed Fix

[WebKit Commit Bot]

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

Clearing flags on attachment: 258782

Committed r188321: <http://trac.webkit.org/changeset/188321>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.