1109764 - Selecting a details view pane should show the button as "checked"

Status: RESOLVED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect)

Description

[Jordan Santell [:jsantell] [@jsantell]]

When toggling panes in the details view (calltree, waterfall), the buttons do not reflect what pane is currently selected, and have no "checked" or "on" state.

Comment 1

[Tim Nguyen :ntim]

When I tested the patch in bug 1077458, it did have a checked state.

Comment 2

[Jordan Santell [:jsantell] [@jsantell]]

Yeah, I thought it was working previously too -- maybe some of the newer CSS changes undid this? Should probably add a test for state selection due to this

Comment 3

[Tim Nguyen :ntim]

I don't think it's related to recent CSS changes, since the checked state works fine for the record button.

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1109764-fix-button-toggle-perf.patch

Ah, looks like during the last steps of reviews, we changed it to a deckbox and lost some of the checked features by default.

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2ee196d15494

Comment 6

[Victor Porof [:vporof][:vp]]

Comment on attachment 8534635 [details] [diff] [review]
1109764-fix-button-toggle-perf.patch

Review of attachment 8534635 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/performance/test/browser_perf-details.js
@@ +30,5 @@
>  }
>  
>  function checkViews (DetailsView, doc, currentView) {
> +  for (let viewName in DetailsView.viewIndexes) {
> +    let button = doc.querySelector(`toolbarbutton[data-view="${viewName}"]`);

Very nice.

::: browser/devtools/performance/views/details.js
@@ +50,5 @@
> +      if (button.getAttribute("data-view") === selectedView)
> +        button.setAttribute("checked", true);
> +      else
> +        button.removeAttribute("checked");
> +    }

Hipster nit: a <tabbox> seems more semantic here, instead of "buttons above a deck", but you really shouldn't listen to me.

Comment 7

[Jordan Santell [:jsantell] [@jsantell]]

<tabbox> could work too, but figured we'd also have other options in that toolbar as well, so this'll give us flexibility until we iron out exactly everything we need. Although we do have several areas with several subviews each, so we should probably make all that consistent atleast. We should review that once both OVERVIEW and DETAILS have subviews.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/81ffd30de127

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/81ffd30de127