285513 - Selected options pane fails to persist if the last selected pane no longer exists

Status: RESOLVED FIXED

(Toolkit :: Preferences, defect)

Description

[Dave Townsend [:mossop]]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2) Gecko/20050309 Firefox/1.0+
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2) Gecko/20050309 Firefox/1.0+

If the last selected options pane no longer exists then when options is
displayed it shows a blank pane, and will do every time it is opened until the
entry in localstore.rdf is changed.

Reproducible: Always

Steps to Reproduce:
1. Install an extension that adds a new pane to the options dialog (e.g.
JavaScript Options <=0.7).
2. Open the options and select the added pane. Close options with this pane
selected.
3. Uninstall the extension and restart Firefox.
4. Open the options.

Actual Results:  
A blank options window shows. Selecting one of the panes displays it just fine,
but after closing and re-opening options a blank pane is shown once again.

Expected Results:  
Spotted that the last selected pane no longer exists and used one of the others
instead.

Comment 1

[Dave Townsend [:mossop]]

There is a fairly simple fix that should probably suffice. Sorry I don't know
how to make patches, but in toolkit/content/widgets/preferences.xml, the line
(currently 460):

var lastPane = this.lastSelected ? document.getElementById(this.lastSelected) :
panes[0];

Should be followed with:

if (!lastPane)
    lastPane=panes[0];

Comment 2

[Dave Townsend [:mossop]]

Attachment: Fixes the problem

Tested this patch on build and it works. I can't imagine there being any
problems caused by it.

Comment 3

[Dave Townsend [:mossop]]

This occurs on all platforms on the trunk. This will cause problems for people
uninstalling some extensions in 1.1 so I think it should be blocking.

Comment 4

[Dave Townsend [:mossop]]

Attachment: Properly fixes the problem

My bad. The last patch only made a valid pane display when an invalid pane was
persisted, it did not fix the persistence problem itself. This is a proper fix.

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 180398 [details] [diff] [review]
Properly fixes the problem

Requesting review. I'm new to this game, hopefully this is the right process.

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 180398 [details] [diff] [review]
Properly fixes the problem

The patch for Bug 284081 may have fixed this. Regardless it makes this patch
unusable so I will revisit this when I get a chance.

Comment 7

[Dave Townsend [:mossop]]

Attachment: Detects the problem and corrects

When trying to choose the first pane to display we try to retrieve the last
displayed pane. If it doesnt exist then clear lastSelected since other code
relies on it, and choose the first pane. Then do the extra check for a window
argument.

Re-requesting review.

Comment 8

[Mike Connor [:mconnor]]

Comment on attachment 182188 [details] [diff] [review]
Detects the problem and corrects

>+        if (this.lastSelected) {
>+          paneToLoad = document.getElementById(this.lastSelected);
>+          if (!paneToLoad) {
>+            paneToLoad=panes[0];
>+            this.lastSelected=null;

nit, spaces around the = the two previous lines.

>+          }
>+        }
>+        else {
>+          paneToLoad=panes[0];
>+        }
>+

there's no sense setting it here if we're setting it below.  Drop the else
statement, and after the if block below, do this, since this ensures that
somehow we get a valid pane and we don't break (i.e. invalid pane arg).

	  if (!paneToLoad)
	    paneToLoad = panes[0];

>         if ("arguments" in window && window.arguments[0])
>           paneToLoad = document.getElementById(window.arguments[0]);

r=me once I see the changes :)

Comment 9

[Dave Townsend [:mossop]]

Attachment: Fix addressing mconnor's comments

This fix now basically does two things. The first part checks if lastSelected
is valid. If not it sets it to null which is the same state as when the options
dialog has never been opened before. This is necessary since most of the pane
changing code breaks if lastSelected is invalid.

The second part just checks that any pane passed as an argument is actually
valid before using it.

The lastSelected pane is held in a local variable between the two parts to save
a call to getDocumentElementById

Comment 10

[Mike Connor [:mconnor]]

Comment on attachment 183941 [details] [diff] [review]
Fix addressing mconnor's comments


>         var paneToLoad;
>-        if ("arguments" in window && window.arguments[0])
>+        if (("arguments" in window && window.arguments[0]) && (document.getElementById(window.arguments[0])))

r=me, with the above changed to this since its cleaner and has the same result:

	if ("arguments" in window &&
document.getElementById(window.arguments[0]))

sorry this got lost in the cycle.

Comment 11

[Dave Townsend [:mossop]]

Attachment: Final fix with mconnor's review

Last change made. Looking for approval for 1.1a2

Comment 12

[Mike Shaver (:shaver emeritus)]

Comment on attachment 184937 [details] [diff] [review]
Final fix with mconnor's review

marking r+ for mconnor, and a=shaver.

Comment 13

[timeless]

Comment on attachment 184937 [details] [diff] [review]
Final fix with mconnor's review

as i explained to mconnor, who apparently failed to act on this before shaver
got around to this bug in his queue, mconnor's advice was absolutely wrong.

you will get an assert if you pass '' to getElementById, your older patch was
better. i'm sorry.

Comment 14

[Dave Townsend [:mossop]]

Comment on attachment 183941 [details] [diff] [review]
Fix addressing mconnor's comments

Bringing back earlier patch, requesting review.

Comment 15

[Mike Connor [:mconnor]]

at least attach a patch without the extraneous brackets.

I'm waiting on review to remove the unnecessary assert here, but I suppose the
empty-check is ok.

Comment 16

[Dave Townsend [:mossop]]

Attachment: Hopefully the last patch

Comment 17

[Mike Shaver (:shaver emeritus)]

Comment on attachment 183941 [details] [diff] [review]
Fix addressing mconnor's comments

a=shaver

Comment 18

[Mike Shaver (:shaver emeritus)]

Comment on attachment 185034 [details] [diff] [review]
Hopefully the last patch

a=shaver

Comment 19

[:Gavin Sharp [email: [e-mail]]]

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

Comment 20

[Dave Townsend [:mossop]]

I can't checkin so if someone could put this in that would be great.

Comment 21

[Dave Townsend [:mossop]]

Updated product to help people searching later

Comment 22

[timeless]

Comment on attachment 185034 [details] [diff] [review]
Hopefully the last patch

mozilla/toolkit/content/widgets/preferences.xml 	1.16