870897 - Make Zoom control customizable

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

The -/100%/+ button in the panel needs to be able to be removed.

When it goes into a toolbar, it should be styled like the current zoom toolbar button (a single toolbar item with a - and + button). It should only go into wide-mode (as it currently is) in the panel.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

I should also note that this change should also remove the old zoom toolbaritem.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Hey Blair,

To make this widget, we've got 2 choices as far as I can tell:

1) We can modify the old-school XUL widget to appear and work differently in the menu panel. This is probably lowest effort, and just means modifying the current widget.
2) We can modify createWidget to allow us to create more sophisticated widgets. createWidget only lets us create single toolbarbuttons at the moment, but the zoom widget is a more complex beast - it's a toolbaritem with several buttons inside of it.

We could give createWidget more powers by, for example, allowing the caller to pass a "makeWidget" function as part of the propreties object that takes the document, and returns a programmatically created node for CustomizableUI to inject into the windows.

Was createWidget ever supposed to have that kind of flexibility? Is there a requirement that says that createWidget should only return toolbarbuttons?

-Mike

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Mike Conley (:mconley) from comment #2)
> Was createWidget ever supposed to have that kind of flexibility?

Yes.

> Is there a
> requirement that says that createWidget should only return toolbarbuttons?

No.

My original thinking was to allow it to create several different types of widgets (normal button, dropdown, toggle button, etc - what Jetpack supports in their new API), and also a "custom" type that would allow the caller to define its own like you've suggested (ie, you'd need to pass in {type:"custom"} to createWidget).

Comment 4

[Mike de Boer [:mikedeboer]]

Attachment: WIP: make the zoom controls customizable

Blair: I'm asking you for feedback here to make sure I'm following the correct path to implement this. The implementation here will be similar for the edit controls in bug 870901. Also, I'm having a bit of trouble getting the styling right...

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 755483 [details] [diff] [review]
WIP: make the zoom controls customizable

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

Yep, looking good.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +303,4 @@
>        this.insertWidgetBefore(node, currentNode, container, aArea);
> +      let widget = gPalette.get(id);
> +      if (widget && widget.onInsert)
> +        widget.onInsert(node, currentNode, container, aArea);

Hmm, think we'd be better off to re-use the existing events, and automatically send them to API widgets in notifyListeners(). I don't think you need those extra params, do you?

