Closed Bug 974947 Opened 6 years ago Closed 6 years ago

Add preferences in options panel to hide command buttons on DevTools tabbar

Categories

(DevTools :: Framework, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 30

People

(Reporter: bgrins, Assigned: bgrins)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(3 files, 2 obsolete files)

We should allow a user to disable at least some of the command buttons in case they would like to free up some space on the tab bar.

And we should have a checkbox on the options panel, like the optional tabs.
Attached image tab-bar.png
We have 6 command buttons on right now.  I think we could make four of them toggle-able: paint flashing, tilt, and scratchpad, responsive design.  Of these, I think responsive design should be visible by default while the others should be hidden.
The initial patch for the options panel had this feature :) - A good place to start with ( bug 851546 obselete patches and screenshots)
Please also make JSscratchpad and inspect buttons toggle-able as well. Basically everything. People work differently--so just open it all up and set reasonable defaults.
See Also: → 939040
I like the idea of a solid color picker that works globally (essentially the Colorzilla add-on brought to native code), but I would prefer to access it either through a swatch in the CSS inspector, or some sort of contextual menu. Please not another dedicated command button that can't be toggled out of the UI.
(In reply to Ben from comment #4)
> I like the idea of a solid color picker that works globally (essentially the
> Colorzilla add-on brought to native code), but I would prefer to access it
> either through a swatch in the CSS inspector, or some sort of contextual
> menu. Please not another dedicated command button that can't be toggled out
> of the UI.

Did you mean to add this comment in Bug 939040?  I agree with the point though.
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Summary: Add preferences to hide command buttons on DevTools tabbar → Add preferences in options panel to hide command buttons on DevTools tabbar
Attached patch toolbox-button-hide.patch (obsolete) — Splinter Review
Joe, can you have a look at this patch?  It toggles the visibility of the available command buttons based on prefs, and provides a UI in the options panel to do so.
Attachment #8383232 - Flags: review?(jwalker)
Comment on attachment 8383232 [details] [diff] [review]
toolbox-button-hide.patch

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

::: browser/app/profile/firefox.js
@@ +1112,5 @@
>  pref("devtools.toolbox.toolbarSpec", '["splitconsole", "paintflashing toggle","tilt toggle","scratchpad","resize toggle"]');
>  pref("devtools.toolbox.sideEnabled", true);
>  pref("devtools.toolbox.zoomValue", "1");
>  
> +// Command Button preferences

You will also notice that I use the term Toolbox Button in most of the rest of the code - this is because I was getting confused expecting command button to map neatly to gcli commands - but not all of them are such, like the command-button-pick.

That said, I'd be happy to change the code back to using command button if that makes the most sense.  We do still call the non-gcli button a command button in the DOM.
Comment on attachment 8383232 [details] [diff] [review]
toolbox-button-hide.patch

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

Looks good.

::: browser/app/profile/firefox.js
@@ +1112,5 @@
>  pref("devtools.toolbox.toolbarSpec", '["splitconsole", "paintflashing toggle","tilt toggle","scratchpad","resize toggle"]');
>  pref("devtools.toolbox.sideEnabled", true);
>  pref("devtools.toolbox.zoomValue", "1");
>  
> +// Command Button preferences

I'm not sure that we have a policy on the right way to handle these buttons, but I think we should make one.

::: browser/devtools/framework/toolbox-options.js
@@ +90,5 @@
> +    let setToolboxButtonsVisibility =
> +      this.toolbox.setToolboxButtonsVisibility.bind(this.toolbox);
> +
> +    let onCheckboxClick = function(id) {
> +      let toolDefinition = toggleableButtons.filter(tool=>tool.id === id)[0];

There's a '=>' (which implies a 'good' binding) inside a 'function' (which messes up the binding).
I think it's probably better to make the function a =>
?

::: browser/devtools/framework/toolbox.js
@@ +580,5 @@
> +    ].map(id => {
> +      let button = this.doc.getElementById(id);
> +      return {
> +        id: id,
> +        label: button.getAttribute("tooltiptext"),

Do you need label (and the button variable)?

@@ +593,5 @@
> +   */
> +  setToolboxButtonsVisibility: function() {
> +    this.toolboxButtons.forEach(buttonSpec => {
> +      let {visibilityswitch, id}=buttonSpec;
> +      let node = this.doc.getElementById(id);

Why not put the node in the toolboxButtons data?
Attachment #8383232 - Flags: review?(jwalker) → review+
(Fly by comments)

Why are the citations class styles removed ?

Also, the citation label should be the last thing vertically in the column. So buttons should be above it.
I think we should make sure everyone has had a chance to weigh in on the buttons issue from comment 1.

My opinions:

* paint flashing: should be off by default
* tilt: I'd like to see us use tilt to visualize z-index hovers in the inspector, and that would be a good time to have it off by default.
* scratchpad: It would be interesting to see telemetry numbers, but my hunch is that we shouldn't turn it off by default
* responsive design: On by default
(In reply to Girish Sharma [:Optimizer] from comment #9)
> (Fly by comments)
> 
> Why are the citations class styles removed ?
> 
> Also, the citation label should be the last thing vertically in the column.
> So buttons should be above it.

The citation styles themselves are still there - this is removing a second rule that applies italic text to the first label sibling after a citation which AFAICT is never matched in the current layout.

I thought it looked a little weird to move the citation text all the way below the list of options, since it was really only referring to the scratchpad box.  Even if we did move the citation below, I don't think that particular rule is needed, unless if I'm missing something.
(In reply to Brian Grinstead [:bgrins] from comment #11)
> (In reply to Girish Sharma [:Optimizer] from comment #9)
> > (Fly by comments)
> > 
> > Why are the citations class styles removed ?
> > 
> > Also, the citation label should be the last thing vertically in the column.
> > So buttons should be above it.
> 
> The citation styles themselves are still there - this is removing a second
> rule that applies italic text to the first label sibling after a citation
> which AFAICT is never matched in the current layout.

Ah, you are right. Previously, there were different symbols for citations, and the labels were after the symbols. Which got changed later on.


> I thought it looked a little weird to move the citation text all the way
> below the list of options, since it was really only referring to the
> scratchpad box.  Even if we did move the citation below, I don't think that
> particular rule is needed, unless if I'm missing something.

If you have addons, they will also have the * if the target is not supported. Plus, generally, all the citations explanations are at the bottom of the page, so I think we should also have all the content first and then the citation explanation.
(In reply to Girish Sharma [:Optimizer] from comment #12)
> (In reply to Brian Grinstead [:bgrins] from comment #11)
> > (In reply to Girish Sharma [:Optimizer] from comment #9)
> > > (Fly by comments)
> > > 
> > > Why are the citations class styles removed ?
> > > 
> > > Also, the citation label should be the last thing vertically in the column.
> > > So buttons should be above it.
> > 
> > The citation styles themselves are still there - this is removing a second
> > rule that applies italic text to the first label sibling after a citation
> > which AFAICT is never matched in the current layout.
> 
> Ah, you are right. Previously, there were different symbols for citations,
> and the labels were after the symbols. Which got changed later on.
> 
> 
> > I thought it looked a little weird to move the citation text all the way
> > below the list of options, since it was really only referring to the
> > scratchpad box.  Even if we did move the citation below, I don't think that
> > particular rule is needed, unless if I'm missing something.
> 
> If you have addons, they will also have the * if the target is not
> supported. Plus, generally, all the citations explanations are at the bottom
> of the page, so I think we should also have all the content first and then
> the citation explanation.

OK, move the citation to the bottom and still going to remove the CSS rule.
Comment on attachment 8383232 [details] [diff] [review]
toolbox-button-hide.patch

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

::: browser/devtools/framework/toolbox-options.js
@@ +90,5 @@
> +    let setToolboxButtonsVisibility =
> +      this.toolbox.setToolboxButtonsVisibility.bind(this.toolbox);
> +
> +    let onCheckboxClick = function(id) {
> +      let toolDefinition = toggleableButtons.filter(tool=>tool.id === id)[0];

It was doing some context binding when adding the event listener.  I've updated to pass checkbox as an argument instead and use a fat arrow

::: browser/devtools/framework/toolbox.js
@@ +580,5 @@
> +    ].map(id => {
> +      let button = this.doc.getElementById(id);
> +      return {
> +        id: id,
> +        label: button.getAttribute("tooltiptext"),

I'm using that label inside of the toolbox-options.js file to populate the label next to each checkbox, since there wasn't any other internationalized strings for each button.

@@ +593,5 @@
> +   */
> +  setToolboxButtonsVisibility: function() {
> +    this.toolboxButtons.forEach(buttonSpec => {
> +      let {visibilityswitch, id}=buttonSpec;
> +      let node = this.doc.getElementById(id);

Updated to pass this with toolboxButtons
Attached patch toolbox-button-hide.patch (obsolete) — Splinter Review
Updated based on comments.  Also moved citation label to the bottom of the box in the UI.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=3a6e838e3ed9


> I think we should make sure everyone has had a chance to weigh in on the
> buttons issue from comment 1.

I will send a message to the mailing list about this.
Attachment #8383232 - Attachment is obsolete: true
Attachment #8383675 - Flags: review+
Also, we will have to either hide the toolbox buttons group completely or add an * on all but inspect button's checkboxes in case of Browser Toolbox where other buttons are not available at all.
(In reply to Girish Sharma [:Optimizer] from comment #17)
> Also, we will have to either hide the toolbox buttons group completely or
> add an * on all but inspect button's checkboxes in case of Browser Toolbox
> where other buttons are not available at all.

Do you know of an easy way to check from the options panel if we are in Browser Toolbox mode?  I'd be fine with just hiding the buttons.
The GCLI command buttons are not available for all remote targets, so you can just check for target.isLocalTab.

Joe, please correct me if I am wrong.
(In reply to Girish Sharma [:Optimizer] from comment #19)
> The GCLI command buttons are not available for all remote targets, so you
> can just check for target.isLocalTab.
> 
> Joe, please correct me if I am wrong.

I'm pretty sure this will give false positives when connecting to a remote device.
(In reply to Brian Grinstead [:bgrins] from comment #20)
> (In reply to Girish Sharma [:Optimizer] from comment #19)
> > The GCLI command buttons are not available for all remote targets, so you
> > can just check for target.isLocalTab.
> > 
> > Joe, please correct me if I am wrong.
> 
> I'm pretty sure this will give false positives when connecting to a remote
> device.

Why do you say so ? isLocalTab should only be true when the tab is local (ie normal firefox devtools) Any kind of remote actual connection has it false.
Adding dev-doc-needed, so that we can ensure we update docs about any soon to be hidden buttons.
Keywords: dev-doc-needed
Actually, browser toolbox was automatically working once adding a check to make sure the button exists before adding it into the toolboxButtons array.  In this case it will add only "Pick an element from the page" to the list.
Attachment #8383675 - Attachment is obsolete: true
Attachment #8383919 - Flags: review+
(In reply to Brian Grinstead [:bgrins] from comment #23)
> Created attachment 8383919 [details] [diff] [review]
> toolbox-button-hide.patch
> 
> Actually, browser toolbox was automatically working once adding a check to
> make sure the button exists before adding it into the toolboxButtons array. 
> In this case it will add only "Pick an element from the page" to the list.

Interdiff from last patch: http://benjamin.smedbergs.us/interdiff/interdiff.php?patch1url=https%3A%2F%2Fbug974947.bugzilla.mozilla.org%2Fattachment.cgi%3Fid%3D8383675&patch2url=https%3A%2F%2Fbug974947.bugzilla.mozilla.org%2Fattachment.cgi%3Fid%3D8383919
https://hg.mozilla.org/mozilla-central/rev/4652f1cda834
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30
I'm not seeing these new prefs in Nightly. Am I missing something?
(In reply to Alex Bell from comment #27)
> I'm not seeing these new prefs in Nightly. Am I missing something?

I do not think this has yet made it into Nightly -- it should make it into next one.  Once it has, the following prefs should be added:

devtools.command-button-pick.enabled
devtools.command-button-splitconsole.enabled
devtools.command-button-paintflashing.enabled
devtools.command-button-tilt.enabled
devtools.command-button-scratchpad.enabled
devtools.command-button-responsive.enabled
I think it's a bit strange to use seperate prefs for each command button.
Why doesn't this edit the devtools.toolbox.toolbarSpec pref instead ?
Or it could create a devtools.toolbox.disabled-command-buttons. And the value would be an array.
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.