124241 - Remove all fixed size attributes from XUL

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[Kai Engert [:KaiE:]]

In several of our XUL files, we use a hack, to make dialogs resize correctly,
when we are using dynamic text attributes (set in onload handlers).

I wrote a test patch with a minimal XUL window, where I do something similar. If
my window definition contains something like WIDTH="400", that hack is necessary
(look at escrowWarn.js and the comment THIS IS NEEDED for an example). The hack
was suggested to us in bug 76248.

However, I found out that this is hack is not needed if we simply stop using
fixed sizes in dialogs. As soon as I remove that WIDTH="400" from the window
definition, everything works immediately. Not even a call to
window.sizeToContent() is required. The OK/Cancel buttons are still visible,
even if the dynamic text is long.

If we need to define sizes for windows, because we want to define a certain
shape for a window, we should use stylesheet logic instead. So instead of using:
  WIDTH="400"
we should code similar to:
  style="width: 40em; max-width: 50em;"

The definition "em" is relative to the available font size, and therefore 40em
is a size recommendation that works on all platforms.

Comment 1

[Kai Engert [:KaiE:]]

Attachment: Suggested fix

I have tried this on Linux, and it looks good. Looking for somebody to test
this on Windows (or I will do soon).

Comment 2

[Stephane Saux]

We've had many weird size problems where platforms, font size, screen size
interact in weird ways to cause this problem.  I believe too that there are some
dialogs that pop up at "inconvenient" times (not sure what these are, maybe in
the middle of a handshake).

When Rangan worked on this problem, he mentioned a specific xul bug that we were
supposed to work around. Rangan can you dig out that bug number?

I'm all for removing workaround.  On the other hand we may on occasion want to
use a fixed size (maybe a minimum size?)

The bottom line is that testing on 2 machines has not in the past proven to be
sufficient.  Maybe John Unruh can also chime in.

Comment 3

[Kai Engert [:KaiE:]]

> We've had many weird size problems where platforms, font size, screen size
> interact in weird ways to cause this problem.  I believe too that there are some
> dialogs that pop up at "inconvenient" times (not sure what these are, maybe in
> the middle of a handshake).

