872209 - Make the add-on bar a customization target

Status: RESOLVED WONTFIX

(Firefox :: Toolbars and Customization, defect)

Description

[:Gijs (he/him)]

Per the discussion in https://groups.google.com/forum/?fromgroups=&hl=en#!topic/firefox-dev/ua6yBQk0E_s, we're going to make the addon bar a customization target instead of removing it.

Comment 1

[xyz]

Thank you so much for this, it is not only a step into the right direction for users - it is also one for add-on creators and removing a bit more complexity out of the field what would have been necessary if people suddenly would have been forced to create for their own addons new toolbars.

With a bit bad luck a user would suddenly been forced to endure various bars at once, depending on the used addons.

Anyway, a step into the right direction. Seems logical sense and Reason are still around! Perhaps you rethink also a bit your ways of locking down UI Elements or removing Small Icons.

After all, the more Addons a User is using to restore the old amount of abilities to change the browser for his needs, the more it increases the lack of speed and stability.

And it is in Mozilla's best interest to be as much competitive with Chrome without the need to remove Features which have been into the Browser since quite some time!

Comment 2

[:Gijs (he/him)]

Attachment: Patch

This works, except when it doesn't.

Special widgets get generated IDs. Those don't work with getPlacementOfWidget because they don't match the ID in the placements in an area.

How does that break anything? Well, the add-on bar has a spring, which takes up almost the entire drop area on the add-on bar. Dropping an item there leads to the customization code bailing out here:

http://hg.mozilla.org/projects/ux/file/e545d3da3300/browser/components/customizableui/src/CustomizeMode.jsm#l596

because no placement is returned as the generated ID doesn't match the 'proper' ID of 'spring'.

One way to fix this would be to specialcase generated IDs in there and strip out the generated part of the ID. Unfortunately that doesn't then work out if we have multiple springs (or any other special item) in one toolbar.

I'm not sure why we're getting the placement info by getting the widget position in gPlacements? Why don't we use the targetNode's index in its parent's childNodes? Are there cases where there is a discrepancy there? That seems more robust than the juggling the IDs, but I may be misunderstanding how this code is set up. :-)

(the reason I'm still putting up this patch is that I think it's technically orthogonal; you can still check this patch works by dropping a new item on the addonbar-closebutton instead)

Comment 3

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

(In reply to :Gijs Kruitbosch from comment #2)
> Created attachment 749825 [details] [diff] [review]
> Patch
> 
> This works, except when it doesn't.

tautology = tautology. :)

> I'm not sure why we're getting the placement info by getting the widget
> position in gPlacements? Why don't we use the targetNode's index in its
> parent's childNodes? Are there cases where there is a discrepancy there?
> That seems more robust than the juggling the IDs, but I may be
> misunderstanding how this code is set up. :-)
> 

It took me a little while to remember why we did it this way. It has to do with the fact that the customization target for the nav-bar used to be only a little block, containing a few items to the right of the URL bar, before bug 864355 landed.

Back then, the initial state for the nav-bar was still copied over from the currentset attribute of the toolbar, and things that weren't available to be put into the little customization target (like the back/forward button, url bar, etc) were simply ignored. But they were kept in the state.

This meant that the state for the customization target would be like:

back/forward button, urlbar, stop, reload, downloads, bookmarks, home, search

And the customization target node would only have the downloads, bookmarks, home, and search nodes in it.

Then, when customizing, if we used the childnode index to put some newbutton before, say, the bookmarks button, then the index of the new item would be 1.

But that'd inject into the state as:

back/forward button, newbutton, urlbar, stop, reload, downloads, bookmarks, home, search

Which, when rebuilding the nav-bar customization target, since it's skipping the items it can't suck out, would put the following items into the target:

newbutton, downloads, bookmarks, home, search

Which is not what the user intended.

This is all scenery. Now that the currentset is supposed to match the contents of the customization target, this should no longer be an issue, so we can probably use the index here and not get burned.

Comment 4

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

Comment on attachment 749825 [details] [diff] [review]
Patch

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

Yep, this is the right idea. Can you also try using the childnode index in CustomizationMode.jsm's _onDragDrop, and make sure that works?

Comment 5

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #4)
> Comment on attachment 749825 [details] [diff] [review]
> Patch
> 
> Review of attachment 749825 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yep, this is the right idea. Can you also try using the childnode index in
> CustomizationMode.jsm's _onDragDrop, and make sure that works?

Yeah, that helps, but it's not enough because in onWidgetAdded/onWidgetMoved we use the next node's ID again, which fails again, which means the node ends up at the end rather than where you dropped it.

I think there we could also use childNodes, I guess? Although that somehow scares me a bit more in case stuff is out of sync or something. Maybe that's an irrational fear, I'll try changing it and seeing how much I can break if I do that... :-)

Comment 6

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #5)
> (In reply to Mike Conley (:mconley) from comment #4)
> > Comment on attachment 749825 [details] [diff] [review]
> > Patch
> > 
> > Review of attachment 749825 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Yep, this is the right idea. Can you also try using the childnode index in
> > CustomizationMode.jsm's _onDragDrop, and make sure that works?
> 
> Yeah, that helps, but it's not enough because in onWidgetAdded/onWidgetMoved
> we use the next node's ID again, which fails again, which means the node
> ends up at the end rather than where you dropped it.
> 
> I think there we could also use childNodes, I guess? Although that somehow
> scares me a bit more in case stuff is out of sync or something. Maybe that's
> an irrational fear, I'll try changing it and seeing how much I can break if
> I do that... :-)

