Bug 132202 – Web Inspector: clicking Timelines tree view nodes should not change the current content view

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

[Brian Burg]

It's exceptionally hard to select/expand/contract the tree view disclosures. If the user wants to use the keyboard arrows, or mis-clicks on the disclosure triangle, then they are whisked away to a different content view. I've gotten really frustrated by this when trying to expand deep trees.

I think that behavior could be moved to command/ctrl + click. Some of the rows also have go-to arrows. We could add more of those if it's hard to view resources otherwise.

[Radar WebKit Bug Importer]

<rdar://problem/16728603>

[Timothy Hatcher]

FWIW, clicking a row in the content view works, and keyboard navigation (expand and contract too) from there works.

I'm not opposed to doing something different here. Maybe go-to arrows is right.

[Brian Burg]

(In reply to comment #2)
> FWIW, clicking a row in the content view works, and keyboard navigation (expand and contract too) from there works.

Yeah, I figured it out eventually. But if you are staring at the text in the sidebar with the intent of exploring subtees (by looking at where the disclosures are), it's a rude surprise.

> I'm not opposed to doing something different here. Maybe go-to arrows is right.

Another thing I raised with Joe on IRC is that this scheme would let us highlight data points for a specific row on the timeline, when the relevant row is selected.

[Matt Baker]

Created attachment 251206 [details]
[Patch] Proposed Fix

[Brian Burg]

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

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

Nice work!

> Source/WebInspectorUI/UserInterface/Views/LayoutTimelineView.js:35
> +    var columns = {eventType: {}, location: {}, width: {}, height: {}, startTime: {}, totalTime: {}};

Was this actually functionally broken, or just not matching column names of other TimelineView subclasses?

> Source/WebInspectorUI/UserInterface/Views/NetworkTimelineView.js:158
> +        console.error("Unknown tree element selected.");

Please log treeElement as a second parameter. It eliminates a debugging step that you would always have to do to investigate this error message.

> Source/WebInspectorUI/UserInterface/Views/TimelineView.js:191
> +            console.error("Unknown tree element selected.");

As above.

> Source/WebInspectorUI/UserInterface/Views/TimelineView.js:249
> +        if (this.navigationSidebarTreeOutline.selectedTreeElement)

The existing logic is fine for this patch. I related an existing bug that proposes improving the logic.

[Matt Baker]

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

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

>> Source/WebInspectorUI/UserInterface/Views/LayoutTimelineView.js:35
>> +    var columns = {eventType: {}, location: {}, width: {}, height: {}, startTime: {}, totalTime: {}};
> 
> Was this actually functionally broken, or just not matching column names of other TimelineView subclasses?

I broke this in https://trac.webkit.org/changeset/182660.

[Matt Baker]

Created attachment 251357 [details]
[Patch] Proposed Fix

[WebKit Commit Bot]

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

Clearing flags on attachment: 251357

Committed r183134: <http://trac.webkit.org/changeset/183134>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.