Bug 144539 – REGRESSION: Web Inspector: no way to navigate to a resource/source location from overview timeline view

• Component: Web Inspector
• Status: RESOLVED FIXED

[Brian Burg]

Created attachment 252256 [details]
Can't navigate to any of these files or mentioned lines.

Right now you have to go to the specific timeline view, then use the go-to arrow there. It would be good to show that on the overview timeline view rows as well.

[Radar WebKit Bug Importer]

<rdar://problem/20792727>

[Timothy Hatcher]

Yes, these should get goto arrows. We should be consistent and put goto arrows in the sidebar column for anything with a location.

[Timothy Hatcher]

Created attachment 252381 [details]
Patch

[Timothy Hatcher]

Created attachment 252382 [details]
Screenshot with Patch

[Brian Burg]

Comment on attachment 252381 [details]
Patch

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

r=me

The overall approach is good. If you can find a cleaner way to determine whether to show arrows, then even better!

> Source/WebInspectorUI/ChangeLog:59
> +        (.item > .status > .status-button): Fix an alignment issues with close and go-to arrows being side-by-side.

issues -> issue

> Source/WebInspectorUI/UserInterface/Views/TimelineSidebarPanel.css:196
> +.sidebar > .panel.navigation.timeline .item:not(:hover, .selected) .status .status-button,

Have you experimented with showing a very dim go-to arrow when not hovered? I am a little worried about discoverability.

> Source/WebInspectorUI/UserInterface/Views/TimelineSidebarPanel.js:364
> +        if (!(treeElement instanceof WebInspector.ResourceTreeElement || treeElement instanceof WebInspector.ScriptTreeElement ||

This hardcoded list makes me nervous.. is there a programmatic way to determine whether the go-to arrow should be created? For example, is the existence of an overridden showContentViewForTreeElement sufficient to make a go-to arrow? What about canShowDifferentContentView?

> Source/WebInspectorUI/UserInterface/Views/TimelineSidebarPanel.js:375
> +            // Only create a go-to button for tree elements that can show a different content view when selected.

So, for every tree element that could show a content view, we create a hidden close element for every tree element and add listeners eagerly. It seems like this hardcoded list is duplicated then, no?

[Brian Burg]

Comment on attachment 252381 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/TimelineSidebarPanel.css:196
>> +.sidebar > .panel.navigation.timeline .item:not(:hover, .selected) .status .status-button,
> 
> Have you experimented with showing a very dim go-to arrow when not hovered? I am a little worried about discoverability.

Or, maybe sticking with the visual language used (inconsistently) elsewhere and additionally underline the source code location text, to make it look like a link when not hovering the row.

[Timothy Hatcher]

(In reply to comment #6)
> Comment on attachment 252381 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=252381&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Views/TimelineSidebarPanel.css:196
> >> +.sidebar > .panel.navigation.timeline .item:not(:hover, .selected) .status .status-button,
> > 
> > Have you experimented with showing a very dim go-to arrow when not hovered? I am a little worried about discoverability.
> 
> Or, maybe sticking with the visual language used (inconsistently) elsewhere
> and additionally underline the source code location text, to make it look
> like a link when not hovering the row.

In using it I think it is fine. We use hover for other go-to arrows. And we force it to show when the row is selected.

[Timothy Hatcher]

Comment on attachment 252381 [details]
Patch

https://trac.webkit.org/r183821