I would be really not surprised, if all problems came from pixel based dialog
sizes, which the XUL engine does not seem to be able to deal with (and which one
shouldn't use anyway).


> When Rangan worked on this problem, he mentioned a specific xul bug that we were
> supposed to work around. Rangan can you dig out that bug number?

Rangan gave me that bug number I mentioned in my first comment.


> I'm all for removing workaround.  On the other hand we may on occasion want to
> use a fixed size (maybe a minimum size?)

You can specify minimum sizes using style sheet rules, similar as I'm doing it
in the patch.


> The bottom line is that testing on 2 machines has not in the past proven to be
> sufficient.  Maybe John Unruh can also chime in.

Sure, testing on many machines is a good idea, but considered, that nobody on
#mozilla seems to remember that problem, and everybody told us to not use fixed
sized dialogs, we should definitively test it.

Comment 4

[Stephane Saux]

works for me then.

Comment 5

[Rangan Sen]

Kaie: There another bug which describes the original xul bug [not the bug
describing the hack] its bug# 91137 ... not sure if I had mentioned that ...
Actually, even with style sheet format, there used to be a sizing problem with
large fonts on NT... I'm not sure if that still holds true...but bug# 91137 is
still open ...

Comment 6

[Kai Engert [:KaiE:]]

> Kaie: There another bug which describes the original xul bug [not the bug
> describing the hack] its bug# 91137 ... not sure if I had mentioned that ...
> Actually, even with style sheet format, there used to be a sizing problem with
> large fonts on NT... I'm not sure if that still holds true...but bug# 91137 is
> still open ...

Rangan, thanks for the hint, I added a comment to bug 91137, but nobody
responded yet. People in that bug do not seem to understand what's happening, or
we haven't yet found those who could know.

We should test whether my patch works with large fonts on NT, if that works, I
think we are in good shape.

I want to note I've been seeing some ugly looking dialogs on Linux, for example
the dialog to prompt for a p12 file password. There was a large blank space in
the lower area of the dialog. However, with that new patch it looks correct.

Should we try to check this patch in, and if we get complaints, just revert the
patch? That might be the easiest way to test whether this patch works, given
that we are currently not close to a release.

Comment 7

[Rangan Sen]

Kaie: I think that would be a good idea. However, in my openion, it might be
better to test it on one dialog first - idealy on a modal dialog that comes up
during a hanshake [the download ca cert dialog would be a good candidate],
because apparently, on in such cases, resize does not work ok [I dodn't fully
understand why].

Comment 8

[Kai Engert [:KaiE:]]

Rangan, on my Linux system dialogs opened during a SSL transaction look ok. I
tested the download CA and the "domain mismatch" dialogs.

Comment 9

[Rangan Sen]

I think it would be a good idea to land these two - and if they are good on
other platforms, the change can be propagated to all other dialogs that use the
hack - what's your openion? 

Comment 10

[Kai Engert [:KaiE:]]

You suggest to land only a portion now, and if we don't hear negative feedback,
land the other portions later. This is a safety approach, but I'm not sure
whether we need to play safe. Stephane, should we?

I see a disadvantage in landing only a subset of the patch. If we change only
seldom seen dialogs, we risk that the effect is not detected by testers soon.
The more dialogs we change, the more likely is error feedback. In addition,
splitting the patch into two separate landings is a bit more work.

Comment 11

[Stephane Saux]

Kai. I'm ok with checking in everything.

Rangan has a point, and there's a risk, but we can roll back any change that
breaks dialogs.

As long as we're aware of this bug, if we see any bug appearing that mention
wrong dialog size, we'll know where to look. Since we don't know of any reason
why not doing the right thing may not work, we should assume it will.

One question though.  I believe your patch sizes the dialogs strictly on the
basis of content (sizeToConten()) That may be sufficient, but I'm afraid that
there are dialog whose size we may not want to go under a certain size. How do
we handle that case?

Comment 12

[Kai Engert [:KaiE:]]

> One question though.  I believe your patch sizes the dialogs strictly on the
> basis of content (sizeToConten()) That may be sufficient, but I'm afraid that
> there are dialog whose size we may not want to go under a certain size. How do
> we handle that case?

Not sure what you mean, do you say: "Although the automatical resize of the XUL
engine makes sure, that all content is fully visible, this is not enough. We
want some dialogs to be larger, even if the extra space is not required." ?

I only can imagine one scenario where the above makes sense: If the dialog
contains edit fields or lists, that are initially empty, but should have a
minimum size.

In that case, we could use
  style="min-width: 10em; min-height: 10em;"
as required. (replace 10 as needed)
However, in the patch I already replaced all existing size recommendations with
similar ones based on styles, so I'd guess we already have good sizes.

If we decide we want more space, it's easy to change the style attributes later.

Comment 13

[Kai Engert [:KaiE:]]

Rangan, can you please review?

Comment 14

[Rangan Sen]

Comment on attachment 68383 [details] [diff] [review]
Suggested fix

r=rangansen

Comment 15

[Joe Hewitt (gone)]

Comment on attachment 68383 [details] [diff] [review]
Suggested fix

sr=hewitt

Comment 16

[Kai Engert [:KaiE:]]

checked in, fixed.

Comment 17

[John Unruh]

Verified fixed. Will open new bugs if any XUL windows were forgotten.

Comment 18

[Mox Soini]

Just adding a comment here since this bug changes takes the hack away that is
discussed in bug 94384 ...

I've tested on builds around 2.-9. of April 2002 with both windows (small and
large font sets) and linux and they work fine :)
Also works when adding the following to userChrome.css (was a testcase)

dialog, button {
  font-size: 5.0mm !important;
  font-family: helvetica !important;
}

... So everythings great ! Just take care of the bugzilla's bugs that are
related to this (discussed further in bug 91137 )