In toolbox.jsm, we set the ordinal attribute to sort the tools according to their ordinal property. This causes too many problems: - styling is hard (can't use first-child / last-child) - menuitem are not ordered the same way (bug 818444) - using arrow keys in the tabs follows the DOM order, not the ordinal order Let's just sort in JavaScript.

This patch has the issue that panels added after the creation of a toolbox won't be sorted correctly. I suspect this will only affect testing, and tools added from addons on first install (and only then, when there is already a toolbox open). My hunch is that this is a minor issue, enough that this fix should go in as is.

Better patch that includes a fix for bug 818444

Comment on attachment 699690 [details] [diff] [review] v2 If I'm not mistaken, this won't work for dynamic tool registration. If an addon registers a tool with ordinal=0, it will be added at the end.

(just saw your comment about addons, let me think about it)

> My hunch is that this is a minor issue, enough that this fix should go in as is. If we can't control the position via `ordinal` for dynamic tools, then it's better to just drop the `ordinal` property and just sort the builtin tools the way we want. How hard is it to use insertBefore()?

(In reply to Paul Rouget [:paul] from comment #5) > > My hunch is that this is a minor issue, enough that this fix should go in as is. > > If we can't control the position via `ordinal` for dynamic tools, then it's > better to just drop the `ordinal` property and just sort the builtin tools > the way we want. > > How hard is it to use insertBefore()? It's not hard. It's just that if we had a bug "When I add a tool dynamically whilst a toolbox is open then it is sorted in the wrong place until I reopen the toolbox" then I think we'd give it quite low priority. I'm assigning it the same priority :)

(In reply to Joe Walker [:joe_walker] [:jwalker] from comment #6) > (In reply to Paul Rouget [:paul] from comment #5) > > > My hunch is that this is a minor issue, enough that this fix should go in as is. > > > > If we can't control the position via `ordinal` for dynamic tools, then it's > > better to just drop the `ordinal` property and just sort the builtin tools > > the way we want. > > > > How hard is it to use insertBefore()? > > It's not hard. It's just that if we had a bug "When I add a tool dynamically > whilst a toolbox is open then it is sorted in the wrong place until I reopen > the toolbox" then I think we'd give it quite low priority. I'm assigning it > the same priority :) We shipped Firefox 20 with this feature. Addons might rely on it. Here you're breaking a working feature to fix the menu. So you can file a follow-up bug, but we will need to fix it by Firefox 21. Or you fix it now. Up to you.

Comment on attachment 700356 [details] [diff] [review] v3 Thank you. r+ > _addToolToWindows: function DT_addToolToWindows(toolDefinition) { >+ // We need to insert the new tool in the right place, which means knowing >+ // the tool that comes before the tool that we're trying to add >+ let allDefs = gDevTools.getToolDefinitionArray(); >+ let prevDef; >+ for (let def of allDefs) { >+ if (def === toolDefinition) { >+ break; >+ } >+ prevDef = def; >+ } Would that work: prevDef = allDef[allDefs.indexOf(toolDefinition) - 1]

fwiw: https://tbpl.mozilla.org/?tree=Try&rev=22d48402e458

(In reply to Paul Rouget [:paul] from comment #9) > Comment on attachment 700356 [details] [diff] [review] > v3 > > Thank you. r+ > > > _addToolToWindows: function DT_addToolToWindows(toolDefinition) { > >+ // We need to insert the new tool in the right place, which means knowing > >+ // the tool that comes before the tool that we're trying to add > >+ let allDefs = gDevTools.getToolDefinitionArray(); > >+ let prevDef; > >+ for (let def of allDefs) { > >+ if (def === toolDefinition) { > >+ break; > >+ } > >+ prevDef = def; > >+ } > > Would that work: > prevDef = allDef[allDefs.indexOf(toolDefinition) - 1] Probably, but paranoia made me do the longer version as it's safe if toolDefinition isn't found. In the short version you end up with allDef[-2].

https://tbpl.mozilla.org/?tree=Fx-Team&rev=fd6d84e5bf49 https://hg.mozilla.org/integration/fx-team/rev/fd6d84e5bf49

Comment on attachment 700356 [details] [diff] [review] v3 [Approval Request Comment] Bug caused by (feature/regressing bug #): new feature (toolbox) User impact if declined: tools are not ordered correctly in the menu Testing completed (on m-c, etc.): m-c Risk to taking this patch (and alternatives if risky): low String or UUID changes made by this patch: no