982005 - Toolbar should have an optional close button

Status: RESOLVED WONTFIX

(Firefox :: Toolbars and Customization, defect)

Description

[Matteo Ferretti [:zer0] [:matteo]]

TL;DR:
As shown in the Australis UX mockup:

http://people.mozilla.org/~shorlander/files/addons-in-toolbar-i01/images/04.png

Toolbar should have the capability to display a close button on the right, leaving everything on the left.

Currently SDK situation:

In order to do so, without using a spacer in the middle – 'cause adding "frame" with flex 1 should takes all the space available – we have a container in the toolbar where put the icons / elements but close button.
However, to use the CustomizableUI capability, the container itself is a Toolbar – resulting therefore in a toolbar inside a toolbar – an hack that we would prefer to avoid, having a proper XBL instead.

Comment 1

[:Gijs (he/him)]

I've said before that for a non-customizable toolbar such as the SDK ones, using CustomizableUI is a bad idea.

However, if you can't reverse that decision for some reason, CustomizableUI already lets you do what you want. You should specify the customizationtarget attribute and indicate the ID of the container of the customizable box. Anything outside of that container won't be used. So no, your inner container doesn't need to be a toolbar and so on. It does need to have an ID. See the nav-bar and its customization target (and the stuff outside it) for an example of how this works.

Comment 2

[Matteo Ferretti [:zer0] [:matteo]]

I don't think should be resolved as INVALID yet; at least I'd like having some discussion with the rest of the team here about it – therefore I will change to UNCONFIRMED, instead of NEW.

Where your suggestion will surely improve the code we have now in SDK – and I will be sure we'll refactoring that – we believe that such component should be present as toolbar XBL because UX states that all toolbars created by add-on should have consistent user experience displaying a close button on the right. Because add-ons are not necessary made all in jetpack, but also old-school XUL approach, we should have this feature as a toolbar XBL component itself, for the developers that are building add-ons without jetpack.
It would be also made SDK code cleaner, because we just reuse the same XBL.

I'm adding Irakli in Cc, because the original discussion was started with him.

Comment 3

[Matteo Ferretti [:zer0] [:matteo]]

Notice that this bug was also created for having a bug number, as reminder in a comment in the current jetpack: we think that the cleaner solution is having an XBL for that, and we'll probably write it ourselves anyway, once some other priorities are over, unless from the discuss we'll find that is the wrong approach.

Comment 4

[:Gijs (he/him)]

Adding XBL types is likely to be problematic because it means we multiply the already significant quantity of toolbar bindings by 2. (ones that are draggable or not, customizable or not, etc...)

It's also impractical because for the customizable ones, you'd never want the close button inside the customization target. Equally, if I understand the aim of this bug correctly, we'd need to ensure that consumers don't have to do stuff themselves. However, for that purpose we can't do that, because if the binding includes the customization target, it'll need an ID. It can't know that ID because it has to come from outside the binding. You could probably use xbl:inherits for this, but then, because it's now an anonymous element, I suspect getElementById() still won't find it, and the CustomizableUI code will bail out because of the non-existing customizationtarget.

So even if we add the close button, add-ons would have to do a lot of work to get the customization target stuff right - while it's arguably harder to understand why they would, because now there's a "magical" close button which is inserted by the binding.

Hardcoded inner content also creates interesting issues with events (see e.g. the mess with the bookmarks menu button and overflow).

It's much simpler for individual consumers to just add the item outside their customizable area if they need one. There is already a generic class that you can use to get the generic close icon. I strongly feel adding extra XBL bindings for this would be serious overkill.

As for other non-jetpack add-ons, you're assuming they would do the work required to use such a binding, and/or that we could produce a sufficient quantity of bindings that they could, even though different add-ons have differing requirements. I don't think either of these assumptions are correct.

Seeing as it's already possible to do everything you need in a non-hacky way, I really don't think this is worth engineering time, so if you're objecting to INVALID, I'd suggest WONTFIX.

Comment 5

[Dave Townsend [:mossop]]

Doesn't seem like it makes sense to do this to the default toolbar. Matteo, if we want a custom XBL widget that the SDK API uses to make the code less hacky then we should do that.