1058990 - Allow special widgets back into menu panel

Status: RESOLVED WONTFIX

(Firefox :: Toolbars and Customization, defect)

Description

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

(Boy am I late to this party...)

I urge you to reconsider bug 1003588. As GiT mentioned in bug 1003588 comment 13, this becomes very troublesome for add-ons that allow for special widgets to be used in the menu panel.

As it is right now, there are only two solutions that I see to overcome this limitation:

1) As GiT suggested in that same comment (link to code), replace/alter the relevant methods in CustomizableUIInternal. This is just error-prone, messy, extremely high maintenance as it carries the possibility of keeping different versions of the methods for different versions of firefox, and if more than one add-on needs to do the same (for whatever reason) it becomes next to impossible to handle.

I don't mean to sound harsh (really, at all, I just want to make my point) but I really don't see any other way of phrasing this; I've been following at least part of firefox development for a few years now, and I don't believe anyone here would want to do this type of thing from within firefox code for any reason, so why should we? Rule of thumb, if it's a bad-coding habit for you then it's the same for us.

2) Create our very own "special widget handling system" and somehow make it work with CUI.

At the very least this would involve injecting our own information into the CUI prefs that hold the placements, and that's just a customization corruption waiting to happen. Not to mention the incredible amount of checking that would just be a repeat of what's already in CUI and, thus, a waste of code. I'd rather use what already exists than clone it on my own file...

0) Adding our own five or six repeats of a clone of a "special" widget just so there's more than one of them available to use. I'm making this point 0 because I don't consider this a viable option. It's just cluttering the customizable area and plain old ugly, definitely not what Australis stands for.

Jared, you mentioned (bug 1003588 comment 14) that add-ons need to go the full distance to add whatever functionality they want to add, and I do agree with that principle. However, I also agree with GiT. Either of the two choices I mentioned above seem more like "a trip around the planet" rather than just "going the full distance".

For example, CustomizableUIInternal is frozen and available only from within the module's scope (not exported), which is a way of saying "don't mess with this". So you can't say we should go the full distance when you place a road block ahead of us (jumping over it with point 1 isn't a proper solution like I mentioned).

And CustomizableUI.jsm alone has over 4000 lines of code; point 2 would essentially repeat some of it and effectively counter a lot of the rest. I do have a lot of CUI code still stuck in my head from having read it several times over for months... I wouldn't want to do that if you paid me, CUI was not written to be countered. When the machines rise, CUI will slaughter us all, with rainbow colored pickaxes just for fun. :)

Honestly, I'm tempted to remove this ability from my add-on altogether, as it's just not worth the incredible amount of effort necessary to make it happen like this. Especially because I don't understand why, instead of the patch that was introduced in bug 1003588, it could have just added a simple "display:none;" for separators/spacers/springs in the menu panel; it's much easier to override, the visual glitches would be gone, both in and out of customize mode, so they wouldn't cause any bother for the rest of the customize system (even if they did, a hit on the "restore defaults" button would still physically remove them and the user would never see them again, as they don't re-appear in the customization area).

Comment 1

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

Gijs, Jared, I'm needinfo'ing you because you were the most behind bug 1003588 and I would like to know if I can change your opinion on the matter.

Comment 2

[GiTcode]

(In reply to Luís Miguel [:Quicksaver] from comment #0)

Exactly!


I did find a way to not have to replace/alter the build in methods by hand in a way that makes it not quite as high maintenance, but it is still very ugly and hackish. (https://addons.mozilla.org/en-US/firefox/files/browse/260722/file/modules/spacers.js#top  [not quite finished, but demonstrates the point]).

Classic Theme Restorer is also affected by this.

Comment 3

[:Gijs (he/him)]

The reason to not "just" go for display: none is that that wouldn't fix the panel rearranging/positioning code that deals with long/short widgets and how to avoid gaps in the menu panel, so it's not as simple as you describe.

We would then have to fix that code to ignore the items, too, but add-ons that replace the functionality would face the same hurdles as you describe with CUIInternal to overcome that limitation (and implement it correctly, whatever that means - it'd depend on how they visualize the special items in the menupanel, which is not entirely clear to me anyway).

That's the third point:

1) creating small (one-item-big) gaps (in other words, horizontal space) is an explicit non-goal;
2) creating vertical lines (as toolbarseparators are wont to do) would bust the horizontal layout, which is almost pixel-perfect)
3) stretched (flex) vertical space I don't think will work, but even if you manage to make it work, I'm willing to bet it breaks the height calculating code for the animations in certain cases.

so I guess I also don't really understand how you'd sensibly implement these in the menupanel anyway.

Finally, I just don't think I see an actual *point* in having these items in the menupanel.

If we're doing anything here, I think we should provide a boolean toggle for add-ons that allows them to turn this restriction off (probably as an attribute on the panel that is persisted so that it's available on startup) - but it wouldn't fix all of the above points, so at the minute I see little reason for us to do that.

Comment 4

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

Attachment: Sepcial Widgets In Menu Panel (The Puzzle Piece)

(In reply to :Gijs Kruitbosch from comment #3)
> The reason to not "just" go for display: none is that that wouldn't fix the
> panel rearranging/positioning code that deals with long/short widgets and
> how to avoid gaps in the menu panel, so it's not as simple as you describe.

I hadn't noticed that "display:none" brought these issues. Of course, without a valid alternative like I thought this was, the easiest solution on your side would be to just disallow them in the panel altogether.

> whatever that means - it'd depend on how they visualize the special items in
> the menupanel, which is not entirely clear to me anyway).

FWIW, screenshot of how The Puzzle Piece used to do this. I can see the separator being at least useful in terms of organization. The spacers/springs I admit I added support for mostly on a "already doing the separators, so why not" whim.

> If we're doing anything here, I think we should provide a boolean toggle for
> add-ons that allows them to turn this restriction off (probably as an
> attribute on the panel that is persisted so that it's available on startup)
> - but it wouldn't fix all of the above points, so at the minute I see little
> reason for us to do that.

This would be fine by me. And at least considering The Puzzle Piece's layout, there wouldn't be much need for all those fixes you mention, not on your or the add-on's side I think.

By just making the separator a wide widget, it would behave correctly in all those situations. (I'd just have to make sure its height matches the height of the other wide widgets, but that's for the add-on to do, not you of course.)

And the user could then use the spacer for fine-tuning their menu; it is already dimensioned as a normal widget, so it doesn't interfere with anything else, or it shouldn't at least.

Personally I would just remove the spring support from the menu if this was the case, as in this case I agree with you, I don't see how it would help in any way in the menu panel.

I'm not sure if GiT (The Add-on Bar Restored) or Aris (Classic Theme Restorer) will agree with my opinions though, as I don't remember of my head how their add-ons implement this or how they style special widgets in the menu panel.

Comment 5

[Aris]

Hi,

the only reason menu panel support for special widgets was added to CTR was users ability to move them there, if using CTR in that case. Mainly springs, spaces and separators required css adjustment to be displayed (almost) correctly and usable, nothing more (similar to what TPP does).

While on the one hand giving users more customization possibilities is a good thing, I'm not sure how many even use or are interested in using special widgets inside menu panel.

So I'm fine with both giving an option to allow them to be used there or permit usage in menu panel at all.

If not using the default spring, space or separator xul items and creating own menuitems that look similar, the current 'restriction' can be bypassed.

Comment 6

[:Gijs (he/him)]

We now have some builtin special widgets, but they still can't go into panels.