Closed
Bug 823916
Opened 11 years ago
Closed 11 years ago
[toolbox] don't use ordinal
Categories
(DevTools :: Framework, defect, P2)
Tracking
(firefox20 fixed)
RESOLVED
FIXED
Firefox 21
Tracking | Status | |
---|---|---|
firefox20 | --- | fixed |
People
(Reporter: paul, Assigned: jwalker)
References
Details
Attachments
(1 file, 2 obsolete files)
15.35 KB,
patch
|
paul
:
review+
bajaj
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Updated•11 years ago
|
Component: Developer Tools → Developer Tools: Framework
Reporter | ||
Updated•11 years ago
|
Priority: -- → P2
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → jwalker
Assignee | ||
Comment 1•11 years ago
|
||
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)
Assignee | ||
Comment 2•11 years ago
|
||
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•11 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•11 years ago
|
||
(just saw your comment about addons, let me think about it)
Reporter | ||
Comment 5•11 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()?
Assignee | ||
Comment 6•11 years ago
|
||
(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•11 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.
Assignee | ||
Comment 8•11 years ago
|
||
Attachment #699690 -
Attachment is obsolete: true
Attachment #700356 -
Flags: review?(paul)
Reporter | ||
Comment 9•11 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+
Assignee | ||
Comment 10•11 years ago
|
||
fwiw: https://tbpl.mozilla.org/?tree=Try&rev=22d48402e458
Assignee | ||
Comment 11•11 years ago
|
||
(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].
Assignee | ||
Comment 12•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Fx-Team&rev=fd6d84e5bf49 https://hg.mozilla.org/integration/fx-team/rev/fd6d84e5bf49
Whiteboard: [fixed-in-fx-team]
Comment 13•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fd6d84e5bf49
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 21
Reporter | ||
Comment 14•11 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?
Updated•11 years ago
|
Attachment #700356 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Reporter | ||
Updated•11 years ago
|
Whiteboard: [land-in-aurora]
Reporter | ||
Comment 15•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/9ce9e6614f92
status-firefox20:
--- → fixed
Whiteboard: [land-in-aurora]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•