256246 - Customize Toolbars palette items positioned sloppily

Status: RESOLVED DUPLICATE of bug 419231

(Firefox :: Toolbars and Customization, defect)

Description

[Jesse Ruderman]

The positioning of items within the Customize Toolbars palette looks sloppy.

Specific problems:
* Items in the palette aren't lined up vertically.
* Excessive space between palette items in some rows (but not all rows).
* Dragging one item out of the palette makes other items move around, but often
only slightly.

Suggestions for fixing:
* Instead of using flex and a 4-items-per-row limit, use a block and
inline-block layout.  (Is this possible in XUL?)
* Give palette items a minimum width, so when two rows consist entirely of
normal buttons, items in those rows will be lined up vertically.

Comment 1

[Martin Pain]

inline-blocks don't seem to exist, unfortunately. They would be a very good
idea, but none of mozilla, microsoft, netscape OR the W3C seem to have thought
of them yet. (or, at least, have dismissed them if they have.) The advantage of
doing it in the current way, over a grid or similar, is that larger items can
fit without making the whole column too wide. The idea of a minimum width sounds
good, to make it fit for most rows, except where it needs a larger space. I'd
suggest that the minimum width would be the best way to make it better. 

Comment 2

[Martin Pain]

Attachment: Replacement customizeToolbar.js to improve look of customize toolbar dialog

I have attached a replacement customizeToolbar.js file
(resource:///chrome/toolkit.jar!content/global/customizeToolbar.js) which
improves the look of the customize toolbar dialog. It puts the toolbar items
into the box, and starts a new row when the current row's contents get wider
than the row. I would suggest that these changes be put into a future release
of firefox, if other people like their effect. (I think this is too small &
trivial to be put in an extension, although that may be useful for previewing
them.)

I have tested it on firefox 1.0.6 on windows 98, and have found no problems.

These are the changes I have made: (lines with [REMOVE] at the start have been
removed, lines with [ADD] have been added. Lines with no note in square
brackets have been left, and are shown here to indicate where in the file the
changes have been made. These notes are not in the attached file.)

[REMOVE] const kRowMax = 4;
const kWindowWidth = 635;
const kWindowHeight = 400;
const kAnimateIncrement = 50;
const kAnimateSteps = kWindowHeight / kAnimateIncrement - 1;
const kVSizeSlop = 5;


function buildPalette()
{
  // Empty the palette first.
  var paletteBox = document.getElementById("palette-box");
  while (paletteBox.lastChild)
    paletteBox.removeChild(paletteBox.lastChild);

  var currentRow =
document.createElementNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",

					    "hbox");
  currentRow.setAttribute("class", "paletteRow");
[ADD]  paletteBox.appendChild(currentRow);


  // Add the toolbar spacer item.
  templateNode =
document.createElementNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",

					      "toolbarspacer");
  templateNode.id = "spacer";
  templateNode.flex = 1;
  wrapPaletteItem(templateNode, currentRow, null);

[REMOVE]  var rowSlot = 3;

  var currentItems = getCurrentItemIds();
  templateNode = gToolbox.palette.firstChild;
  while (templateNode) {
    // Check if the item is already in a toolbar before adding it to the
palette.
    if (!(templateNode.id in currentItems)) {
      var paletteItem = templateNode.cloneNode(true);

[REMOVE]      if (rowSlot == kRowMax && false) {
[ADD]	   if (currentRow.lastChild.boxObject.x +
currentRow.lastChild.boxObject.width > currentRow.boxObject.width) {
[ADD]	     oldRow = currentRow;
[REMOVE]	// Append the old row.
[REMOVE]	paletteBox.appendChild(currentRow);

	// Make a new row.
	currentRow =
document.createElementNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",

					      "hbox");
	currentRow.setAttribute("class", "paletteRow");
[REMOVE]	rowSlot = 0;
[ADD]	     // Append the new row.
[ADD]	     paletteBox.appendChild(currentRow);

[ADD]	     //Move last child from previous row
[ADD]	     currentRow.appendChild(oldRow.lastChild);
      }

[REMOVE]      ++rowSlot;
      wrapPaletteItem(paletteItem, currentRow, null);
    }
    
    templateNode = templateNode.nextSibling;
  }

  if (currentRow) { 
    fillRowWithFlex(currentRow);
    paletteBox.appendChild(currentRow);
  }
}


function fillRowWithFlex(aRow)
{
[REMOVE]  var remainingFlex = kRowMax - aRow.childNodes.length;
[ADD]  var remainingFlex = 1;
  if (remainingFlex > 0) {
    var spacer =
document.createElementNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",

					  "spacer");
    spacer.setAttribute("flex", remainingFlex);
    aRow.appendChild(spacer);
  }
}

Comment 3

[Jesse Ruderman]

Attachment: cvs diff -u8 of previous attachment against customizeToolbar.js

There are some hunks here that aren't in the previous comment.	I don't know
whether that's because Martin missed some changes when he commented, or whether
these changes were unintentional.

Martin: please submit your changes as patches created using diff -u8, not by
attaching your changed files.  Also, you should work from version in cvs rather
files in nightly jars, which have already been through a preprocessor.

Comment 4

[Jesse Ruderman]

> I don't know whether that's because Martin missed some changes when he 
> commented, or whether these changes were unintentional.

It's also possible that he started with a different version of customizeToolbar.js.

