823916 - [toolbox] don't use ordinal

Status: RESOLVED FIXED

(DevTools :: Framework, defect, P2)

Description

[Paul Rouget [:paul]]

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.

Comment 1

[Joe Walker [:jwalker]]

Attachment: v1

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.

Comment 2

[Joe Walker [:jwalker]]

Attachment: v2

Better patch that includes a fix for bug 818444

Comment 3

[Paul Rouget [:paul]]

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.

Comment 4

[Paul Rouget [:paul]]

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

Comment 5

[Paul Rouget [:paul]]

> 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()?

Comment 6

[Joe Walker [:jwalker]]

(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 :)

Comment 7

[Paul Rouget [:paul]]

(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 8

[Joe Walker [:jwalker]]

Attachment: v3

Comment 9

[Paul Rouget [:paul]]

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]

Comment 10

[Joe Walker [:jwalker]]

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

Comment 11

[Joe Walker [:jwalker]]

(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].

Comment 12

[Joe Walker [:jwalker]]

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

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/fd6d84e5bf49

Comment 14

[Paul Rouget [:paul]]

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

Comment 15

[Paul Rouget [:paul]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9ce9e6614f92