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

RESOLVED FIXED in mozilla1.8beta3

Status

()

RESOLVED FIXED
14 years ago
12 years ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

unspecified
mozilla1.8beta3
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 obsolete attachments)

(Assignee)

Description

14 years ago
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.
(Assignee)

Comment 1

14 years ago
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];
(Assignee)

Comment 2

14 years ago
Created attachment 180393 [details] [diff] [review]
Fixes the problem

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

Comment 3

14 years ago
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.
OS: Linux → All
Hardware: PC → All
Version: unspecified → Trunk
(Assignee)

Updated

14 years ago
Flags: blocking-aviary1.1?
(Assignee)

Comment 4

14 years ago
Created attachment 180398 [details] [diff] [review]
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.
Attachment #180393 - Attachment is obsolete: true
(Assignee)

Comment 5

14 years ago
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.
Attachment #180398 - Flags: review?(mconnor)

Updated

14 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

14 years ago
Attachment #180398 - Flags: review?(mconnor)
(Assignee)

Comment 6

14 years ago
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.
Attachment #180398 - Attachment is obsolete: true
(Assignee)

Comment 7

14 years ago
Created attachment 182188 [details] [diff] [review]
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.
Attachment #182188 - Flags: review?(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 :)
Attachment #182188 - Flags: review?(mconnor) → review-
(Assignee)

Comment 9

14 years ago
Created attachment 183941 [details] [diff] [review]
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
Attachment #182188 - Attachment is obsolete: true
Attachment #183941 - Flags: review?(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.
Attachment #183941 - Flags: review?(mconnor) → review+
(Assignee)

Comment 11

14 years ago
Created attachment 184937 [details] [diff] [review]
Final fix with mconnor's review

Last change made. Looking for approval for 1.1a2
Attachment #183941 - Attachment is obsolete: true
Attachment #184937 - Flags: approval-aviary1.1a2?
Comment on attachment 184937 [details] [diff] [review]
Final fix with mconnor's review

marking r+ for mconnor, and a=shaver.
Attachment #184937 - Flags: review+
Attachment #184937 - Flags: approval-aviary1.1a2?
Attachment #184937 - Flags: approval-aviary1.1a2+

Comment 13

14 years ago
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.
Attachment #184937 - Attachment is obsolete: true
Attachment #184937 - Flags: review-
Attachment #184937 - Flags: review+
Attachment #184937 - Flags: approval-aviary1.1a2+

Updated

14 years ago
Assignee: bugs → dave.townsend
(Assignee)

Comment 14

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

Bringing back earlier patch, requesting review.
Attachment #183941 - Attachment is obsolete: false
Attachment #183941 - Flags: approval-aviary1.1a2?
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.
(Assignee)

Comment 16

14 years ago
Created attachment 185034 [details] [diff] [review]
Hopefully the last patch
Attachment #183941 - Attachment is obsolete: true
Attachment #185034 - Flags: review?(mconnor)
Attachment #185034 - Flags: approval-aviary1.1a2?

Updated

14 years ago
Attachment #185034 - Flags: review?(mconnor) → review+
Comment on attachment 183941 [details] [diff] [review]
Fix addressing mconnor's comments

a=shaver
Attachment #183941 - Flags: approval-aviary1.1a2? → approval-aviary1.1a2+
Comment on attachment 185034 [details] [diff] [review]
Hopefully the last patch

a=shaver
Attachment #185034 - Flags: approval-aviary1.1a2? → approval-aviary1.1a2+
*** Bug 296434 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 20

14 years ago
I can't checkin so if someone could put this in that would be great.
(Assignee)

Updated

14 years ago
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
(Assignee)

Comment 21

14 years ago
Updated product to help people searching later
Component: Preferences → Preferences
Product: Firefox → Toolkit
Target Milestone: --- → mozilla1.8beta3
Version: Trunk → unspecified

Comment 22

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

mozilla/toolkit/content/widgets/preferences.xml 	1.16
Attachment #185034 - Attachment is obsolete: true
Attachment #185034 - Flags: review+

Updated

14 years ago
Flags: blocking-aviary1.1?

Updated

12 years ago
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.