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

Status: VERIFIED FIXED

(DevTools :: Framework, defect)

Description

[Brian Grinstead [:bgrins]]

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.

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: 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.

Comment 2

[Girish Sharma [:Optimizer]]

The initial patch for the options panel had this feature :) - A good place to start with ( bug 851546 obselete patches and screenshots)

Comment 3

[Ben]

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.

Comment 4

[Ben]

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.

Comment 5

[Brian Grinstead [:bgrins]]

(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.

Comment 6

[Brian Grinstead [:bgrins]]

Attachment: toolbox-button-hide.patch

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.

Comment 7

[Brian Grinstead [:bgrins]]

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 8

[Joe Walker [:jwalker]]

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?

Comment 9

[Girish Sharma [:Optimizer]]

(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.

Comment 10

[Joe Walker [:jwalker]]

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

Comment 11

[Brian Grinstead [:bgrins]]

(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.

Comment 12

[Girish Sharma [:Optimizer]]

(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.

Comment 13

[Brian Grinstead [:bgrins]]

(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 14

[Brian Grinstead [:bgrins]]

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

Comment 15

[Brian Grinstead [:bgrins]]

Attachment: toolbox-button-hide.patch

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.

Comment 16

[Brian Grinstead [:bgrins]]

Attachment: toolbox-buttons-toggle.png

Comment 17

[Girish Sharma [:Optimizer]]

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.

Comment 18

[Brian Grinstead [:bgrins]]

(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.

Comment 19

[Girish Sharma [:Optimizer]]

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.

Comment 20

[Brian Grinstead [:bgrins]]

(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.

Comment 21

[Girish Sharma [:Optimizer]]

(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.

Comment 22

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Adding dev-doc-needed, so that we can ensure we update docs about any soon to be hidden buttons.

Comment 23

[Brian Grinstead [:bgrins]]

Attachment: 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.

Comment 24

[Brian Grinstead [:bgrins]]

(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

Comment 25

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/integration/fx-team/rev/4652f1cda834
https://tbpl.mozilla.org/?tree=Fx-Team&rev=4652f1cda834

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4652f1cda834

Comment 27

[Alex Bell]

I'm not seeing these new prefs in Nightly. Am I missing something?

Comment 28

[Brian Grinstead [:bgrins]]

(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

Comment 29

[Tim Nguyen :ntim]

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 ?

Comment 30

[Tim Nguyen :ntim]

Or it could create a devtools.toolbox.disabled-command-buttons. And the value would be an array.

Comment 31

[Will Bamberg [:wbamberg]]

Now documented here -> https://developer.mozilla.org/en-US/docs/Tools_Toolbox#Settings