There's a second bug in there it looks like, because the insertion point fetches by ID but some items are wrapped in a toolbarpaletteitem, which means insertBefore bails out because the node isn't a direct child of the area container.

Comment 7

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #6)
> (In reply to :Gijs Kruitbosch from comment #5)
> > (In reply to Mike Conley (:mconley) from comment #4)
> > > Comment on attachment 749825 [details] [diff] [review]
> > > Patch
> > > 
> > > Review of attachment 749825 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Yep, this is the right idea. Can you also try using the childnode index in
> > > CustomizationMode.jsm's _onDragDrop, and make sure that works?
> > 
> > Yeah, that helps, but it's not enough because in onWidgetAdded/onWidgetMoved
> > we use the next node's ID again, which fails again, which means the node
> > ends up at the end rather than where you dropped it.
> > 
> > I think there we could also use childNodes, I guess? Although that somehow
> > scares me a bit more in case stuff is out of sync or something. Maybe that's
> > an irrational fear, I'll try changing it and seeing how much I can break if
> > I do that... :-)
> 
> There's a second bug in there it looks like, because the insertion point
> fetches by ID but some items are wrapped in a toolbarpaletteitem, which
> means insertBefore bails out because the node isn't a direct child of the
> area container.

... and I'm confused some more because the +1 stuff in there means we insert *after* the target node, whereas the code here:

http://hg.mozilla.org/projects/ux/file/a645c1fa2810/browser/components/customizableui/src/CustomizeMode.jsm#l645

and the black bar I see on Windows (not showing up on OS X for some reason?) appear *before* the node I'm hovering over. So AFAICT we don't need the +1? Or, perhaps more likely, there's still something to this API that I don't quite understand.

Comment 8

[:Gijs (he/him)]

Attachment: Patch v2

This fixes the issues I think I see per the previous comments by using container.childNodes in the onWidgetAdded, onWidgetMoved and _onDragDrop methods of the two jsms. As far as I can tell, this fixes everything to behave as I would expect it to.

NB: applied on top of the patch in bug 855683 (as they touch the same nextNode bits in onWidgetAdded/onWidgetMoved, I had to make a decision either way).

Comment 9

[:Gijs (he/him)]

Comment on attachment 750467 [details] [diff] [review]
Patch v2

Clearing this for now.

Comment 10

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

This bug depends on a decision on what to do with the add-on bar. UX, Jetpack, Product and Engineering are batting around ideas. In the meantime, this is slipping to M6.

Comment 11

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #10)
> This bug depends on a decision on what to do with the add-on bar. UX,
> Jetpack, Product and Engineering are batting around ideas. In the meantime,
> this is slipping to M6.

And now to M7, as we still don't have a decision and won't until next week's meeting at the earliest, which is post-M6.

Comment 12

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

Dolske has given us the OK to WONTFIX this.

Comment 13

[Chris]

is this a dumb question?

but why fix whats not broken.

Whats the justification to rip apart an existing UI users are used to?

Is there some hidden discussion thats took place somewhere that says people hate the existing UI or something?

I do know when FF25 hits a lot of people are going to be mighty annoyed and its a sure fire way to reduce firefox's userbase further.

I want a bar at the bottom of my browser that is used to display status of various addons I also have status4ever keeping my proper functions which were again wrongly removed in an earlier version of firefox.

Comment 14

[xyz]

Firefox just wants to be Chrome. Just take a look at what Firefox has and Chrome not. Compare what Firefox does not have after Australis has hit. You have your answer!

Comment 15

[xyz]

Solution.. Switch to Palemoon or Cyberfox :D

Comment 16

[Chris]

indeed I am looking into palemoon atm, as well as the ESR buildl.

But palemoon atm their solution is to use ESR, what happens when the ESR expires?

Whats happening to firefox is just painful to watch, its a clear copycat situation of chrome.

Comment 17

[xyz]

The Palemoon Modder will add back all the missing features, same do Cyberfox Modders. No matter how deep Australis will be integrated into Firefox, they will remove, it may be a lot of work, but it can be removed. Mozilla for sure will it make as complicated as possible to clean the Code, since they are for sure no fans of such Australis free Alternatives!

Comment 18

[rexyrexy2]

They just want to be chrome for NO REASON AT ALL. Removing the add-on bar IS THE MOST INSANE IDEA EVER!!! what if you have many add-ons? by taking this and custom toolbars AND the bookmark bar away, you are essentially taking away much of what makes firefox stand out. This is very similar to opera switching to webkit. it takes away what makes the browser what it is. Firefox has always had the ability to comepletely change to older looks. this is SHUNNING that user ability to change it back to the old look. Plus I don't want palemoon as it has many, many, many differences and removed features.

Comment 19

[xyz]

Just post in Blogs, Forums, tell Friends that people should use Palemoon or Cyberfox instead Firefox. They are 1:1 Forks, without Chrome like Changes, stop complaining here and do your part ;)