882353 - Reduce duplication of updateWidgetStyle in CustomizableWidgets.jsm

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

The function for updateWidgetStyle in CustomizableWidgets.jsm is duplicated for the zoom-controls and edit-controls.

It varies only slightly in the zoom-controls case by setting the reset button hidden if it's in the panel and also setting the noautoclose attribute. We should be able to remove the duplication but keep the same behavior.

Comment 1

[:Gijs (he/him)]

To fix this bug, the "updateWidgetStyle" function should be moved outside of the two widgets that currently have one, possibly be renamed to something like "updateCombinedWidgetStyle" and take care of both the zoom controls and edit controls correctly. Here are the original functions:

http://mxr.mozilla.org/projects-central/source/ux/browser/components/customizableui/src/CustomizableWidgets.jsm?rev=9737cc4b6837#339

http://mxr.mozilla.org/projects-central/source/ux/browser/components/customizableui/src/CustomizableWidgets.jsm?rev=9737cc4b6837#461

Comment 2

[Darshana Padmadas]

Hello!
I'd like to get started on this bug.Could you assign this to me?

Comment 3

[:Gijs (he/him)]

(In reply to Darshana Padmadas from comment #2)
> Hello!
> I'd like to get started on this bug.Could you assign this to me?

Done! Let me know if you have any questions. :-)

Comment 4

[Darshana Padmadas]

Attachment: output.txt

Comment 5

[:Gijs (he/him)]

Comment on attachment 826763 [details] [diff] [review]
output.txt

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

This is close, but to be honest I have no idea how it worked - the attrs.noautoclose setting can't be doing the right thing if you're setting it before declaring attrs. Please apply the following corrections and submit a new version of the patch.

::: browser/components/customizableui/src/CustomizableWidgets.jsm
@@ +38,4 @@
>    }
>  }
>  
> +

Nit: there's already spacing here, no need to add extra lines.

@@ +39,5 @@
>  }
>  
> +
> +
> +function updateCombinedWidgetStyle(aArea,modifyAutoclose) {

Nit: space before "modifyAutoclose"

Nit: please use "aModifyAutoclose" as the name for this parameter

@@ +40,5 @@
>  
> +
> +
> +function updateCombinedWidgetStyle(aArea,modifyAutoclose) {
> +        let inPanel = (aArea == CustomizableUI.AREA_PANEL);

Nit: Please use two spaces as indentation. Don't use tabs, don't use 8 spaces or whatever this is.

@@ +41,5 @@
> +
> +
> +function updateCombinedWidgetStyle(aArea,modifyAutoclose) {
> +        let inPanel = (aArea == CustomizableUI.AREA_PANEL);
> +	if (modifyAutoclose){

Nit: space before {

@@ +42,5 @@
> +
> +function updateCombinedWidgetStyle(aArea,modifyAutoclose) {
> +        let inPanel = (aArea == CustomizableUI.AREA_PANEL);
> +	if (modifyAutoclose){
> +	  attrs.noautoclose = inPanel? true:null;

Nit: space before "?" and around ":"

@@ +47,5 @@
> +	}
> +        let cls = inPanel ? "panel-combined-button" : "toolbarbutton-1";
> +        if (!aArea)
> +          cls = null;
> +        let attrs = {class: cls};

This won't work. You should declare and set attrs before setting the noautoclose property on it. Move the if (modifyAutoclose) block down to below this line.

@@ +365,4 @@
>          updateZoomResetButton();
>        }
>  
> +      

Nit: Please remove this empty line.

@@ +373,5 @@
>  
> +          updateCombinedWidgetStyle(aArea,true);
> +	  updateZoomResetButton();
> +	  
> +	  

Nit: Please remove these two extra empty lines

@@ +392,4 @@
>  
>            // When a widget is demoted to the palette ('removed'), it's visual
>            // style should change.
> +          updateCombinedWidgetStyle();

If you do this, the attribute won't get removed. You should pass (null, true) as arguments.

@@ +399,5 @@
>          onWidgetReset: function(aWidgetId) {
>            if (aWidgetId != this.id)
>              return;
> +          updateCombinedWidgetStyle(this.currentArea,true);
> +	   updateZoomResetButton();

Nit: please align this with the line above.

@@ +406,5 @@
>          onWidgetMoved: function(aWidgetId, aArea) {
>            if (aWidgetId != this.id)
>              return;
> +          updateCombinedWidgetStyle(aArea,true);
> +	   updateZoomResetButton();

Nit: please align this with the line above.

Comment 6

[Darshana Padmadas]

Attachment: new_output.txt

Comment 7

[:Gijs (he/him)]

https://hg.mozilla.org/projects/ux/rev/8e9d485dd741

Comment 8

[:Gijs (he/him)]

Comment on attachment 827212 [details] [diff] [review]
new_output.txt

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

Great! There were still a bunch of whitespace changes that needed correcting, but I've gone ahead and done that for you and landed the patch. Thank you! :-)

Comment 9

[:Gijs (he/him)]

This doesn't need to be checked in, I already did so. See comment 7 and comment 8.

Comment 10

[Darshana Padmadas]

Alright.Thanks for all your help. :)

Comment 11

[:Gijs (he/him)]

So both of us completely missed that you kind of need to pass node to this function, because in that scope node doesn't exist. Followup fix landed:

https://hg.mozilla.org/projects/ux/rev/5f73d1f59d3b

Comment 12

[Darshana Padmadas]

Oh I'm so sorry I missed that!Has this issue been fixed now? Any more changes to be made?

Comment 13

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/8e9d485dd741
https://hg.mozilla.org/mozilla-central/rev/5f73d1f59d3b