292496 - preferences window grows to double of its normal height in special circumstances

Status: RESOLVED FIXED

(Toolkit :: Preferences, defect)

Description

[Steffen Wilberg]

In bug 284081 I made the prefwindow open and select the Content pane when having
software install (xpinstall.enabled) disabled, clicking on an xpi so that the
info bar appears, and clicking on the "Edit Options" button.

There's a bug when browser.preferences.animateFadeIn is set to true (which is
the default on Mac): When the last pane of the prefwindow was something other
than Content, and the prefwindow is not already open when you click the "Edit
Options" button on the info bar, the prefwindow opens, shows the Content panel,
and sizes to about double of the normal height.

Steps to reproduce:
1. Set xpinstall.enabled to false and browser.preferences.animateFadeIn to true.
2. Open the prefwindow, select a pane other than Content, and close it.
3. Click on a xpi link, like on http://www.hacksrus.com/~ginda/chatzilla/.
4. The "software installation is disabled" info bar appears. Click the "Edit
Options" button.

Actual result:
The prefwindow opens and grows much too large, about double of its normal height.

Expected result:
The prefwindow shows up and takes its normal size.

Comment 1

[Steffen Wilberg]

Attachment: screenshot

Comment 2

[Steffen Wilberg]

Attachment: patch

The first hunk is to make the code I added in bug 284081 more robust, to
prevent bugs like bug 292411 from happening in the future.

The second hunk is to fix this bug. The problem is that I don't understand
what's going on. So this is merely a starting point.

Comment 3

[Nickolay_Ponomarev]

The problem is that the code in constructor sets paneToLoad to 'content', but
lastSelected is still, say, 'general'. So constructor calls showPane('content'),
which calls _selectPane('content'), which decides it wants to animate the
transition and calls animate('general', 'content').

There we get oldHeight=generalPane.contentHeight=0, as it isn't yet loaded, and
sizeDelta=contentPane.contentHeight > 0.

The animate timer is set up, it increments (not sets the absolute value of)
window.innerHeight, but at the moment it starts running the window is already
shown and innerHeight is contentPane.contentHeight + selector height + buttons
height.

The fix to this bug is to set lastSelected in constructor, or, alternatively, to
fix everyone else who uses the lastSelected, but the whole system seems to me
quite fragile, and it would imho be better to also calculate the total height of
selector+buttons+whatever and set window.innerHeight to sum of that and
_currentHeight. That way we will never get too high windows. If you're not
convinced, I can file a separate bug with more motivation (and at least one
other bug current implementation causes).

Comment 4

[Steffen Wilberg]

Attachment: set lastSelected

Thanks for the analysis. This sets lastSelected in the constructor, as
suggested. It's at least an easy fix for this bug.

Comment 5

[Mike Connor [:mconnor]]

Comment on attachment 186248 [details] [diff] [review]
set lastSelected

Gah, this is a really old request.  r=me, since this seems to fix a case that could be pain for some callers (even though the original bug doesn't exist)

if this is worth taking on the 1.8.1 branch, please ask me for branch approval.

Comment 6

[Steffen Wilberg]

Steps to reproduce:
1. In about:config, set browser.preferences.animateFadeIn to true.
2. Open the prefwindow, select a pane other than Content, and close it.
3. In the error console, evaluate: top.window.opener.openPreferences("paneContent");

Comment 7

[Steffen Wilberg]

Checked in.

Comment 8

[Steffen Wilberg]

Comment on attachment 186248 [details] [diff] [review]
set lastSelected

This bug isn't visible at the moment since nobody opens the prefwindow with a specific pane anymore. But that might change again. Besides,  extensions might add a pane to the prefwindow and open that.

Comment 9

[Steffen Wilberg]

Checked into branch.