Closed Bug 823916 Opened 7 years ago Closed 7 years ago

[toolbox] don't use ordinal

Categories

(DevTools :: Framework, defect, P2)

x86
All
defect

Tracking

(firefox20 fixed)

RESOLVED FIXED
Firefox 21
Tracking Status
firefox20 --- fixed

People

(Reporter: paul, Assigned: jwalker)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Component: Developer Tools → Developer Tools: Framework
Priority: -- → P2
Assignee: nobody → jwalker
Attached patch v1 (obsolete) — Splinter Review
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.
Attachment #699669 - Flags: review?(paul)
Attached patch v2 (obsolete) — Splinter Review
Better patch that includes a fix for bug 818444
Attachment #699669 - Attachment is obsolete: true
Attachment #699669 - Flags: review?(paul)
Attachment #699690 - Flags: review?(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.
Attachment #699690 - Flags: review?(paul) → review-
(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.
Attached patch v3Splinter Review
Attachment #699690 - Attachment is obsolete: true
Attachment #700356 - Flags: review?(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]
Attachment #700356 - Flags: review?(paul) → review+
(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://hg.mozilla.org/mozilla-central/rev/fd6d84e5bf49
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 21
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
Attachment #700356 - Flags: approval-mozilla-aurora?
Attachment #700356 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [land-in-aurora]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.