68352 - height of commonDialog.xul does not account for html content line-wrapping

Status: RESOLVED FIXED

(Core :: XUL, defect, P3)

Description

[Dimitrios]

When clicking on a downloadable item at
http://sherlock.mozdev.org/download.html, the confirmating dialog box is
truncated on it's bottom side. A known javascript bug ?

Comment 1

[Dimitrios]

Attachment: a screenshot

Comment 2

[Phil Schwartau]

Browser, not engine. Reassigning to XP Toolkit/Widgets for further triage - 

Comment 3

[John Morrison]

I think that what is happening is that when the computed height of the dialog 
(for commonDialog.xul) is made, it is not accounting for some line-wrapping 
that the html content will do. So the height of the dialog is set, but due to 
the length of the first line (which becomes two lines), there is one 
line-height less than required. The buttons then gets pushed down to the bottom 
margin of the dialog. 

Comment 4

[Peter Trudelle]

shouldn't that be handled by intrinsic sizing?  ->evaughan

Comment 5

[Peter Trudelle]

->danm, cc hyatt

Comment 6

[Peter Trudelle]

->moz0.9.1

Comment 7

[timeless]

*** Bug 72408 has been marked as a duplicate of this bug. ***

Comment 8

[Peter Trudelle]

DanM says this is the same root cause as 76350, resolving as dup.


*** This bug has been marked as a duplicate of 76350 ***

Comment 9

[Peter Trudelle]

or, may be related to bug 77639

Comment 10

[Dimitrios]

Does it make sense to mark a bug as a duplicate of a more recent one? Yes, if
the newer bug has a more general yet accurate summary / description (it happend
in a bug I reported a few months ago and I didn't complain). But this time this
is not the case and, to me, it is a bad practice. Changing OS to all and
suggesting re-opening the bug or marking it fixed. If you mark it fixed or
whatsoever, that would also mean that the Serlock download dialog box problem is
also fixed (and this is definitely not related to bug 76350 or bug 77639).
Feel free to whatever you want to this bug but first please give me an
explanation. I didn't take it personally and I never felt nothing important to
the project but I prefer to see decent quality procedures beeing maintained.

Comment 11

[Dimitrios]

After Peter Trudelle's suggestion (the Sherlock dialog problem continues to
exist in build 2001050420), I re-open the bug. Peter, thank you for your spirit
of understanding.

Comment 12

[Dan M]

This is that other bug, the one where wrapped (html) text decides to rewrap 
itself after the window size has been determined. The following hack seems to 
"fix" the problem, in that windows seem to no longer be clipped on the bottom. I 
think this is really a layout problem and this bug wants an owner in the layout 
group. But for now this hack might suffice:

Index: nsDocumentViewer.cpp
===================================================================
RCS file: /cvsroot/mozilla/content/base/src/nsDocumentViewer.cpp,v
retrieving revision 1.119
diff -u -r1.119 nsDocumentViewer.cpp
--- nsDocumentViewer.cpp	2001/05/08 16:44:07	1.119
+++ nsDocumentViewer.cpp	2001/05/10 01:39:35
@@ -4976,6 +4976,13 @@
    presContext->GetTwipsToPixels(&pixelScale);
    width = PRInt32((float)shellArea.width*pixelScale);
    height = PRInt32((float)shellArea.height*pixelScale);
+   /* resize once more. this seems to serve as a reminder to layout that
+      when the window is sized as we just determined, not to change its
+      mind about wrapping certain content, like html text. sounds like
+      a bug in layout. but given that, this hack for now seems to
+      reliably keep intrinsically sized windows containing wrapped text
+      from being clipped on the bottom. */
+   presShell->ResizeReflow(NS_UNCONSTRAINEDSIZE, NS_UNCONSTRAINEDSIZE);
 
    nsCOMPtr<nsIDocShellTreeOwner> treeOwner;
    docShellAsItem->GetTreeOwner(getter_AddRefs(treeOwner));

Comment 13

[Dan M]

  Instead of the above hack, what might be a general solution:
  This bug is caused by a rounding error. Frame size calculations are carried out 
in twips. The window is sized in pixels. For example, a different (more easily 
reached) window I've been testing with on my machine gets a calculated width of 
9800 twips, which is 653.3 pixels. The integer pixel conversion rounds to the 
nearest int, and the window is consequently sized to 653 pixels.
  Every error of this type that I've seen looks the same: the size calculation 
returns a glove-wrapped fit. By the time the window is shown, layout decides to 
wrap the last word on the longest line of wrappable text. This is no doubt caused 
by the twip-to-pixel truncation making the window ever so barely more narrow.
  I'm afraid of the repercussions of teaching NSTwipsToIntPixels to take the 
ceiling rather than rounding. Seems like the best fix is to just assume a pixel 
was lost and add it back during intrinsic window sizing:

Index: content/base/src/nsDocumentViewer.cpp
===================================================================
RCS file: /cvsroot/mozilla/content/base/src/nsDocumentViewer.cpp,v
retrieving revision 1.119
diff -u -r1.119 nsDocumentViewer.cpp
--- nsDocumentViewer.cpp	2001/05/08 16:44:07	1.119
+++ nsDocumentViewer.cpp	2001/05/10 23:16:22
@@ -4981,7 +4981,16 @@
    docShellAsItem->GetTreeOwner(getter_AddRefs(treeOwner));
    NS_ENSURE_TRUE(treeOwner, NS_ERROR_FAILURE);
 
-   NS_ENSURE_SUCCESS(treeOwner->SizeShellTo(docShellAsItem, width, height),
+   /* presContext's size was calculated in twips and has already been
+      rounded to the equivalent pixels (so the width/height calculation
+      we just performed was probably exact, though it was based on
+      values already rounded during ResizeReflow). In a surprising
+      number of instances, this rounding makes a window which for want
+      of one extra pixel's width ends up wrapping the longest line of
+      text during actual window layout. This makes the window too short,
+      generally clipping the OK/Cancel buttons. Here we add one pixel
+      to the calculated width, to circumvent this problem. */
+   NS_ENSURE_SUCCESS(treeOwner->SizeShellTo(docShellAsItem, width+1, height),
       NS_ERROR_FAILURE);
 
    return NS_OK;

There's still a problem that the size reported by intrinsic sizing uses no text 
wrapping. I think we should make some attempt to give the window a golden ratio 
size. But that would be a different problem (and dependent on bug 72152, which 
causes any attempt to recalculate an intrinsic window's size to fail). At least 
this patch will prevent the window's buttons' truncation.

Comment 14

[Peter Trudelle]

->evaughan/0.9.2

Comment 15

[Dan M]

no

Comment 16

[Dan M]

the adjustment for rounded width is checked in