[toolbox] don't use ordinal

RESOLVED FIXED in Firefox 20

Status

P2
normal
RESOLVED FIXED
6 years ago
6 months ago

People

(Reporter: paul, Assigned: jwalker)

Tracking

Trunk
Firefox 21
x86
All

Firefox Tracking Flags

(firefox20 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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
(Reporter)

Updated

6 years ago
Priority: -- → P2
Assignee: nobody → jwalker
Created attachment 699669 [details] [diff] [review]
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.
Attachment #699669 - Flags: review?(paul)
Created attachment 699690 [details] [diff] [review]
v2

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

Comment 3

6 years ago
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-
(Reporter)

Comment 4

6 years ago
(just saw your comment about addons, let me think about it)
(Reporter)

Comment 5

6 years ago
> 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 :)
(Reporter)

Comment 7

6 years ago
(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.
Created attachment 700356 [details] [diff] [review]
v3
Attachment #699690 - Attachment is obsolete: true
Attachment #700356 - Flags: review?(paul)
(Reporter)

Comment 9

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 21
(Reporter)

Comment 14

6 years ago
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+
(Reporter)

Updated

6 years ago
Whiteboard: [land-in-aurora]
(Reporter)

Comment 15

6 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/9ce9e6614f92
status-firefox20: --- → fixed
Whiteboard: [land-in-aurora]

Updated

6 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.