Comment 5

[Martin Pain]

I started from the version of the file from the installation on my computer.
I've now read up on some of the documentation about patching (should have done
that earlier - oops), and am now downloading cygwin, & will work from the source
in future.

Comment 6

[Jesse Ruderman]

Firefox hangs when I load the attachment in comment 2.  I filed bug 306502 for that.

Comment 7

[nrlz]

I don't know if it's true for other people, but at least for me the disorganized look of the icons kind of facilitates the "shelf" metaphor. If things were too aligned, would it lessen its manipulability affordance? If there is going to be any form of organization, how about an alternating bricks alignment (like on a brick wall)?

Comment 8

[Dave Townsend [:mossop]]

I've been looking at this over the weekend and knocked together a few ideas but it is unclear what is the best way forward. I've put together some screenshots of ideas including the current state and the result of the patch in this bug. Rather than spam more than necessary the images and some comments are all available at:

http://users.blueprintit.co.uk/~dave/filestore/customise/

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Thanks for breaking that down, Dave. My feeling is that the third or fourth option of the ones you list above is the best way to go; right now, Option 3 (fixed width cols) looks the best, and I'm not sure that the long-label thing will be a huge problem; Option 4 has potential, but I think the default column width should be a little wider than it currently is.

Comment 10

[Dave Townsend [:mossop]]

Attachment: Example layout

This is a screenshot of a more working version of the fourth layout from my previous comment. There are a couple of changes to how the dialog works that are necessary to make this layout work:

Firstly in order to stop the rows looking like a ragged mess with lots of gaps on the right a simple algorithm is used to distribute the icons as best as possible. The problem with this is that when the dialog is resized the icons have to be laid out again, and in some cases some of the icons can jump from somewhere close to the top down to the bottom. This can be unexpected to the user but Im not sure its a big problem since I don't think users will be trying to keep track of where things are while they are resizing.

Secondly when an icon is dragged from a toolbar to the dialog, it is no longer necessarily added the the end. Instead the icon is added into the first row that has available space.

Thirdly When an icon is dragged from the dialog to a toolbar we have a problem. My current work has nothing happen, the row just has a gap in it. If the row is them empty then it is removed. One possible alternative to this would be to look for icon(s) in a following row that fits the gap and move those up, then of course you would have to do the same for the row(s) they came from etc. I think that is going to be somewhat strange though. You could have an icon 5 rows down suddenly jump up.

Comment 11

[Martin Pain]

If when an icon is draged to the toolbar it just leaves a gap in the pallette, I don't think that would be a very big problem at all. If we follow the "shelf" metaphor, then a gap would be left if it were a real shelf.
I tend to program with the idea of programs doing EXACTLY what would be expected of them. Think about it - which method you've mentioned would appear more 'natural' to end-users (taking non-techies into account)? To be honest, I think looking organised feels natural, rather than the current look, but i'm not sure which way would be best. Possibly listed them all based on size, smallest first,  with the same column approach as #4 from the examples by Dave Townsend.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #10)
> possible. The problem with this is that when the dialog is resized the icons
> have to be laid out again, and in some cases some of the icons can jump from
> somewhere close to the top down to the bottom. This can be unexpected to the

Yeah, that's not good at all. I could see the icons moving from one place to another between instances of the window being open, but once the window is open, the order should remain constant. If that can't be done with a flexible layout algorithm, then maybe the third solution is indeed the best (especially since I don't think the long-label thing will ever become a problem).

The second and third points you raised made me wonder something: why are we removing items from the customize window when they get dropped onto the toolbar?   A lot of the dynamic layout problems go away if we make it so that the layout doesn't need to be dynamic, but rather needs to be done a single time when the window is first opened. In the case where a user drags a button onto the toolbar that already exists, that can just get represented as a request to move the button (unless it's a button that can be duplicated).

Comment 13

[Dave Townsend [:mossop]]

(In reply to comment #12)
> The second and third points you raised made me wonder something: why are we
> removing items from the customize window when they get dropped onto the
> toolbar?   A lot of the dynamic layout problems go away if we make it so that
> the layout doesn't need to be dynamic, but rather needs to be done a single
> time when the window is first opened. In the case where a user drags a button
> onto the toolbar that already exists, that can just get represented as a
> request to move the button (unless it's a button that can be duplicated).
> 

I agree this basically makes the layout problem go away (though you'll still get changes when new toolbar items are added by extensions of course). However I'm not sure how usable this is. I would expect there would have to be some fairly obvious indication that an item already exists on a toolbar and that you aren't going to get a second one.

Maybe even the item should just be undraggable from the dialog and rendered in some kind of disabled state in the dialog to make it clear it's gone already and undraggable. Though I guess that's still going to confuse though in a different way.

Comment 14

[pile0nades]

What about something like this: http://userstyles.org/style/show/547 ? It is a userstyle hack that is like the attached patch, but allows for any number of columns depending on the window size. Also has a minimum width, keeping most things aligned.

Comment 15

[Dave Townsend [:mossop]]

(In reply to comment #14)
> What about something like this: http://userstyles.org/style/show/547 ? It is a
> userstyle hack that is like the attached patch, but allows for any number of
> columns depending on the window size. Also has a minimum width, keeping most
> things aligned.
> 

That also enforces a fixed size on each toolbar item, including the url bar etc.