Created attachment 144498 [details] [review]
[docDisplay] Fix minimum height request, handle 0 allocation better

Review of attachment 144498 [details] [review]:

Hmm, patch to getPreferredHeight looks fine, but that function has other issues:

        // Two columns, where we go vertically down first.  So just take
        // the height of half of the children as our preferred height.

        let firstColumnChildren = children.length / 2;

Shouldn't that be:

 Math.ceil(children.length / 2);

        alloc.min_size = 0;

It's bizarre to set min_size = 0 when you are counting on alloc.natural_size being initialized to zero. Either trust or don't trust. Not half-way.

::: js/ui/docDisplay.js
@@ +396,2 @@
                     break;
                 }

So, what if you had two items:

 First item fits vertically
 Second item taller by itself than the total vertical space.

Then this code would put the second item into the second column without checking. The right fix is different from this - it's something like:

                columnIndex += 1;
                x = x + itemWidth + DEFAULT_SPACING;
        	// And y is back to the top.
                y = box.y1;
+               i -= 1; /* retry this item in the second column */
+               continue;                
            }

(or better, make the for into a while loop and move the i++ to the end after you succesfully allocate a child to avoid the i -= 1 hack)

Then that will fix your bug and the other case I mentioned as well.

Bug 596984 - [docDisplay] Fix minimum height request, handle 0 allocation better - UNCONFIRMED