Closed Bug 1109764 Opened 8 years ago Closed 8 years ago

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

Categories

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

37 Branch
x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 37

People

(Reporter: jsantell, Assigned: jsantell)

References

Details

Attachments

(1 file)

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.
When I tested the patch in bug 1077458, it did have a checked state.
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
I don't think it's related to recent CSS changes, since the checked state works fine for the record button.
Ah, looks like during the last steps of reviews, we changed it to a deckbox and lost some of the checked features by default.
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Attachment #8534635 - Flags: review?(vporof)
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.
Attachment #8534635 - Flags: review?(vporof) → review+
<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.
https://hg.mozilla.org/mozilla-central/rev/81ffd30de127
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 37
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.