@@ +370,5 @@
>      // handles that fine, but this is a public API.
>      return CustomizableUI.PROVIDER_XUL;
>    },
>  
> +  getWidgetNode: function(aWidgetId, aWindow, aArea) {

You don't need to pass in aArea here (and I really want to avoid it anyway, to keep that API as simple as possible) - buildWidget() can use widget.currentArea

@@ +693,5 @@
>  
> +    let node;
> +    if (aWidget.type == "custom") {
> +      if (aWidget.onBuild) {
> +        node = aWidget.onBuild(aDocument, aArea);

This needs wrapped in a try/catch - need to avoid any one buggy widget breaking all of CustomizableUI.

@@ +696,5 @@
> +      if (aWidget.onBuild) {
> +        node = aWidget.onBuild(aDocument, aArea);
> +      }
> +      if (!node)
> +        ERROR("Custom widget with id " + aWidget.id + " does not return a node");

Should also check this is actually a DOM node, and not something we can't use.

Comment 6

[Mike de Boer [:mikedeboer]]

Attachment: WIP: make the zoom controls customizable

Blair: I changed the API according to your suggestions (and MUCH more to my liking ;) ). Does this look OK?

Mike: I'm having problems with the look and feel of this widget inside the popup/ panel - see screenshot. Also, will this widget be removable? If yes, how would it look like inside the palette? Currently I have the Reset Zoom button inside the toolbar as well, just to try how it feels - I kinda like it.

Comment 7

[Mike de Boer [:mikedeboer]]

Attachment: Styling glitches... help!?!

the screenshot I mentioned earlier...

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: WIP: make the zoom controls customizable

unbitrotted patch. Side-effect: zoom controls can't be dragged into nav-bar anymore.

Comment 9

[Mike de Boer [:mikedeboer]]

Attachment: WIP: make the zoom controls customizable

with this patch, DnD works again. So, the widget will be removable, so we need to know the style inside the palette.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 755946 [details]
Styling glitches... help!?!

Clearing feedback request, since we seem to have made headway on this in IRC.

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 756033 [details] [diff] [review]
WIP: make the zoom controls customizable

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +140,5 @@
>  } catch (e) {}
>  
>  function LOG(aMsg) {
>    if (gDebug) {
> +    dump("[CustomizableUI] " + aMsg + "\n");//Services.console.logStringMessage("[CustomizableUI] " + aMsg);

Revert now?

@@ +145,3 @@
>    }
>  }
> +function ERROR(aMsg) dump("[CustomizableUI] " + aMsg + "\n");//Cu.reportError("[CustomizableUI] " + aMsg);

Ditto.

@@ +440,5 @@
>      // widget to the requested location.
>      for (let areaNode of areaNodes) {
>        let window = areaNode.ownerDocument.defaultView;
>        let container = areaNode.customizationTarget;
> +      let [provider, widgetNode] = this.getWidgetNode(aWidgetId, window, aArea);

Can revert this now.

::: browser/components/customizableui/src/CustomizableWidgets.jsm
@@ +335,5 @@
> +        onWidgetRemoved: function(aWidgetId, aPrevArea) {
> +          if (aWidgetId != this.id)
> +            return;
> +          
> +          // TODO: when a widget is demoted to the palette ('removed'), it's visual

Oh noes - trailing whitespace!!!!1111oneone~

::: browser/themes/osx/browser.css
@@ +1025,5 @@
>    border-top-left-radius: 0;
>    border-bottom-left-radius: 0;
>  }
>  
> +#zoom-reset-button {

Why are these here, and everything else in panelUIOverlay.css?

Comment 12

[Mike de Boer [:mikedeboer]]

Attachment: Make the zoom controls customizable

Fixed the styling issues on mac by using flex=1 properly.

Comment 13

[Mike de Boer [:mikedeboer]]

Attachment: Make the zoom controls customizable

Comment 14

[Mike de Boer [:mikedeboer]]

Attachment: Make the zoom controls customizable

Comment 15

[Mike de Boer [:mikedeboer]]

Attachment: Make the zoom controls customizable

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 756554 [details] [diff] [review]
Make the zoom controls customizable

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

Mike - this looks really really good! Very close here - just a few suggestions.

::: browser/base/content/browser.xul
@@ -1009,5 @@
>  
> -      <toolbaritem id="zoom-controls" class="chromeclass-toolbar-additional"
> -                   title="&zoomControls.label;">
> -        <toolbarbutton id="zoom-out-button" class="toolbarbutton-1"
> -                       label="&fullZoomReduceCmd.label;"

Because we're switching to using a .properties string bundle in bug 868433, we don't need to keep these .dtd strings.

So you can remove fullZoomReduceCmd.label, zoomOutButton.tooltip, fullZoomEnlargeCmd.label and zoomInButton.tooltip.

::: browser/components/customizableui/src/CustomizableWidgets.jsm
@@ +347,5 @@
> +        onWidgetRemoved: function(aWidgetId, aPrevArea) {
> +          if (aWidgetId != this.id)
> +            return;
> +
> +          // When a widget is demoted to the palette ('removed'), it's visual

it's -> its

@@ +349,5 @@
> +            return;
> +
> +          // When a widget is demoted to the palette ('removed'), it's visual
> +          // style should change.
> +          listener.onWidgetAdded(aWidgetId, false);

Not the hugest fan of this call to onWidgetAdded from within onWidgetRemoved.

Perhaps we should move the contents of onWidgetAdded into a separate function within listener that both onWidgetAdded and onWidgetRemoved can call.

Comment 17

[Mike de Boer [:mikedeboer]]

Attachment: Make the zoom controls customizable

* made function reuse more explicit
* removed strings that aren't in use someplace else

Comment 18

[u428464]

Does this cover the fact to have large widgets buttons borderless or should another bug be filed ?

Comment 19

[Mike de Boer [:mikedeboer]]

(In reply to Guillaume C. [:ge3k0s] from comment #18)
> Does this cover the fact to have large widgets buttons borderless or should
> another bug be filed ?

What do you mean exactly, Guillaume? Could you point me to a screenshot perhaps?

Comment 20

[u428464]

(In reply to Mike de Boer [:mikedeboer] from comment #19)
> (In reply to Guillaume C. [:ge3k0s] from comment #18)
> > Does this cover the fact to have large widgets buttons borderless or should
> > another bug be filed ?
> 
> What do you mean exactly, Guillaume? Could you point me to a screenshot
> perhaps?

Yeap, on latest mockups the buttons are borderless ( http://people.mozilla.com/~shorlander/files/australis-design-specs/images-osx/[e-mail] ) see also : http://people.mozilla.com/~shorlander/customizationMode-liveDemo-i02/windows7-customizationMode-i02.html (click menu button to see the styling).

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 756570 [details] [diff] [review]
Make the zoom controls customizable

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

r=me with the following nit-fix. Thanks Mike!

\o/

::: browser/components/customizableui/src/CustomizableWidgets.jsm
@@ +257,5 @@
> +      let inPanel = (this.currentArea == CustomizableUI.AREA_PANEL);
> +      let noautoclose = inPanel ? "true" : null;
> +      let flex = inPanel ? "1" : null;
> +      let cls = inPanel ? "panel-combined-button" : "toolbarbutton-1";
> +      let buttons = [

One last thing, just a style nit - let's structure that like this:

let buttons = [{
  // ...
}, {
  // ...
}];

Comment 22

[Mike de Boer [:mikedeboer]]

Ok, now I understand :) To answer your question: no, this is not covered by this patch. Another bug should be filed.

Comment 23

[Mike Conley (:mconley) (:⚙️)]

(In reply to Guillaume C. [:ge3k0s] from comment #20)
> (In reply to Mike de Boer [:mikedeboer] from comment #19)
> > (In reply to Guillaume C. [:ge3k0s] from comment #18)
> > > Does this cover the fact to have large widgets buttons borderless or should
> > > another bug be filed ?
> > 
> > What do you mean exactly, Guillaume? Could you point me to a screenshot
> > perhaps?
> 
> Yeap, on latest mockups the buttons are borderless (
> http://people.mozilla.com/~shorlander/files/australis-design-specs/images-
> osx/[e-mail] ) see also :
> http://people.mozilla.com/~shorlander/customizationMode-liveDemo-i02/
> windows7-customizationMode-i02.html (click menu button to see the styling).

Let's file a separate bug to have the appearance reviewed and (if necessary) tweaked.

Mike - can you file that, please?

Comment 24

[Mike de Boer [:mikedeboer]]

Attachment: Make the zoom controls customizable

Fixed nit. Carrying over r=mconley.

Mike: did you notice that the whole CustomizableWidgets array is indented incorrectly? (talking about nits ;) )

Comment 25

[u428464]

Filed bug 878065.

Comment 26

[Mike Conley (:mconley) (:⚙️)]

(In reply to Mike de Boer [:mikedeboer] from comment #24)
> Created attachment 756574 [details] [diff] [review]
> Make the zoom controls customizable
> 
> Fixed nit. Carrying over r=mconley.
> 
> Mike: did you notice that the whole CustomizableWidgets array is indented
> incorrectly? (talking about nits ;) )

D'oh!

Comment 27

[Mike de Boer [:mikedeboer]]

Attachment: Make the zoom controls customizable

Unbitrotted patch. Carrying over r=mconley.

Comment 28

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 756593 [details] [diff] [review]
Make the zoom controls customizable

I missed something when testing - the reset button in the toolbar isn't hidden (and shows all of Toolbar.png).

I also noticed that if I put the control into the nav-bar, and then "Restore Defaults", the reset button isn't shown.

Comment 29

[Mike de Boer [:mikedeboer]]

Attachment: Make the zoom controls customizable

Solved issues mentioned by mconley. Especially the Restore Defaults case is a bit tricky, since it doesn't follow the usual code-path.

Comment 30

[Mike de Boer [:mikedeboer]]

Attachment: Make the zoom controls customizable

Unbitsnotting

Comment 31

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 756831 [details] [diff] [review]
Make the zoom controls customizable

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

r=me - just a suggestion about setAttributes which helps de-dupe it for the edit controls.

Great job on this!

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +1134,5 @@
>          if (aEvent in listener) {
>            listener[aEvent].apply(listener, aArgs);
>          }
>        } catch (e) {
> +        Cu.reportError(e + " -- " + (e.fileName ? e.fileName + ":" + e.lineNumber : ""));

Might as well just use console.error here now that 878300 is almost ready to land. I guess it depends on when that last review comes in.

::: browser/components/customizableui/src/CustomizableWidgets.jsm
@@ +247,5 @@
>        }
>      }
> +  }, {
> +    id: "zoom-controls",
> +    name: "Zoom Controls",

Another bitrot warning for bug 868433

@@ +282,5 @@
> +        label: "Zoom in",
> +        tooltiptext: "Zoom in"
> +      }];
> +
> +      function setAttributes(aNode, aAttrs) {

Seems silly to duplicate this for both Zoom and Edit controls. Maybe suck it out into the global scope of CustomizableWidgets.jsm for all to use.

::: browser/themes/shared/browser.inc
@@ +1,3 @@
>  %filter substitution
>  
> +%define primaryToolbarButtons #back-button, #forward-button, #reload-button, #stop-button, #home-button, #print-button, #downloads-button, #downloads-indicator, #bookmarks-menu-button, #new-tab-button, #new-window-button, #fullscreen-button, #zoom-out-button, #zoom-reset-button, #zoom-in-button, #sync-button, #feed-button, #alltabs-button, #tabview-button, #webrtc-status-button, #social-share-button

Bitrot warning due to bug 877335. It's a race. :)

Comment 32

[Mike Conley (:mconley) (:⚙️)]

One thing - there's a comment in browser/themes/linux/browser.css referring to this bug. I wrote that comment when I thought that the zoom control icons were being replaced with a single image. Not sure if we're still doing that or not.

But I think maybe we should remove that comment.

Comment 33

[Mike de Boer [:mikedeboer]]

Attachment: Make the zoom controls customizable

Carrying over r=mconley

Comment 34

[:Gijs (he/him)]

Pushed: http://hg.mozilla.org/projects/ux/rev/c986b5090517

Comment 35

[Mike Conley (:mconley) (:⚙️)]

Backed out due to possibly introducing leaks.

https://hg.mozilla.org/projects/ux/rev/de6bb0c6450a

Comment 36

[Mike Conley (:mconley) (:⚙️)]

So this patch is introducing leaks (and so is the Edit one for similar reasons).

The reason, is because when onWidgetDestroyed is called, the comparison between the widget ID and this.id never evaluates to be true, meaning that the CustomizableUI listener is never removed.

So there's the leak.

As mentioned in IRC, a faster way to test this instead of running the entire mochitest-bc suite, is to use this with a debug build:

./mach mochitest-browser browser/base/content/test/browser_URLBarSetURI.js

This triggers the leak because this test enters and exits customization mode.

Comment 37

[Mike de Boer [:mikedeboer]]

Attachment: Make the zoom controls customizable

Added event hook to allow (custom) widgets to clean up after themselves when instances are removed.

Comment 38

[Mike de Boer [:mikedeboer]]

Attachment: Make the zoom controls customizable

Unbitrotted patch. Carrying over r=mconley, sr=Unfocused

Comment 39

[:Gijs (he/him)]

Pushed again: https://hg.mozilla.org/projects/ux/rev/d92bea85aa14

Comment 40

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/d92bea85aa14