749804 - Remove add-on bar and move content in navigation bar

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Zhenshuo Fang (:fang) - Firefox UX Team]

Since we will have more space in the navigation bar when we move the search bar, it would be nice to remove the add-on bar and put the content in the navigation bar.

Comment 1

[Raymond Lee [:raymondlee]]

Is there any plan when this will be implemented?

Comment 2

[Andrew Buehler]

According to my best interpretation, this bug report is essentially requesting to move the add-on bar from the bottom of the browser window to the same place as the other toolbars, at the top of the browser window.

In some cases, I specifically want to place UI elements (primarily, though not inherently limited to, add-on interface icons) at the bottom of the browser window.

Would this still be possible - via a reasonable user-facing interface, not via direct editing of e.g. XUL files - without the bottom-of-the-window "toolbar space" of the add-on bar?

If not, I would like to request that this change not be made. Contrary to the statement in the original bug report, such an interface regression would seem to me to be exactly the opposite of nice.

Comment 3

[Dave Garrett]

I ~think~ the original intent of this bug was to plan for moving the default position of add-on buttons up to the top so the bottom bar won't be needed. I also think this idea was never really pursued and don't know if this bug is still valid.

I generally agree that even if it's empty the bottom bar should be an available option.

Comment 4

[Andrew Buehler]

The original bug report suggested two things: "remove the add-on bar" and "put the content in the navigation bar". Moving the default position of add-on buttons up to the top would seem to fall under the latter, but I don't see any interpretation of the former except removing the bottom bar entirely.

I would probably *prefer* to have (at least some) add-ons continue to default to placing icons et cetera in the bottom bar, but as long as it's configurable, I don't absolutely object to changing the default. Removing the option of placing things at the bottom on purpose is quite another matter.

Comment 5

[Willy_ Foo_Foo]

