Web Inspector: Can't select last row in the timeline because it's covered by filter selector

[Nikita Vasilyev]

Created attachment 256138 [details]
Animated GIF of the problem

A straightforward solution would be to add an empty row at the bottom of the data grid.

[Radar WebKit Bug Importer]

<rdar://problem/21673208>

[Devin Rousso]

Created attachment 256358 [details]
Patch

[Timothy Hatcher]

Comment on attachment 256358 [details]
Patch

Screenshot please.

[Timothy Hatcher]

I applied the patch to try it out. Neat!

I was imaging replacing the label with the scope picker. So center it and change "All" to include the label, like "All Resources".

Though, JavaScript & Events does not have a filter. So maybe your design is better.

[Timothy Hatcher]

Comment on attachment 256358 [details]
Patch

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

Looks good!

> Source/WebInspectorUI/ChangeLog:35
> [phone]  Devin Rousso  <[e-mail]>
> +
> +        Web Inspector: Pressing tab on a comment in the styles sidebar doesn't highlight the comment
> +        https://bugs.webkit.org/show_bug.cgi?id=146709

Oops.

> Source/WebInspectorUI/UserInterface/Views/LayoutTimelineView.js:46
>      columns.eventType.scopeBar = WebInspector.TimelineDataGrid.createColumnScopeBar("layout", typeToLabelMap);

I don't think you need to assign to columns.eventType.scopeBar anymore.

> Source/WebInspectorUI/UserInterface/Views/NetworkTimelineView.js:49
>      columns.type.scopeBar = WebInspector.TimelineDataGrid.createColumnScopeBar("network", typeToLabelMap);

Ditto.

> Source/WebInspectorUI/UserInterface/Views/TimelineDataGrid.js:52
> +    if (this._filterableColumns.length)
>          this._updateScopeBarForcedVisibility();

This shouldn't be needed now, right?

> Source/WebInspectorUI/UserInterface/Views/TimelineDataGrid.js:81
> -    scopeBarItems.unshift(new WebInspector.ScopeBarItem(prefix + "type-all", WebInspector.UIString("All"), true));
> +    scopeBarItems.unshift(new WebInspector.ScopeBarItem(prefix + "type-all", WebInspector.UIString("All")));

Why not exclusive anymore? I'd think it should be still.

> Source/WebInspectorUI/UserInterface/Views/TimelineDataGrid.js:83
> -    return new WebInspector.ScopeBar(prefix + "scope-bar", scopeBarItems, scopeBarItems[0]);
> +    return new WebInspector.ScopeBar(prefix + "scope-bar", scopeBarItems, scopeBarItems[0], true);

Why are these exclusive?

> Source/WebInspectorUI/UserInterface/Views/TimelineDataGrid.js:137
>      updateLayout: function()
>      {
>          WebInspector.DataGrid.prototype.updateLayout.call(this);
> -
> -        if (this._navigationBar)
> -            this._navigationBar.updateLayout();
>      },

Can remove the whole override now.

> Source/WebInspectorUI/UserInterface/Views/TimelineSidebarPanel.css:145
> +.sidebar > .panel.navigation.timeline > .title-bar.timeline-events > .title-bar-scope-selector {

title-bar-scope-bar

> Source/WebInspectorUI/UserInterface/Views/TimelineSidebarPanel.css:148
> +    right: 0;

Add font-weight: normal to remove the inherited bold text.

I think we also might want to drop the blue background and have black text and arrows when "All" is selected. Having it be blue when a filter is applied will make it more obvious, and that eye catching blue is distracting when All is selected. (This would be different if All was a separate scope item like Resources sidebar or Network. I think combining it is good though.)

> Source/WebInspectorUI/UserInterface/Views/TimelineSidebarPanel.js:42
> +        this._timelineEventsTitleBarScopeSelector = document.createElement("div");

I'd call this Container not Selector.

> Source/WebInspectorUI/UserInterface/Views/TimelineSidebarPanel.js:310
> +    set contentTreeOutlineScopeSelector(element)

I think a contentTreeOutlineScopeBar would be a better setter, then get the element to append from the scopeBar.

A getter would be nice too, even if not needed now.

> Source/WebInspectorUI/UserInterface/Views/TimelineSidebarPanel.js:963
> +WebInspector.TimelineSidebarPanel.TitleBarScopeSelectorStyleClass = "title-bar-scope-selector";

title-bar-scope-bar

> Source/WebInspectorUI/UserInterface/Views/TimelineView.js:69
> +    get navigationSidebarTreeOutlineScopeSelector()

navigationSidebarTreeOutlineScopeBar

> Source/WebInspectorUI/UserInterface/Views/TimelineView.js:74
> +        return this._scopeBar.element;

Return the ScopeBar.

[Devin Rousso]

Comment on attachment 256358 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/LayoutTimelineView.js:46
>>      columns.eventType.scopeBar = WebInspector.TimelineDataGrid.createColumnScopeBar("layout", typeToLabelMap);
> 
> I don't think you need to assign to columns.eventType.scopeBar anymore.

This is actually used later to add the change event listener for that particular column.  From the comments in the code, it looks like it is planned to allow multiple filters on different columns, and this syntax would be used to make that work.

>> Source/WebInspectorUI/UserInterface/Views/TimelineDataGrid.js:81
>> +    scopeBarItems.unshift(new WebInspector.ScopeBarItem(prefix + "type-all", WebInspector.UIString("All")));
> 
> Why not exclusive anymore? I'd think it should be still.

According to your comment below, I made this non-exclusive and a different color when it is selected.

>> Source/WebInspectorUI/UserInterface/Views/TimelineDataGrid.js:83
>> +    return new WebInspector.ScopeBar(prefix + "scope-bar", scopeBarItems, scopeBarItems[0], true);
> 
> Why are these exclusive?

This isn't exclusive.  This says that anything non-exclusive is grouped together into a select element.

[Devin Rousso]

Created attachment 256394 [details]
Patch

[Timothy Hatcher]

Comment on attachment 256394 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/TimelineSidebarPanel.css:157
> +.sidebar > .panel.navigation.timeline > .title-bar.timeline-events > .title-bar-scope-bar > .default-item-selected > .multiple path {

path should be "svg .stroked". You should also add:

.sidebar > .panel.navigation.timeline > .title-bar.timeline-events > .title-bar-scope-bar > .default-item-selected > .multiple svg .filled {
    fill: black !important;
}

[Devin Rousso]

Created attachment 256402 [details]
Patch

[Timothy Hatcher]

Comment on attachment 256402 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/TimelineSidebarPanel.css:161
> +.sidebar > .panel.navigation.timeline > .title-bar.timeline-events > .title-bar-scope-bar > .default-item-selected > .multiple svg path.stroked {

Not path, it could be a circle or rect, etc. So just: "svg .stroked"

[Devin Rousso]

Created attachment 256411 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 256411 [details]
Patch

Clearing flags on attachment: 256411

Committed r186536: <http://trac.webkit.org/changeset/186536>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.