(In reply to Andrew Buehler from comment #4)
 
> I would probably *prefer* to have (at least some) add-ons continue to
> default to placing icons et cetera in the bottom bar, but as long as it's
> configurable, I don't absolutely object to changing the default. Removing
> the option of placing things at the bottom on purpose is quite another
> matter.

I absolutely object to changing the default !!
people who use loads of addons (like me) , it's easy to keep
my addons shortcuts, buttons etc for easy accessibility.
add-ons continue to
default to placing icons et cetera in the bottom bar is the way
That why people love Mozilla ,because of it's customization
Most People 
FF & hate chrome(which fills up the nav/address bar,which people like clean)
So please Do-not Remove the Features
which are useful to users(stating Check-boxes that can kill, since most users use defaults !!) & do a Chrome/ie
instead make it most customizable browser please
and
let the user decide how he wants his/her browser to look

Comment 6

[Willy_ Foo_Foo]

Not sure that torching every check box in the options menu will solve that problem. User error will FIND a way to mess something up. It doesn't matter how hard you try to remove easy access options.
tab-bar and add-ons bar are completely un-customizable- That's a big NO
The current Firefox button isn't movable by default either.
The back-forward buttons, the url-bar and the reload-stop button are joined as one unmovable element - completely non-customizable 
Doesn't "Reset Firefox" cover the dangers these tantalizing check boxes present anyway?
Another consequence of removing too many options is that one day, an average user will need one of those (for whatever reason), Google it, and get to the about:config page. Chances he'll break something then are way higher than they are now, also because resetting options is harder. "How was that called again? Something with browser. No, javascript. Or was it something else anyway?". A misclick in about:config is easier to make and harder to revert than in the options panel

Comment 7

[Alice0775 White]

Do not move addon-bar from bottom!!
1. The navbar does not have enough width to display location bar and such buttons.
   I have total approx. 700-800px width of toolbuttons in addon-bar, and I can easily access such buttons in any time.
2. in maximized window mode, Bug 644621 appears If there is not addon-bar in the bottom.

Comment 8

[:Gijs (he/him)]

Attachment: Remove the add-on bar

This was the original removal patch reviewed by Jared in bug 869939.

Comment 9

[:Gijs (he/him)]

Attachment: WIP to have an automigrating shim

This gives an idea of what I'm currently working on. It's not finished yet, among other things we will (also) need to deal with the currentset attribute in an ordinary UI migration in nsBrowserGlue.js and remove its persisted value, and there will need to be more testing, polish, etc.

The reason I'm posting something now is for one thing to get feedback on this approach, and to give a report of what I've found so far in testing this with some add-ons.

First, a wide SDK add-on like memchaser. The SDK doesn't like having its iframes moved around from under iself, a docshell goes null and the widget stops working completely. We can probably fix this by just fixing the SDK, to an extent. However, it ends up in the menupanel automatically right now, and a 360px widget like memchaser is too wide even for the menupanel. We will need to add code to constrain the size of these widgets if we put them there. Furthermore, them not being nice and square and managably-sized like the rest of the buttons messes with the layout of the palette in customization mode, too.

Second, adblockplus. ABP is a bit interesting because it persists a custom attribute on the toolbox to indicate to itself where it should insert its item. Unfortunately the attribute updating only happens when aftercustomization fires. So I added code to fire this event, because I figured that sort of makes sense if we're moving widgets around (i.e. from the add-on bar to the navbar). However, it still doesn't work because the code here (https://mxr.mozilla.org/addons/source/1865/lib/ui.js#1051, line 1057) hardcodes that it wants a toolbar as its parent. In the nav-bar, we use an hbox for this purpose, so the code sets the item to hidden,,, which means the widget disappears completely after a restart (on initial install, it is positioned correctly and seems to work).

Third, a random add-on I found that overlays directly onto the add-on bar using a XUL overlay. Moving it works (although I realized we need to force-set removable=true in order for the customization code to be OK with it). Unfortunately, the add-on I tested broke completely because it depends too much on being in the add-on bar (https://mxr.mozilla.org/addons/source/590/chrome/content/ipv6ident.js#237 ) and having an addonbar-closebutton there (which I removed...).

Fourth, noscript fails utterly because it hardcodes references to the status bar, which I've also removed completely, after which it can't find its button and just fails outright. However, if that wouldn't fail, I fear that the event I added to try to make adblockplus work would result in an infinite loop of this function:
https://mxr.mozilla.org/addons/source/722/chrome/content/noscript/noscriptOverlay.js#355

because it would add an item, that'd get automoved and trigger an aftercustomization event, etc. etc. etc. (I haven't examined this out in detail because that didn't seem worth it, so I may be wrong)

So, all in all... I'm not convinced adding this shim is better than not adding it. Of course this sample is not scientific, but I get the impression most of the popular add-ons have quite intricate code dealing with its insertion which will need dedicated outreach to just be fixed, shim or no, the SDK code will need to be fixed, shim or no, and everything else... well, it'd depend on how much care the developers took.

Jared, can you have a look and tell me what you think?

Comment 10

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

Comment on attachment 761558 [details] [diff] [review]
WIP to have an automigrating shim

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

Hm, I'm not sure. The write up that you provided makes this seem like a pretty scary change. Let's get input from some other people too to see what they think.

::: browser/components/customizableui/content/toolbar.xml
@@ +340,5 @@
> +          let nodeWidth = aNode.getBoundingClientRect().width;
> +          let targetArea = this._delegatingToolbar;
> +          let isTooBig = (nodeWidth == 0 || nodeWidth * 1 > this._itemMaxWidth);
> +          if (isTooBig) {
> +            targetArea = CustomizableUI.AREA_PANEL;

If it's too big for the nav-bar, let's just move it to the palette.

@@ +346,5 @@
> +          Cu.reportError(aNode.id + " is " + (!isTooBig ? "not " : "") +
> +                         "too big (" + nodeWidth + "px wide), moving to " + targetArea);
> +
> +          if (aNode.getAttribute("removable") != "true") {
> +            aNode.setAttribute("removable", "true");

Isn't this just setting removable="true" on all nodes? Might as well call setAttribute unguarded.

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 761558 [details] [diff] [review]
WIP to have an automigrating shim

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

Code-wise, it looks alright.

The real question is impact - does this patch make things better or worse for people with these add-ons installed?

Looking at the addons mxr, there are less then 100 add-ons that use overlays for the addon-bar. There were too many results to show, when searching for any add-on that references the addon-bar. A lot of those are Jetpack add-ons, but not all. Those that are Jetpack can be (presumably) be solved in the SDK itself without a shim, and a shim isn't enough anyway. For the others, that are manually inserting via JS, I'd expect a higher chance of them depending on the addon-bar and therefore breaking.

So, seems we may be better off without the shim (or at least, the automigration part - may or may not be worth having a dummy node that never shows).

Comment 12

[:Gijs (he/him)]

While looking at add-ons (summary up as soon as I've finished...), I noticed that we'll need to update:

https://developer.mozilla.org/en-US/docs/Code_snippets/Toolbar
https://developer.mozilla.org/en-US/docs/The_add-on_bar

and possibly other documents. Delaying setting dev-doc-needed until we have a decision and have landed 'something', though.

Comment 13

[:Gijs (he/him)]

Attachment: Detailed migration results

As part of understanding the impact of this change and the migration, I took the top 100 add-ons on AMO and tried to see how they'd be affected by the removal and/or the migration.

I took their add-on IDs and looked in the add-ons MXR for items which reference the 'addon-bar' in their code (I didn't include add-ons which only referenced it in CSS).

Note that I explicitly didn't look for references to the status bar shim, because consensus seems to be that that is getting removed, with or without Australis, in either 24 or 25. Note that I did keep an empty status-bar shim around to reduce confusion about what was causing issues; it is never user-visible, though, and its contents never get migrated anywhere (I tried, it leads to all kinds of hairiness because its contents aren't really toolbaritems, and generally didn't seem worth it).

I did a small distribution table of how many add-ons reference the add-on bar, per increasing top N of popular add-ons.
06/10  (60%)
09/20  (45%)
11/30  (37%)
14/40  (35%)
14/50  (28%)
21/75  (28%)
26/100 (26%)

So this distribution tapers off towards 1/4 of add-ons (which are on AMO) that seem to reference it in some way. It may taper off further or not, that's hard to tell from just this data. It seems unlikely to go up, however… Of the 26 add-ons I collected with these queries, only 1 (Collusion!) uses the add-on SDK. For the other 25, I tested how they reacted to the migration. I also did quick code checks to see if my initial impression of whether or not there was a problem was correct.

Speaking purely about the add-on bar removal and migration, out of the 25 add-ons:

10 add-ons didn't have any add-on bar related issues at all (of which 2 were successfully migrated)
1 shows two buttons initially, and one button after a restart (this add-on was also successfully migrated)
8 get icons in the palette, with migration not seeming to do anything. In a few cases this is because they only modify the add-on bar's currentset attribute. We *may* be able to improve this.
4 have no icons because they overlay the status bar, but otherwise work.
2 are actually genuinely broken: 1 only when its 'appearance' preferences are changed to a non-default setting, 1 (forecast fox) because it uses the status bar and that is (as far as I can tell) the only way to have it do anything functional.

Generally, I would say this means the impact of the changes is not quite as severe as comment 9 suggested.

Personally, I would lean towards keeping either a 'non-functional', always hidden shim, or doing the migration. Between those two options, because some of the add-ons' behavior did improve with the migration, and we can probably improve that a little if we tweak our insertItem/currentSet usage, I would lean on the side of migrating over having a non-functional, hidden shim just to avoid JS errors.

I noticed non-addon-bar-related issues with the new customization or tabs code in at least 10 of the add-ons I tested. Given that similar issues are likely to affect some of the other 74 add-ons, that means the impact of the customization changes as-is is likely larger than those of the add-on bar removal itself.

Comment 14

[:Gijs (he/him)]

Attachment: Add-on bar usage mxr queries for the add-ons tested

Comment 15

[:Gijs (he/him)]

Final full disclosure notes:

I couldn't easily test the fireshot add-on (it is incompatible with trunk) and used code inspection instead.

I am a contributor for 2 of the top 100 add-ons (ChatZilla and Venkman), neither of which use the add-on bar.

Comment 16

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

Removing the items from M7 that do not block us from landing on m-c.

Comment 17

[:Gijs (he/him)]

Attachment: Patch for review

So this is what I have right now. It improves on the patch with which I was testing a little and also migrates currentSet users, which gets at least 2 more add-ons to work correctly that were missing icons before, in this limited sample.

On the 25 add-ons, I think getting 20% of them to migrate correctly with limited work on their part is a net win, and we should pick this over doing nothing or having an empty shim. I also think that given the scope of the problems and the ones customization causes anyway, this is a doable thing to ship with Australis. Obviously that decision could be revisited if the fallout turns out to be too heavy. Reverting this change and making the add-on bar customizable should be a relatively simple change to make if this turns out to not be the right path within the next few cycles.

Mike, Blair, what do you think?

Comment 18

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

This work blocks fixing the failing Jetpack tests (bug 885015), which in turn blocks us from landing on m-c.

It blocks the tests because, according to my understanding in talking with Gijs, the work to make the test pass would be undone by this bug anyway, and we don't want to create extra work for ourselves.

So we think this blocks M8 (landing to m-c).

Comment 19

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

Comment on attachment 765393 [details] [diff] [review]
Patch for review

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

Some minor nits, but I'm pretty happy if this takes care of most cases. Great job on this, Gijs.

::: browser/base/content/browser.css
@@ +28,5 @@
>  }
>  
> +#addon-bar {
> +  -moz-binding: url("chrome://browser/content/customizableui/toolbar.xml#addonbar-delegating");
> +  visibility:visible;

nit - space after :

Couldn't we just collapse the add-on bar? visibility: collapse? (You'd still need to set margins to 0 though). Or are we unable to read the bounding box dimensions for the add-on bar items if we go that route?

@@ +37,5 @@
> +  border: 0 none;
> +}
> +
> +#addonbar-closebutton {
> +  visibility: visible;

collapse?

@@ +42,5 @@
> +  height: 0 !important;
> +}
> +
> +#status-bar {
> +  height: 0 !important;

collapse?

::: browser/base/content/browser.xul
@@ +860,5 @@
>          </hbox>
>        </toolbaritem>
>      </toolbar>
>  
> +    <toolbar id="addon-bar" toolbar-delegate="nav-bar" item-max-width="100">

Maybe put in a comment here saying that this is a shim that'll soon go away.

::: browser/components/customizableui/content/toolbar.xml
@@ +275,5 @@
> +    <implementation>
> +      <constructor><![CDATA[
> +          // Reading these immediately so nobody messes with them anymore:
> +          this._delegatingToolbar = this.getAttribute("toolbar-delegate");
> +          this._itemMaxWidth = 1 * this.getAttribute("item-max-width");

Nice coersion. :) I guess parseInt(this.getAttribute("item-max-width"), 10) is less desirable somehow?

@@ +277,5 @@
> +          // Reading these immediately so nobody messes with them anymore:
> +          this._delegatingToolbar = this.getAttribute("toolbar-delegate");
> +          this._itemMaxWidth = 1 * this.getAttribute("item-max-width");
> +          // Leaving those in here to unbreak some code:
> +          this._whiteListed = ["addonbar-closebutton", "status-bar"];

If these are static, we might as well make them a read-only private property. Bonus points if we make them a Set, so we can just use has instead of indexOf.

@@ +309,5 @@
> +        ]]></body>
> +      </method>
> +      <method name="evictNodes">
> +        <body><![CDATA[
> +          this._areModifying = true;

Let's define this field in the binding, and have it default to false. Expando always makes me uncomfortable. :)

@@ +326,5 @@
> +      <method name="evictNode">
> +        <parameter name="aNode"/>
> +        <body>
> +        <![CDATA[
> +          let scope = {}

Nit, missing semicolon

@@ +327,5 @@
> +        <parameter name="aNode"/>
> +        <body>
> +        <![CDATA[
> +          let scope = {}
> +          if (this._whiteListed.indexOf(aNode.id) > -1) {

Let's put this before the scope def'n.

@@ +386,5 @@
> +          this.appendChild(node);
> +          this.evictNode(node);
> +          // We will now have moved stuff around; kick off an aftercustomization event
> +          // so add-ons know we've just moved their stuff:
> +          let win = this.ownerDocument.defaulView;

Isn't this just window?

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +958,5 @@
>      // throw away aPosition though, as that can only be bogus if the area hasn't
>      // yet been restorted (caller can't possibly know where its putting the
>      // widget in relation to other widgets).
>      if (this.isAreaLazy(aArea)) {
> +      gFuturePlacements.get(aArea).add(aWidgetId);

How embarrassing... Good catch. :)

Comment 20

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 765393 [details] [diff] [review]
Patch for review

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

What Mike said. Except...

(In reply to Mike Conley (:mconley) from comment #19)
> Couldn't we just collapse the add-on bar? visibility: collapse? (You'd still
> need to set margins to 0 though). Or are we unable to read the bounding box
> dimensions for the add-on bar items if we go that route?

No, it's pretty common for add-ons to remove visibility:collapse to force the add-on bar to show when they're installed.

::: browser/base/content/browser.xul
@@ +862,5 @@
>      </toolbar>
>  
> +    <toolbar id="addon-bar" toolbar-delegate="nav-bar" item-max-width="100">
> +      <hbox id="addonbar-closebutton" removable="false"/>
> +      <statusbar id="status-bar"/>

removable=false on addonbar-closebutton but not status-bar?

::: browser/components/customizableui/content/toolbar.xml
@@ +303,5 @@
> +            if (!this._areModifying) {
> +              this.evictNodes();
> +            }
> +          }.bind(this);
> +          let mutationObserver = new MutationObserver(mutationFn);

Using an arrow function makes this much neater, IMO:

  let mutationObserver = new MutationObserver(() => {
    if (!this._areModifying) {
      this.evictNodes();
    }
  });

@@ +309,5 @@
> +        ]]></body>
> +      </method>
> +      <method name="evictNodes">
> +        <body><![CDATA[
> +          this._areModifying = true;

"_areModifying" is an odd name. _isModifying?

@@ +330,5 @@
> +          let scope = {}
> +          if (this._whiteListed.indexOf(aNode.id) > -1) {
> +            return;
> +          }
> +          Cu.import("resource:///modules/CustomizableUI.jsm", scope);

Shouldn't be importing this every time evictNode() is called - just have the constructor import it into a property on the binding.

@@ +338,5 @@
> +            aNode.setAttribute("removable", "true");
> +
> +            let nodeWidth = aNode.getBoundingClientRect().width;
> +            if (nodeWidth == 0 || nodeWidth * 1 > this._itemMaxWidth) {
> +              throw aNode.id + " is too big (" + nodeWidth + "px wide), moving to the palette";

throw new Error(...)

@@ +355,5 @@
> +
> +          // Surprise: addWidgetToArea(panel) will get you nothing if the panel is not constructed
> +          // yet. Fix:
> +          if (aNode.parentNode == oldParent) {
> +            let palette = this.toolbox.palette || this.toolbox.querySelector('toolbarpalette');

Kill the |this.toolbox.querySelector('toolbarpalette');| - we don't look up the palette that way anywhere else.

@@ +375,5 @@
> +          }
> +
> +          let widget = CustomizableUI.getWidget(aId);
> +          widget = widget && widget.forWindow(this.ownerDocument.defaultView);
> +          let node = widget && widget.node;

Can this not be replaced by |this.ownerDocument.getElementById()| ? I don't think we need to (or should) support non-XUL widgets here anyway.

@@ +401,5 @@
> +        ]]></getter>
> +      </property>
> +      <property name="currentSet">
> +        <getter><![CDATA[
> +          if (!this._customizationTarget)

Eh?

@@ +404,5 @@
> +        <getter><![CDATA[
> +          if (!this._customizationTarget)
> +            return "";
> +
> +          return [node.id for (node of this.children)].join(',');

Shouldn't this take into account the whitelist?

@@ +410,5 @@
> +        <setter><![CDATA[
> +          let v = val.split(',');
> +          let newButtons = v.filter(x => x && this._whiteListed.indexOf(x) == -1);
> +          for (x of newButtons) {
> +            this.insertItem(x);

This seems broken, as it assumes every item is new. That should be generally true during runtime, but the following scenario is possible:

  let currentSet = addonbar.currentSet;
  do_async_thing(); // all existing widgets get migrated during this
  addonbar.currentSet = currentSet + ",my-widget";

And IIRC, mutation observers are async, so this could happen in other ways too.

Also, is anything likely to break if this doesn't implement removing items?

Comment 21

[:Gijs (he/him)]

Attachment: Auto-migration patch v1.1

(In reply to Mike Conley (:mconley) from comment #19)
> Couldn't we just collapse the add-on bar? visibility: collapse? (You'd still
> need to set margins to 0 though). Or are we unable to read the bounding box
> dimensions for the add-on bar items if we go that route?

That, plus what Blair said.

> > +    <toolbar id="addon-bar" toolbar-delegate="nav-bar" item-max-width="100">
> Maybe put in a comment here saying that this is a shim that'll soon go away.
Done.

> ::: browser/components/customizableui/content/toolbar.xml
> @@ +275,5 @@
> > +    <implementation>
> > +      <constructor><![CDATA[
> > +          // Reading these immediately so nobody messes with them anymore:
> > +          this._delegatingToolbar = this.getAttribute("toolbar-delegate");
> > +          this._itemMaxWidth = 1 * this.getAttribute("item-max-width");
> 
> Nice coersion. :) I guess parseInt(this.getAttribute("item-max-width"), 10)
> is less desirable somehow?

It's slower because it'll tolerate more stuff than a simple coercion (eg. "10afadf"). However, we're not using this flexibility now and probably won't later, so I've just made it a constant in evictNode. The only reason I'm not doing that with the delegate ID is that it'd be hardcoding document IDs inside a binding, which feels really dirty.
 
> @@ +277,5 @@
> > +          // Reading these immediately so nobody messes with them anymore:
> > +          this._delegatingToolbar = this.getAttribute("toolbar-delegate");
> > +          this._itemMaxWidth = 1 * this.getAttribute("item-max-width");
> > +          // Leaving those in here to unbreak some code:
> > +          this._whiteListed = ["addonbar-closebutton", "status-bar"];
> 
> If these are static, we might as well make them a read-only private
> property. Bonus points if we make them a Set, so we can just use has instead
> of indexOf.
Done, although that means I'm creating a new set every time? Maybe I misunderstood what you meant.


> > +          this._areModifying = true;
> Let's define this field in the binding, and have it default to false.
> Expando always makes me uncomfortable. :)
Done and changed to _isModifying per Blair's comment.

> @@ +386,5 @@
> > +          let win = this.ownerDocument.defaulView;
> Isn't this just window?
A fine point. Fixed.
 

(In reply to Blair McBride [:Unfocused] from comment #20)
> > +      <hbox id="addonbar-closebutton" removable="false"/>
> > +      <statusbar id="status-bar"/>
> 
> removable=false on addonbar-closebutton but not status-bar?
Fixed.

> ::: browser/components/customizableui/content/toolbar.xml
> @@ +303,5 @@
> > +            if (!this._areModifying) {
> > +              this.evictNodes();
> > +            }
> > +          }.bind(this);
> > +          let mutationObserver = new MutationObserver(mutationFn);
> 
> Using an arrow function makes this much neater, IMO:
> 
>   let mutationObserver = new MutationObserver(() => {
>     if (!this._areModifying) {
>       this.evictNodes();
>     }
>   });
Yeah, I was going for consistency with CustomizableUI.jsm where we seem to use bind a lot more, and only use => for single-line filter/map arguments. Fixed now!

> @@ +309,5 @@
> > +          this._areModifying = true;
> "_areModifying" is an odd name. _isModifying?
Fixed.
 
> @@ +330,5 @@
> > +          Cu.import("resource:///modules/CustomizableUI.jsm", scope);
> 
> Shouldn't be importing this every time evictNode() is called - just have the
> constructor import it into a property on the binding.
Or just use the global window one like we use in the other toolbar binding's insertItem... Which is what I've done now. Seems to work!

> @@ +338,5 @@
> > +              throw aNode.id + " is too big (" + nodeWidth + "px wide), moving to the palette";
> 
> throw new Error(...)
Fixed.

> @@ +355,5 @@
> > +
> > +          // Surprise: addWidgetToArea(panel) will get you nothing if the panel is not constructed
> > +          // yet. Fix:
> > +          if (aNode.parentNode == oldParent) {
> > +            let palette = this.toolbox.palette || this.toolbox.querySelector('toolbarpalette');
> 
> Kill the |this.toolbox.querySelector('toolbarpalette');| - we don't look up
> the palette that way anywhere else.

The toolbox's constructor gets called asynchronously, which means that sometimes this code runs before toolbox.palette is defined, hence my needing to get it manually. If you have a better way of doing this, I'd love to change it. :-)
 
> @@ +375,5 @@
> > +          }
> > +
> > +          let widget = CustomizableUI.getWidget(aId);
> > +          widget = widget && widget.forWindow(this.ownerDocument.defaultView);
> > +          let node = widget && widget.node;
> 
> Can this not be replaced by |this.ownerDocument.getElementById()| ? I don't
> think we need to (or should) support non-XUL widgets here anyway.

But we do need to support toolbox palette'd widgets, which aren't in the DOM anymore. Alternatively, I could use your suggestion + the querySelector (+ escaping magic) from customizableUI. Didn't want to duplicate code, or expose the 'find my XUL node' functionality separately, so this seemed cleaner.

> @@ +401,5 @@
> > +          if (!this._customizationTarget)
> 
> Eh?
Oops. Fixed.
 
> @@ +404,5 @@
> > +          return [node.id for (node of this.children)].join(',');
> 
> Shouldn't this take into account the whitelist?

I'm not sure what you mean here. Take it into account how/why?

> 
> @@ +410,5 @@
> > +        <setter><![CDATA[
> > +          let v = val.split(',');
> > +          let newButtons = v.filter(x => x && this._whiteListed.indexOf(x) == -1);
> > +          for (x of newButtons) {
> > +            this.insertItem(x);
> 
> This seems broken, as it assumes every item is new. That should be generally
> true during runtime, but the following scenario is possible:
> 
>   let currentSet = addonbar.currentSet;
>   do_async_thing(); // all existing widgets get migrated during this
>   addonbar.currentSet = currentSet + ",my-widget";
> And IIRC, mutation observers are async, so this could happen in other ways
> too.

I've added a list of migrated items to circumvent this.

However, the mutation observer point interests me for other reasons - wouldn't this mean the 'guard' boolean I added doesn't actually work? I've seen Mike use a similar approach in the panel, where we also have mutation observers... This seems really odd. Should I be disconnecting/reattaching the observer instead?

> 
> Also, is anything likely to break if this doesn't implement removing items?

Buttons may stick around on the navbar, should they have been moved there, longer than expected, that's about it, AIUI. But generally, currentset is fault-tolerant like our placements code, in that it can contain non-existing items. I haven't seen anyone using currentset to remove items, so I don't expect real problems. In any case, the shim is a stopgap but most people want to update their add-ons I expect...

Comment 22

[:Gijs (he/him)]

Note that this doesn't seem to cover the case where you upgrade with one of these add-ons installed, which is surprising to me. I need to look into this more. :-\

Comment 23

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #22)
> Note that this doesn't seem to cover the case where you upgrade with one of
> these add-ons installed, which is surprising to me. I need to look into this
> more. :-\

It looks like we're restoring the legacy placements before the add-on's overlay (to the palette) has been applied, which means we have a node stuck in the placement array which then ends up staying in the palette because it 'arrives' later than our addWidgetToArea call. I suspect this is also why some of the add-ons' nodes don't show up in the nav bar (see the detailed results I posted a few days ago). I'm not entirely sure what the best solution to this is - another mutation observer on the palette would do, but I suspect we want something else and it might have to do with our ts regressions, too; if we're restoring legacy state earlier than we used to, that would definitely influence paint times. Mike, does that sound at all plausible or am I off in the woods somewhere? OTOH, shouldn't we be running registerToolbar only after onreadystatechange, and shouldn't that take care of this? :-\

Confused! And now off into a plane.

Comment 24

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

Comment on attachment 765885 [details] [diff] [review]
Auto-migration patch v1.1

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

::: browser/components/customizableui/content/toolbar.xml
@@ +332,5 @@
> +            aNode.setAttribute("removable", "true");
> +
> +            let nodeWidth = aNode.getBoundingClientRect().width;
> +            if (nodeWidth == 0 || nodeWidth > kItemMaxWidth) {
> +              throw new Error(aNode.id + " is too big (" + nodeWidth +

This exception is going to get thrown, and then tossed away, since the catch has its own try / catch block...

If this information is useful (and I think it is), perhaps it should be pumped out to the error console instead of eaten silently.

@@ +419,5 @@
> +        ]]></getter>
> +      </property>
> +      <property name="_whiteListed" readonly="true">
> +        <getter><![CDATA[
> +          return new Set(["addonbar-closebutton", "status-bar"]);

You're right, I think - this'll create a new Set every time. Maybe I meant a read-only field instead of a property. :)

Comment 25

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

(In reply to :Gijs Kruitbosch from comment #23)
> (In reply to :Gijs Kruitbosch from comment #22)
> OTOH, shouldn't we be running
> registerToolbar only after onreadystatechange, and shouldn't that take care
> of this? :-\
> 

Yes, I thought so... which add-on are you using to test this?

Comment 26

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #24)
> Comment on attachment 765885 [details] [diff] [review]
> Auto-migration patch v1.1
> 
> Review of attachment 765885 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/customizableui/content/toolbar.xml
> @@ +332,5 @@
> > +            aNode.setAttribute("removable", "true");
> > +
> > +            let nodeWidth = aNode.getBoundingClientRect().width;
> > +            if (nodeWidth == 0 || nodeWidth > kItemMaxWidth) {
> > +              throw new Error(aNode.id + " is too big (" + nodeWidth +
> 
> This exception is going to get thrown, and then tossed away, since the catch
> has its own try / catch block...
> 
> If this information is useful (and I think it is), perhaps it should be
> pumped out to the error console instead of eaten silently.

I'll add back the Cu.reportError in that block.

> @@ +419,5 @@
> > +        ]]></getter>
> > +      </property>
> > +      <property name="_whiteListed" readonly="true">
> > +        <getter><![CDATA[
> > +          return new Set(["addonbar-closebutton", "status-bar"]);
> 
> You're right, I think - this'll create a new Set every time. Maybe I meant a
> read-only field instead of a property. :)

Can do, but then that means people would be able to modify it. Modifying something labeled 'whiteList' sounds scary, though I suppose in this case what'd happen is... items don't get migrated? Might not actually be bad.

(In reply to Mike Conley (:mconley) from comment #25)
> (In reply to :Gijs Kruitbosch from comment #23)
> > (In reply to :Gijs Kruitbosch from comment #22)
> > OTOH, shouldn't we be running
> > registerToolbar only after onreadystatechange, and shouldn't that take care
> > of this? :-\
> > 
> 
> Yes, I thought so... which add-on are you using to test this?

Stylish ( https://addon.mozilla.org/addon/stylish )

I've now dug a little deeper. It looks like when we hit the block here:

http://hg.mozilla.org/projects/ux/file/49e160b46dad/browser/components/customizableui/src/CustomizableUI.jsm#l722

for items in the add-on bar, the toolbox doesn't have a palette yet. However, when we hit it milliseconds later for the nav-bar, it does. Log:

[16:37:51.963] "[CustomizableUI]" "Saving state."
[16:37:51.963] "[CustomizableUI]" "State saved as: {"placements":{"PanelUI-contents":["edit-controls","zoom-controls","new-window-button","privatebrowsing-button","save-page-button","print-button","history-panelmenu","fullscreen-button","find-button","preferences-button","add-ons-button"],"addon-bar":["stylish-toolbar-button","addonbar-closebutton","customizableui-special-spring13718254719631","status-bar"]},"seen":[]}"
[16:37:51.963] "[CustomizableUI]" "Searching for stylish-toolbar-button in toolbox."
[16:37:51.964] "[CustomizableUI]" "Toolboxes for window: 1"
[16:37:51.964] "[CustomizableUI]" "No node for stylish-toolbar-button found."
[16:37:51.964] "[CustomizableUI]" "Unknown widget: stylish-toolbar-button"
[16:37:51.964] "[CustomizableUI]" "Searching for addonbar-closebutton in toolbox."
[16:37:51.965] "[CustomizableUI]" "Searching for status-bar in toolbox."
[16:37:51.965] "[CustomizableUI]" "Restoring PersonalToolbar from default state"
[16:37:51.965] "[CustomizableUI]" "Placements for PersonalToolbar:
	personal-bookmarks"
[16:37:51.966] "[CustomizableUI]" "Restoring nav-bar from default state"
[16:37:51.966] "[CustomizableUI]" "Placements for nav-bar:
	unified-back-forward-button
	urlbar-container
	reload-button
	stop-button
	search-container
	webrtc-status-button
	bookmarks-menu-button
	downloads-button
	home-button
	social-share-button"
[16:37:51.968] "[CustomizableUI]" "Searching for search-container in toolbox."
[16:37:51.969] "[CustomizableUI]" "Toolboxes for window: 1"
[16:37:51.969] "[CustomizableUI]" "Looking for search-container"
[16:37:51.969] "[CustomizableUI]" "Have: search-container,bookmarks-menu-button,home-button,print-button,downloads-button,new-window-button,fullscreen-button,sync-button,tabview-button,stylish-toolbar-button"

Comment 27

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

(In reply to :Gijs Kruitbosch from comment #26)
> (In reply to Mike Conley (:mconley) from comment #24)
> > Comment on attachment 765885 [details] [diff] [review]
> > Auto-migration patch v1.1
> > 
> > Review of attachment 765885 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: browser/components/customizableui/content/toolbar.xml
> > @@ +332,5 @@
> > > +            aNode.setAttribute("removable", "true");
> > > +
> > > +            let nodeWidth = aNode.getBoundingClientRect().width;
> > > +            if (nodeWidth == 0 || nodeWidth > kItemMaxWidth) {
> > > +              throw new Error(aNode.id + " is too big (" + nodeWidth +
> > 
> > This exception is going to get thrown, and then tossed away, since the catch
> > has its own try / catch block...
> > 
> > If this information is useful (and I think it is), perhaps it should be
> > pumped out to the error console instead of eaten silently.
> 
> I'll add back the Cu.reportError in that block.
> 
> > @@ +419,5 @@
> > > +        ]]></getter>
> > > +      </property>
> > > +      <property name="_whiteListed" readonly="true">
> > > +        <getter><![CDATA[
> > > +          return new Set(["addonbar-closebutton", "status-bar"]);
> > 
> > You're right, I think - this'll create a new Set every time. Maybe I meant a
> > read-only field instead of a property. :)
> 
> Can do, but then that means people would be able to modify it. Modifying
> something labeled 'whiteList' sounds scary, though I suppose in this case
> what'd happen is... items don't get migrated? Might not actually be bad.
> 

But that field can be made readonly with the readonly attribute, no? https://developer.mozilla.org/en-US/docs/XUL/Tutorial/Adding_Properties_to_XBL-defined_Elements#Readonly_Attribute

> (In reply to Mike Conley (:mconley) from comment #25)
> > (In reply to :Gijs Kruitbosch from comment #23)
> > > (In reply to :Gijs Kruitbosch from comment #22)
> > > OTOH, shouldn't we be running
> > > registerToolbar only after onreadystatechange, and shouldn't that take care
> > > of this? :-\
> > > 
> > 
> > Yes, I thought so... which add-on are you using to test this?
> 
> Stylish ( https://addon.mozilla.org/addon/stylish )
> 
> I've now dug a little deeper. It looks like when we hit the block here:
> 
> http://hg.mozilla.org/projects/ux/file/49e160b46dad/browser/components/
> customizableui/src/CustomizableUI.jsm#l722
> 
> for items in the add-on bar, the toolbox doesn't have a palette yet.
> However, when we hit it milliseconds later for the nav-bar, it does. Log:
> 
> [16:37:51.963] "[CustomizableUI]" "Saving state."
> [16:37:51.963] "[CustomizableUI]" "State saved as:
> {"placements":{"PanelUI-contents":["edit-controls","zoom-controls","new-
> window-button","privatebrowsing-button","save-page-button","print-button",
> "history-panelmenu","fullscreen-button","find-button","preferences-button",
> "add-ons-button"],"addon-bar":["stylish-toolbar-button","addonbar-
> closebutton","customizableui-special-spring13718254719631","status-bar"]},
> "seen":[]}"
> [16:37:51.963] "[CustomizableUI]" "Searching for stylish-toolbar-button in
> toolbox."
> [16:37:51.964] "[CustomizableUI]" "Toolboxes for window: 1"
> [16:37:51.964] "[CustomizableUI]" "No node for stylish-toolbar-button found."
> [16:37:51.964] "[CustomizableUI]" "Unknown widget: stylish-toolbar-button"
> [16:37:51.964] "[CustomizableUI]" "Searching for addonbar-closebutton in
> toolbox."
> [16:37:51.965] "[CustomizableUI]" "Searching for status-bar in toolbox."
> [16:37:51.965] "[CustomizableUI]" "Restoring PersonalToolbar from default
> state"
> [16:37:51.965] "[CustomizableUI]" "Placements for PersonalToolbar:
> 	personal-bookmarks"
> [16:37:51.966] "[CustomizableUI]" "Restoring nav-bar from default state"
> [16:37:51.966] "[CustomizableUI]" "Placements for nav-bar:
> 	unified-back-forward-button
> 	urlbar-container
> 	reload-button
> 	stop-button
> 	search-container
> 	webrtc-status-button
> 	bookmarks-menu-button
> 	downloads-button
> 	home-button
> 	social-share-button"
> [16:37:51.968] "[CustomizableUI]" "Searching for search-container in
> toolbox."
> [16:37:51.969] "[CustomizableUI]" "Toolboxes for window: 1"
> [16:37:51.969] "[CustomizableUI]" "Looking for search-container"
> [16:37:51.969] "[CustomizableUI]" "Have:
> search-container,bookmarks-menu-button,home-button,print-button,downloads-
> button,new-window-button,fullscreen-button,sync-button,tabview-button,
> stylish-toolbar-button"

(╯°□°）╯︵ ┻━┻

Comment 28

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

(In reply to :Gijs Kruitbosch from comment #26)
> 
> Stylish ( https://addon.mozilla.org/addon/stylish )
> 
> I've now dug a little deeper. It looks like when we hit the block here:
> 
> http://hg.mozilla.org/projects/ux/file/49e160b46dad/browser/components/
> customizableui/src/CustomizableUI.jsm#l722
> 
> for items in the add-on bar, the toolbox doesn't have a palette yet.
> However, when we hit it milliseconds later for the nav-bar, it does. Log:

Ah, ok - I can see this happening of the _init of the add-on bar is called first.

The other customizable toolbars set the toolbox palette in their _init methods if it isn't already there. I think the add-on bar binding should probably do the same.

Comment 29

[:Gijs (he/him)]

Attachment: Auto-migration patch v1.2

(In reply to Mike Conley (:mconley) from comment #27)
> (In reply to :Gijs Kruitbosch from comment #26)
> > (In reply to Mike Conley (:mconley) from comment #24)
> > > @@ +419,5 @@
> > > > +        ]]></getter>
> > > > +      </property>
> > > > +      <property name="_whiteListed" readonly="true">
> > > > +        <getter><![CDATA[
> > > > +          return new Set(["addonbar-closebutton", "status-bar"]);
> > > 
> > > You're right, I think - this'll create a new Set every time. Maybe I meant a
> > > read-only field instead of a property. :)
> > 
> > Can do, but then that means people would be able to modify it. Modifying
> > something labeled 'whiteList' sounds scary, though I suppose in this case
> > what'd happen is... items don't get migrated? Might not actually be bad.
> > 
> 
> But that field can be made readonly with the readonly attribute, no?
> https://developer.mozilla.org/en-US/docs/XUL/Tutorial/
> Adding_Properties_to_XBL-defined_Elements#Readonly_Attribute

Yes, but because the set is modifiable and kept by reference, they can still modify the set. I don't think that really matters though, so I've changed it.

This patch incorporates the toolbox.palette fix, which means I could get rid of the ugly querySelector call. Getting closer, I hope!

Comment 30

[:Gijs (he/him)]

Attachment: Auto-migration patch v1.2

It does help to update the closing </property>... errr, I mean </field> tag too. Actually built and ran this patch before submitting it!

Comment 31

[Justin Dolske [:Dolske]]

P1 for needing to fully understand the plan here.

Comment 32

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to :Gijs Kruitbosch from comment #29)
> Yes, but because the set is modifiable and kept by reference, they can still
> modify the set. I don't think that really matters though, so I've changed it.

This was doable in the first patch too ;) So yea, no big deal.

Comment 33

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to :Gijs Kruitbosch from comment #21)
> > @@ +404,5 @@
> > > +          return [node.id for (node of this.children)].join(',');
> > 
> > Shouldn't this take into account the whitelist?
> 
> I'm not sure what you mean here. Take it into account how/why?

I meant: Should currentset exclude items from the whitelist?

But, I see the current add-on bar doesn't. So may as well keep that behaviour, correct or otherwise.

Comment 34

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 766131 [details] [diff] [review]
Auto-migration patch v1.2

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

r+ with the proviso that when (/if) this lands, it gets Extra Special™ QA attention.

Comment 35

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

Comment on attachment 766131 [details] [diff] [review]
Auto-migration patch v1.2

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

LGTM with my last nit fixed - no need for further review.

I heartily agree with Blair - this is gonna need a ton of eyes on it.

::: browser/base/content/browser.css
@@ +27,5 @@
>    -moz-binding: url("chrome://browser/content/customizableui/toolbar.xml#toolbar-menubar-autohide");
>  }
>  
> +#addon-bar {
> +  -moz-binding: url("chrome://browser/content/customizableui/toolbar.xml#addonbar-delegating");

Final nit - these CSS rules should be in alphabetical order.

Comment 36

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) from comment #35)
> > +#addon-bar {
> > +  -moz-binding: url("chrome://browser/content/customizableui/toolbar.xml#addonbar-delegating");
> 
> Final nit - these CSS rules should be in alphabetical order.

I don't know where this idea came from, but I don't agree with it. There are often crucial properties, properties that are the crux of a given rule. It makes sense to put them first. It also often makes sense to group certain related properties, e.g. margin and padding or width and height.

Comment 37

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

(In reply to Dão Gottwald [:dao] from comment #36)
> (In reply to Mike Conley (:mconley) from comment #35)
> > > +#addon-bar {
> > > +  -moz-binding: url("chrome://browser/content/customizableui/toolbar.xml#addonbar-delegating");
> > 
> > Final nit - these CSS rules should be in alphabetical order.
> 
> I don't know where this idea came from, but I don't agree with it. There are
> often crucial properties, properties that are the crux of a given rule. It
> makes sense to put them first. It also often makes sense to group certain
> related properties, e.g. margin and padding or width and height.

I agree with Dao. The property list is usually at most 10 properties. Visually scanning a 10 item list will not be much faster whether it is ordered or not. However ordering these items alphabetically will separate related properties. 

It's also not clear what to do with prefixed properties. Should we put -moz-border-top-colors next to -moz-binding, or next to border-top-width? I don't understand why we would want it away from the border-top-width property.

Comment 38

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

(In reply to Dão Gottwald [:dao] from comment #36)
> (In reply to Mike Conley (:mconley) from comment #35)
> > > +#addon-bar {
> > > +  -moz-binding: url("chrome://browser/content/customizableui/toolbar.xml#addonbar-delegating");
> > 
> > Final nit - these CSS rules should be in alphabetical order.
> 
> I don't know where this idea came from,

MattN was the one who first mentioned this to me.

> but I don't agree with it. There are
> often crucial properties, properties that are the crux of a given rule. It
> makes sense to put them first.

I suppose? So, I imagine that the primary advantage of putting the properties in alphabetical order is that it makes it easier for someone to quickly determine if a property exists in the list without having to scan the whole block. I do think that has value.

On the other hand, I'm really not interested in arguing this point, as it's not something I feel too strongly about, and we've got a lot of work to do and not a lot of time to do it. :)

So please ignore my last request, Gijs.

Comment 39

[:Gijs (he/him)]

Pushed:

https://hg.mozilla.org/projects/ux/rev/dd59f8effb73
https://hg.mozilla.org/projects/ux/rev/ef0973113930

Comment 40

[:Gijs (he/him)]

Uhm, so the defaultPlacements being empty was obviously wrong and breaking our tests... not sure how I missed that when testing this. Anyway, bustage fix pushed with a quick r+ from mconley

https://hg.mozilla.org/projects/ux/rev/170da549f617

Comment 41

[:Gijs (he/him)]

Attachment: Don't leak

Annnnnnd then we had leak issues. Ugh. Eg.: https://tbpl.mozilla.org/php/getParsedLog.php?id=24639239&tree=UX

So internally, I suspect mutation observer targets keep references to their observers, so they can call them if anything mutates (duh). And in this case we were using a bound function which was keeping a reference to the node. And so we had a beautiful cycle from which we never free'd anything and so any test creating a new window leaked.

Anyhow, this stops using a bound function, which I've locally verified seems to fix this problem. However, I'd quite like not to do too much more dancing here so I've also fired off a try run including this patch (better late than never, or something): https://tbpl.mozilla.org/?tree=Try&rev=4a0efea25020

Sorry. :-(

Comment 42

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

Comment on attachment 768149 [details] [diff] [review]
Don't leak

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

Looks fine - just fix the nits, then land this puppy.

::: browser/components/customizableui/content/toolbar.xml
@@ +310,5 @@
> +          // We can't easily use |this| or strong bindings for the observer fn here
> +          // because that creates leaky circular references when the node goes away,
> +          // and XBL destructors are unreliable.
> +          let mutationObserver = new MutationObserver(function(mutations) {
> +            if (!mutations || !mutations.length) {

Based on my understanding of MutationObserver, there's never a case where mutations are undefined / null. It's always an Array - though it might be an empty one.

So I think we just need to account for the case where there are no elements in the Array, and that is all.

@@ +313,5 @@
> +          let mutationObserver = new MutationObserver(function(mutations) {
> +            if (!mutations || !mutations.length) {
> +              return;
> +            }
> +            let toolbar = mutations[0].target; 

Nit - trailing ws

Comment 43

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #42)
> Comment on attachment 768149 [details] [diff] [review]
> Don't leak
> 
> Review of attachment 768149 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks fine - just fix the nits, then land this puppy.
> 
> ::: browser/components/customizableui/content/toolbar.xml
> @@ +310,5 @@
> > +          // We can't easily use |this| or strong bindings for the observer fn here
> > +          // because that creates leaky circular references when the node goes away,
> > +          // and XBL destructors are unreliable.
> > +          let mutationObserver = new MutationObserver(function(mutations) {
> > +            if (!mutations || !mutations.length) {
> 
> Based on my understanding of MutationObserver, there's never a case where
> mutations are undefined / null. It's always an Array - though it might be an
> empty one.
> 
> So I think we just need to account for the case where there are no elements
> in the Array, and that is all.
> 
> @@ +313,5 @@
> > +          let mutationObserver = new MutationObserver(function(mutations) {
> > +            if (!mutations || !mutations.length) {
> > +              return;
> > +            }
> > +            let toolbar = mutations[0].target; 
> 
> Nit - trailing ws

Fixed, pushed: https://hg.mozilla.org/projects/ux/rev/72acd03fccd6

Comment 44

[Matthew N. [:MattN]]

After this change and bug 869543 (pre-Australis), I think the footer image of Lightweight Themes won't be visible anymore by default. I filed bug 898988 to deal with the footer end of life for Firefox.

Comment 45

[Cattleya]

I only want to say that: "Computer screen resolution is getting bigger and bigger, what is the reason to remove add-on bar to save some small space ?"

And add-on bar is very useful, basic user may not use more button, but advanced user can use a large number of button like this:  http://cdn.ghacks.net/wp-content/uploads/2013/07/firefox-ui-madness.png

This make FIrefox UI look terrible. Firefox is a browser used by almost advanced user, not like Chrome.

Firefox still can customize with userChrome.css or Stylish, if I use more button in Firefox add-on bar and still want to save space, simply I make add-on bar autohide, that much better than remove add-on and move all button to navbar, with >20 buttons icon, navbar is look really weird.

Comment 46

[Terrell Kelley]

This bug was WONTFIXED a long time ago under bug 869939. In fact, there were plenty of reports that this had been tabled for now.

And I just tried it in the latest UX build, and it made my navbar a cluttered mess. That's completely contrary to what you are supposedly doing.

Also, I notice that, probably because of https://groups.google.com/forum/#!topic/firefox-dev/ua6yBQk0E_s,  no one is working on an addon that will allow a bottom toolbar for those who still want it. This link has been widely published at places like http://www.ghacks.net/2013/05/15/good-news-firefoxs-add-on-bar-wont-be-removed-after-all-in-australis-redesign/

I'd personally be okay with Australis if it wasn't for you guys trying to kill that thing. I just don't get it. If people want to use it, let them use it. People explicitly use Firefox for the customization abilities, and Australis is supposed to make those better by making them easier. But if it takes away features, it is not accomplishing its goal.

Not that I see why another drag and drop interface is any better than the old drag and drop interface.

Comment 47

[Terrell Kelley]

Also, I note you haven't actually created the omnibar yet, which should be something you'd do before you landed this in UX.

Comment 48

[Sören Hentzschel]

(In reply to Terrell Kelley from comment #46)
> Also, I notice that, probably because of
> https://groups.google.com/forum/#!topic/firefox-dev/ua6yBQk0E_s,  no one is
> working on an addon that will allow a bottom toolbar for those who still
> want it.

It's not true.
http://aris-at-mozilla.blogspot.ca/

Comment 49

[Adam Porter]

I have yet to see a rationale for this change--that is, other than, "It would be nice," and, "The UX team wants this."  On the contrary, real users have expressed the heartfelt desire to keep the addon bar--but where are the real users demanding its destruction?

Mozilla seems to be bent on imitating Chrome.  Why?  If I wanted to use a browser with a crippled extension UI and API, I'd use Chrome.  I absolutely despise the way Chrome won't let extensions do anything other than put giant icons in a single place.  Now Firefox is going to do the same thing?  Firefox was supposed to be about putting me--the user--in control, and about giving addon developers power to extend Firefox in ways Mozilla couldn't imagine or spend time on.

This is very disappointing and frustrating.  I've been using Firefox since at least Phoenix 0.6.  Firefox was supposed to be about giving users the power to "Take back the web!"  That was even Firefox's official slogan for some time.  Now Mozilla seems to be taking it back--from its users.

Mozilla used to be leading the way, but now it's just following--and fads, at that.  I don't know if Google--Mozilla's primary source of funding--is leaning on Mozilla, or if Mozilla's simply lost its way, but there seems to be a leadership vacuum that is crying out for a return to the old ways, to the spirit which led to Firefox being created in the first place.  Will a new Phoenix rise from Firefox's ashes?  Time will tell.  But it's not too late for Mozilla to wake from its slumber--or is it a stupor?

Comment 50

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/dd59f8effb73
https://hg.mozilla.org/mozilla-central/rev/ef0973113930
https://hg.mozilla.org/mozilla-central/rev/170da549f617
https://hg.mozilla.org/mozilla-central/rev/72acd03fccd6

Comment 51

[Terrell Kelley]

(In reply to Adam Porter from comment #49)

> I have yet to see a rationale for this change--that is, other than, "It
> would be nice," and, "The UX team wants this."  On the contrary, real users
> have expressed the heartfelt desire to keep the addon bar--but where are the
> real users demanding its destruction?

I came up with one: they want to discourage people from using multiple addons. You can only fit maybe four up there comfortably before it looks like complete ****. And the devs are always on about how Firefox is faster and uses less memory than Chrome, but only if you don't use addons. 

So the idea, I think, is to create the illusion of more customization, bringing it out front and allowing the menu button to be customized, all while making regular users loathe to add more addons on a whim, as they'll just clutter everything up. I mean, notice how the replacement addon is developed by a dev and doesn't actually mention the Addon Bar, making people less likely to find it. Only those of us who are heavy users will know about it.

I mean, that makes a lot more sense than the devs thinking they have the right to thrust their aesthetic preferences on everyone else, doesn't it? Especially when literally all the feedback on the removal of the addon bar has been negative.  They must believe that there is some greater purpose that offsets the anger they are engendering in their core, most vocal, and most passionate userbase. They must think there's something better than allowing addon developers to keep using a toolbar they obviously like (or they wouldn't keep using it).

It sure isn't how hard it is to code the toolbar--as they are supposedly deliberately making that easier. Heck, that's the type of **** they should be eliminating. With a handful of exceptions, almost all addon toolbars are malware. And now other addon developers are being encouraged to make their own, instead of having a single target to contain anything that doesn't make sense to be on the primary toolbar.

Comment 52

[Lukas Blakk [:lsblakk] use ?needinfo]

We'll want to release note this starting from Firefox 29 when Australis ships

Comment 53

[Andy Mercer]

I'm going to leave this feedback here in hopes that someone involved with this might actually see. I thought about just ranting, but that is typically counter productive. So I'll just try to lay out my argument.

Firefox isn't Chrome. The best thing that FF has going for it is the customization abilities. Given that Chrome is backed by Google, there's no way to match the resources that can be thrown at it. You'll never blow them away in speed. Plus we are coming to the end of this round of the browser wars in general because we are reaching parity for the major aspects of HTML5 and CSS3.

The thing that FF has that Chrome doesn't is customization. I can add, delete, and move features as much as I want. I can get my screen to look exactly how I want. I can't do that in Chrome, so I've stuck with Firefox when many around me has gone to Chrome. Removing the Add-On bar is just one more nail in the coffin of said customization. The more you try to be Chrome, the less relevant Firefox becomes. The world doesn't need two Chromes.

Comment 54

[Paul Silaghi, QA [:pauly]]

I tested the top 20 addons from https://addons.mozilla.org/en-US/firefox/extensions/?sort=users on Australis vs Holly. 
Couple of Australis specific issues:
adblock plus - the widget is gone from the nav-bar after restart
video downloadhelper - the widget from the nav-bar doesn't work
tab mix plus - unable to switch tabs after install
personas plus - the widget doesn't appear in the nav-bar

Any thoughts?

Comment 55

[Dave Townsend [:mossop]]

(In reply to Paul Silaghi, QA [:pauly] from comment #54)
> I tested the top 20 addons from
> https://addons.mozilla.org/en-US/firefox/extensions/?sort=users on Australis
> vs Holly. 
> Couple of Australis specific issues:
> adblock plus - the widget is gone from the nav-bar after restart
> video downloadhelper - the widget from the nav-bar doesn't work
> tab mix plus - unable to switch tabs after install
> personas plus - the widget doesn't appear in the nav-bar
> 
> Any thoughts?

Make sure you're testing with a very recent nightly and if so please file bugs on these issues so we can look at them

Comment 56

[:Gijs (he/him)]

(In reply to Dave Townsend (:Mossop) from comment #55)
> (In reply to Paul Silaghi, QA [:pauly] from comment #54)
> > I tested the top 20 addons from
> > https://addons.mozilla.org/en-US/firefox/extensions/?sort=users on Australis
> > vs Holly. 
> > Couple of Australis specific issues:
> > adblock plus - the widget is gone from the nav-bar after restart
> > video downloadhelper - the widget from the nav-bar doesn't work
> > tab mix plus - unable to switch tabs after install
> > personas plus - the widget doesn't appear in the nav-bar
> > 
> > Any thoughts?
> 
> Make sure you're testing with a very recent nightly and if so please file
> bugs on these issues so we can look at them

Also note that the ABP and Video downloadhelper issues are already known and have bugs filed.

Comment 57

[Paul Silaghi, QA [:pauly]]

(In reply to :Gijs Kruitbosch from comment #56)
> Also note that the ABP and Video downloadhelper issues are already known and
> have bugs filed.
That's why I commented here and not directly filed the issues.
Anyway, I still can't find the ABP and Video downloadhelper issues.

(In reply to Paul Silaghi, QA [:pauly] from comment #54)
> personas plus - the widget doesn't appear in the nav-bar
Can't reproduce, but found something else related to personas.

I'll continue the top add-ons testing, but I need to know where I can find the known issues to avoid the duplicates.

Comment 58

[:Gijs (he/him)]

(In reply to Paul Silaghi, QA [:pauly] from comment #57)
> (In reply to :Gijs Kruitbosch from comment #56)
> > Also note that the ABP and Video downloadhelper issues are already known and
> > have bugs filed.
> That's why I commented here and not directly filed the issues.
> Anyway, I still can't find the ABP and Video downloadhelper issues.

I can't find a bug for the ABP issue offhand, but the issue has been noted in this bug (see also the detailed migration results attachment, and some of the comments below) as well as in the MDN add-on compatibility draft. The VDH issue is filed as bug 946583.

> (In reply to Paul Silaghi, QA [:pauly] from comment #54)
> > personas plus - the widget doesn't appear in the nav-bar
> Can't reproduce, but found something else related to personas.
> 
> I'll continue the top add-ons testing, but I need to know where I can find
> the known issues to avoid the duplicates.

Please file future issues as blocking the 'australis-addons' bug rather than this bug, including the two bugs that you just added to this one, as they're not automatically caused by the add-on bar removal per se. We (or add-on authors) should investigate the issues and figure out whether the bug is on our side or the add-on's, and prioritize/resolve the bug appropriately.

Comment 59

[Paul Silaghi, QA [:pauly]]

Thanks Gijs for explaining.

Comment 60

[Luís Miguel [:quicksaver] (not reading bugmail)]

(In reply to Sören Hentzschel from comment #48)
> (In reply to Terrell Kelley from comment #46)
> > Also, I notice that, probably because of
> > https://groups.google.com/forum/#!topic/firefox-dev/ua6yBQk0E_s,  no one is
> > working on an addon that will allow a bottom toolbar for those who still
> > want it.
> 
> It's not true.
> http://aris-at-mozilla.blogspot.ca/

And also https://addons.mozilla.org/en-US/firefox/addon/the-puzzle-piece/ , version 1.2 (just released) fully supports Australis and brings back the add-on bar and everything you'd need/want for it. Sorry for the spam, I thought I should mention it here.

Comment 61

[Bogdan Maris, Desktop QA]

Verified that the add-on bar is removed and installed top 20 add-ons to check if the buttons are moved to navigation bar. Testing was done on Windows 7 64bit, Ubuntu 13.10 64bit, Mac OS X 10.8.5 using latest Aurora.

Comment 62

[Nix\]

Some addons does not work in navigation bar... A LOT

Comment 63

[timo.fiersen]

I'm very disappointed to see that you have been deliberately ignoring all the negative input on this, why don't you people listen to your userbase anymore? Wasn't Mozilla about openness and community once?

There was absolutely no need whatsoever to remove the addon bar, there is no proper alternative, so removing it does only harm! Now I have 15+ huge-ass addon icons cluttering the navigation bar, which is pretty distracting to say the least.

Look at the feeback in news articles, forums, the firefox input portal, the skyrocketing stats for addons like status-4-evar (https://addons.mozilla.org/en-US/firefox/addon/status-4-evar/statistics/?last=30)... this should give you a hint that removing the bar wasn't a good idea.

Please, do us all a favour and revert this change.

Comment 64

[:Gavin Sharp [email: [e-mail]]]

We have not been ignoring the negative feedback.

We have heard it loud and clear, and decided not to address it with changes in Firefox proper. (As you mentioned, there are add-ons available to restore the functionality if you really want it.)

Producing software requires much more than "listening to your users" - it requires listening *and then making decisions*. There are users on both sides of any decision we make, and we don't always hear from them equally due to inherit bias in feedback mechanisms.

Comment 65

[Andy Mercer]

"There are users on both sides of any decision we make" 

I'm curious to hear what the feedback is on the pro side for removing it as an option. I can see where many people wouldn't want it visible, but what is the rationale for not even having it as an option? Does it significantly reduce code? Is it an upkeep question?

Comment 66

[Andrew Buehler]

(In reply to :Gavin Sharp (email [e-mail]) from comment #64)
> We have not been ignoring the negative feedback.
> 
> We have heard it loud and clear, and decided not to address it with changes
> in Firefox proper.

By at least one definition of the word, this qualifies as ignoring it.

I would also like clarification on how you *have* decided to address it, since the only means I can think of to address it without making changes to Firefox proper is by implementing an add-on to restore the missing functionality - and unless one or more of the existing add-ons is in fact backed by the Firefox developers, it's pretty much too late for doing that to make any difference, at least to Mozilla's public image. (And if one of the existing add-ons *is* backed by the Firefox developers, that fact is not being advertised well enough to offse the negative public-image impact of this change.)

(I had an extended futher comment on various parts of this, to the tune of about 3.5K of text, but although I do think it's worthwhile I also don't need the hassle which would come with trying to have that discussion here - especially since past experience indicates that barring a minor miracle, no useful result would come of it.)

Comment 67

[Matt A. Tobin]

(In reply to Andrew Buehler from comment #66)
> (In reply to :Gavin Sharp (email [e-mail]) from comment #64)

Save your breath. MozCo will do as it sees fit no matter what. I would recommend if you enjoy the changes to continue using the browser as usual but if you do not then don't. I would also recommend that everyone be careful about voicing their opinions on Bugzilla because this could very quickly turn into another "Tabs on Top controversy" which could result in being restricted or banned from buzilla.

Thank you for your time.

Peace,

Matt A. Tobin
<Self important title />
Binary Outcast

Comment 68

[:Gijs (he/him)]

(In reply to Andy Mercer from comment #65)
> "There are users on both sides of any decision we make" 
> 
> I'm curious to hear what the feedback is on the pro side for removing it as
> an option.

Speaking purely from what I've heard (and without intending to give a full and complete picture), many people feel that having add-on buttons share the main menu and the main navigation toolbar with the builtin buttons is an improvement, and that the loss of the awkward, on-its-lonesome/browser-with-a-beard toolbar-at-the-bottom is "good riddance".

> I can see where many people wouldn't want it visible, but what is
> the rationale for not even having it as an option? Does it significantly
> reduce code? Is it an upkeep question?

Yes and yes (as in, these are contributing factors), but not only. As long as we would have kept it around, add-ons would not have changed away from it. We tried to get rid of the statusbar aeons ago, and failed, and kept a shim around, and now, several years later, people were *still* relying on it. Statusbar add-ons weren't customizable, and integrating them properly with customization was very, very hard (also because the add-ons relied on their boxes never being somewhere else than in the status bar, in the add-on bar). There are some add-ons that have added the add-on bar back, and some have made the status bar movable. In my experience, many add-ons using statusbar elements did not cooperate with this (plus the statusbar looks horribly out of place styling-wise). We're already seeing lots of bugs filed about the few bits of UI that aren't customizable (the url bar, the menu button). Leaving an uncustomizable blob at the bottom would not have changed the status quo, and would not have helped add-ons become first-class citizens in the browser.

(In reply to Andrew Buehler from comment #66)
> I would also like clarification on how you *have* decided to address it,
> since the only means I can think of to address it without making changes to
> Firefox proper is by implementing an add-on to restore the missing
> functionality - and unless one or more of the existing add-ons is in fact
> backed by the Firefox developers, it's pretty much too late for doing that
> to make any difference, at least to Mozilla's public image. (And if one of
> the existing add-ons *is* backed by the Firefox developers, that fact is not
> being advertised well enough to offse the negative public-image impact of
> this change.)

I don't understand how add-ons being backed by Firefox developers or not makes a difference. The beauty of the open web, an open source browser and an add-on system is that anyone can fork, add to, subtract from, or adapt Firefox, not just people employed for, or voluntarily involved in, the development of the main Firefox codebase.

I know that the authors of several of the add-ons implementing replacements for the add-on bar have received guidance/help/support of one form or another, from people who were already core Firefox developers, and conversely, have provided feedback on APIs and other issues with the new release (so in a certain sense, they have become "Firefox developers") to the rest of the Firefox team. I don't think that that is relevant, though. We've always been very hands-off about "backing" add-ons beyond the "recommended" or "featured" add-ons system on AMO, but that isn't the same as them being "backed" by Firefox developers. I'm not really sure what kind of assurances/support from the core team you're looking for, or why.

Comment 69

[Andrew Buehler]

Add-ons being backed by Firefox developers doesn't make a direct, major difference to whether end users can achieve a browser configuration close to what they want (assuming the users can find the add-ons in the first place), except insofar as developers familiar with the code which was removed may be better able to implement an add-on with matching functionality than someone from outside who has to start from scratch. However, I believe it does make a major difference in what those users think of Firefox, its developers, and Mozilla as a whole - which bears heavily on the negative feedback being received.


Consider two scenarios.

In one, a feature is removed from Firefox, an add-on is implemented to replace it before the version with the removal is released live (with design and possibly coding help from Firefox developers, if indeed such developers are not the ones implementing the add-on to begin with), and official resources (including responses to complaints in places like this) point to that add-on as the official solution when people ask about the removed feature.

To the end user, that looks like "They didn't remove it, they just moved it into an add-on", or perhaps like "They removed it, but they took steps to make sure it wasn't entirely gone". The user's impression of Mozilla gets a negative impact from Mozilla having removed the feature from the core, offset by a positive impact from Mozilla having provided an add-on to restore the removed feature. At worst there's a minor net negative, and there could even be a positive boost overall.

In the other scenario, a feature is removed from Firefox, and a third-party developer implements an add-on to replace it.

To the end user, that looks like "They removed it, and this unrelated person put in the work to fix what they broke." The user's impression of Mozilla gets the negative impact from Mozilla having removed the feature from the core, without any offsetting positive impact. This negative impact is likely to carry over to the user's impression of Firefox, and pile more weight on the camel's back of the user's potential decision to switch to using something else.


I've described this various ways (some better than this) in various contexts in the past, some of which never made it out of my own head, but AFAIR I've never gotten much in the way of response. I think this is a very important sub-component of the also very important issue of what looks to me like Mozilla's large, and growing, public image problem.

The fact that people can say things like those in comment 67 ("MozCo will do as it sees fit no matter what") in all sincerity should by itself be an indication of the existence, and to some extent the nature, of that problem. Addressing the problem is much another question, but I'm not sure I've yet seen any indication that anyone is even trying to pursue that.

(Again, I could go on at considerably greater length, both about the public-image problem I see and about various issues related to the removal of the add-on bar, but I question the utility of doing so here. Unfortunately, I also don't know of anywhere else where such discussion would be neither actively unwelcome nor "shouting into the void" - which is itself a manifestation of another part of the image problem.)

Comment 70

[Dave Garrett]

(In reply to Andrew Buehler from comment #69)
Very well said. Thank you.

Nonetheless, this is a closed bug, so debating here isn't all that productive.

Comment 71

[Andrew Buehler]

I think I did also say that, actually - at least the latter part.

Comment 72

[:Gavin Sharp [email: [e-mail]]]

(In reply to Andrew Buehler from comment #69)
> The fact that people can say things like those in comment 67 ("MozCo will do
> as it sees fit no matter what") in all sincerity should by itself be an
> indication of the existence, and to some extent the nature, of that problem.
> Addressing the problem is much another question, but I'm not sure I've yet
> seen any indication that anyone is even trying to pursue that.

Given the size of our community and user base, the only alternative to having people who think and say those things is to do absolutely nothing, make no changes, and let Firefox die a slow, stagnant death. We're not going to pick that choice.

You can absolutely argue that our threshold for tolerance of dissent is too great, but if you want to do that it's important for you to question how much of the feedback you're seeing, how representative it is of broader sentiment, and which sources of feedback you're not considering. It's very easy to see 10, or 100, or 1000 people passionately commenting on a bug/forum/tech article and draw conclusions about the "obvious mistake" we made (particularly when those people agree with you), but it's much harder to weigh that feedback against the benefits we think a change might have to our ability to continue improving Firefox, or the benefit of those improvements to the silent majority of users not submitting that feedback.

This bug really isn't the place to discuss this. I'd be happy to continue discussion on this topic on firefox-dev - please post there if you want to follow up.