Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a3pre) Gecko/20070310 Minefield/3.0a3pre When I open the Certificate Manager, the "Authorities" tab is selected but the content in the displayed panel is that of the "Your Certificates" tab. Switching tabs restores the window until it is closed and opened again. Steps to reproduce: 1. In Options/Preferences -> Advanced -> Encryption, click on the "View Certificates" button to open the Certificate Manager. Actual results: The "Authorities" tab is selected, but the installed client certificates are shown. Expected results: The "Your Certificates" tab is selected and the installed client certificates are shown. This regressed between the trunk builds from 2007-02-19-04 and 2007-02-20-05, so it is possibly related to bug 370742. http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-02-19+03%3A00%3A00&maxdate=2007-02-20+05%3A00%3A00&cvsroot=%2Fcvsroot

There is a attribute 'selected="true"' set for the CA tab in certManager.xul that has been there since the initial checkin, but as far as I could tell have never worked. I have tried builds as old as Mozilla 1.0 and they all open Cert manager with the client cert tab selected. Removing the 'selected="true"' attribute seems to fix this bug, but I don't know if that is the right fix. Should the focus be on the CA tab when Cert manager opens?

This is a bug in tabbox.xml because the selectedIndex setter returns early after checking '!tabs[val].selected' which will be true, yet the selected panel is not changed.

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a5pre) Gecko/2007042901 SeaMonkey/1.5a] (suiterunner, tinderbox-builds) (W2Ksp4) Same bug with suiterunner: first install, "Authorities" tab is selected and displayed blank (with new/default profile), switching tabs restores its content. Quite misleading !

(In reply to comment #2) > This is a bug in tabbox.xml because the selectedIndex setter returns early > after checking '!tabs[val].selected' which will be true, yet the selected > panel is not changed. Would a change like this be ok, then? That does the trick for me. If this fix seems acceptable, whom should I ask for review?

(In reply to comment #1) > Removing the 'selected="true"' attribute seems to fix this bug, but I don't > know if that is the right fix. Should the focus be on the CA tab when Cert > manager opens? I think that's ok when the Cert Manager is opened for the very first time (because the CA tab is probably the only one which is non-empty in this case). Afterwards, however, I would say it makes more sense to remember the last selected tab, which can be achieved by the attached patch (make selectedIndex persistent for this tabbox). Kai, do you agree?

Comment on attachment 267843 [details] [diff] [review] Separate improvement: make selected tab in Cert Manager persistent I agree with this idea and the patch. Thanks Kaspar. But just to repeat the obvious, this is separate from the regression bug.

Comment on attachment 267842 [details] [diff] [review] change selectedIndex setter for tabbox I tested this patch and it fixes the bug for me. (Please note, if you happen to run into a weird display and assertions while working with cert manager, please apply attachment 267909 [details] [diff] [review] from bug 383969.) Neil, can you please review, or should somebody else? Are additional reviews required? Thanks.

Adding dependency to bug 370724 which is suspected as having caused this regression.

Comment on attachment 267842 [details] [diff] [review] change selectedIndex setter for tabbox This patch will fire the select event even when setting selectedIndex to the same value.

(In reply to comment #9) > (From update of attachment 267842 [details] [diff] [review]) > This patch will fire the select event even when setting selectedIndex to the > same value. Do you know a better solution? Who could help? As this is a regression, we should either have a fix soon, or back out the original patch.

It should just not fire the events in that case, or implement a _suppressOnSelect feature which is set during the constructor around the selectedIndex call (listbox and some other elements have this for certain cases) But if you need the fix very soon, you can check this one in, and file a followup bug on improving it.

(In reply to comment #11) > It should just not fire the events in that case, or implement a > _suppressOnSelect feature which is set during the constructor around the > selectedIndex call (listbox and some other elements have this for certain > cases) Ok, so from what I understand... would this be the correct patch, then? This also fixes the bug (with Cert Manager), but only fires the event if necessary.

Comment on attachment 267972 [details] [diff] [review] change selectedIndex setter for tabbox, patch v2 No, you would need to check tabs[val].selected near the beginning of the function before it gets changed, store the state in a variable, then check it afterwards. Also, the tabpanels.selectedIndex setter fires an event. Is there a timeline on when this is needed for? I can take a better look later in the week if desired.

(In reply to comment #13) > (From update of attachment 267972 [details] [diff] [review]) > No, you would need to check tabs[val].selected near the beginning of the > function before it gets changed, store the state in a variable, then check it > afterwards. Well, seems like I should better leave this to someone who is actually familiar with tabbox.xml... ;-) But anyway, out of curiosity - am I missing something? As far as I can see, neither val nor tabs[val].selected get changed anywhere in this function (only tabs[val]._selected), so why would an extra variable be needed to store the state? > Also, the tabpanels.selectedIndex setter fires an event. Yes, but this is actually what really *fixes* the bug :-) If tabpanels.selectedIndex = val; doesn't get executed (and if the event is not fired), Cert Manager still opens with the wrong content for the "Authorities" tab... > Is there a timeline on when this is needed for? I can take a better look later > in the week if desired. It isn't *that* urgent, I guess, so your help later this week would certainly be welcome - thanks.

We should definitely fix this before alpha 6.

(In reply to comment #14) > > But anyway, out of curiosity - am I missing something? As far as I can see, > neither val nor tabs[val].selected get changed anywhere in this function (only > tabs[val]._selected), so why would an extra variable be needed to store the > state? Setting _selected modifies the selected attribute which .selected reads.

It's unfortunate this is still broken. Today is the checkin deadline alpha 6. Can we back the original patch out?

I discussed this with Neil. He convinced me the "<tab selected=true>" feature didn't work in the past. Therefore I'm ok to remove that attribute, at least temporarily until this bug is fixed. Patch coming up.

This patch removes the selected="true" attribute, which never worked in the past, and is not yet fully working.

Comment on attachment 270038 [details] [diff] [review] Patch v4, workaround [Backout: Comment 57] r+sr=shaver, though I'd prefer FIXME over XXX there.

Both patches checked in (workaround and improvement), with Mike's request added. Neil, where do you plan to work on improving the selected="true" feature? Maybe we should file a spearate bug and make it block this one? Thanks

(In reply to comment #21) > Maybe we should file a spearate bug and make it block this one? Yes

(In reply to comment #22) > (In reply to comment #21) > > Maybe we should file a spearate bug and make it block this one? > Yes done, filed 386202. We should re-enable the selected="true" when bug 386202 is fixed.

(In reply to comment #21) > Both patches checked in (workaround and improvement), with Mike's request > added. Unfortunately, the persist="selectedIndex" thing (added with the patch from attachment 267843 [details] [diff] [review]) regressed on trunk on 2007-09-14, with this change to nsPresShell.cpp: http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsPresShell.cpp&branch=3.1057&root=/cvsroot&subdir=/mozilla/layout/base&command=DIFF_FRAMESET&rev1=3.1056&rev2=3.1057 (it's unrelated to the selected=true attribute, NB) Having selectedIndex persist for cert manager is very useful, though - especially when bug 387480 will land (people will add cert overrides to the third tab, so it would be quite cumbersome if cert manager always opens with the first tab selected). FWIW, adding these lines to the end of LoadCerts() in certManager.js will "fix" the problem: var tab = document.getElementById("certmanagertabs").selectedIndex; if (tab != 0) { document.getElementById("certmanagertabs").selectedIndex = 0; document.getElementById("certmanagertabs").selectedIndex = tab; } [I'm not proposing this as a patch, of course, but maybe it helps to determine what exactly is happening... who can help with fixing this? Neil?]

Kaspar, thanks for your comment. I ran into this regression as well, and am very frustrated about it, but hadn't yet looked up this bug number. I would like to remove the persist="selectedIndex" until someone reports that it's really working.

(In reply to comment #27) > Created an attachment (id=283202) [details] > Patch to disable persistent selection and make cert manager usable Before rushing into reverting this, I would actually prefer to hear the opinion of someone familiar with XBL and the toolkit widgets... maybe there's an easy workaround to this. Having non-persistent tabs in cert manager is not what the user expects - the Advanced prefs e.g. also remembers the last tab (has code reading the selectedIndex from prefs.js though).

(In reply to comment #28) > Before rushing into reverting this, I would actually prefer to hear the opinion > of someone familiar with XBL and the toolkit widgets... maybe there's an easy > workaround to this. We should test it locally once someone gives us hope it might be fixed. > Having non-persistent tabs in cert manager is not what the user expects - the > Advanced prefs e.g. also remembers the last tab (has code reading the > selectedIndex from prefs.js though). Right now it looks completely broken and confusing. I really prefer a working basic functionality over something that's cool but broken.

(In reply to comment #25) > (In reply to comment #21) > > Both patches checked in (workaround and improvement), with Mike's request > > added. > > Unfortunately, the persist="selectedIndex" thing (added with the patch from > attachment 267843 [details] [diff] [review]) regressed on trunk on 2007-09-14, with this change to > nsPresShell.cpp: > > http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsPresShell.cpp&branch=3.1057&root=/cvsroot&subdir=/mozilla/layout/base&command=DIFF_FRAMESET&rev1=3.1056&rev2=3.1057 > If it's a regression from that, then you should file a bug, cc the author of the change, and mark it blocking. The correct fix for this is to then fix the regression. I have verified that the persist is working correctly in a Sept 12.

(In reply to comment #30) > If it's a regression from that, then you should file a bug, cc the author of > the change, and mark it blocking. Done - bug 373525 (I hope I picked the right component).

I meant bug 398289 (persist="selectedIndex" no longer working for tabbox elements). Sorry for the bugspam.

Comment on attachment 283202 [details] [diff] [review] Patch to disable persistent selection and make cert manager usable r+, but please include a comment to revert the change one the regression is fixed. bob

Added comment proposed by Bob. Carrying forward r+ Requesting approval for this regression workaround.

Comment on attachment 283229 [details] [diff] [review] Patch to disable persistent selection and make cert manager usable v2 [Backout: Comment 50] I checked in this patch to temporarily disable persistence.

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a9pre) Gecko/2007100304 Minefield/3.0a9pre] (nightly) (W2Ksp4) Checking "Tools > Options... > Advanced > Encryption > View Certificates" with this nightly (and new profile), which is before "2007-10-03 10:21 / kaie%kuix.de / Bug 373525" and "2007-10-03 16:38 / bzbarsky%mit.edu / Bug 394676": no bug, persistence works fine. Why wouldn't I see the regression ?

(In reply to comment #36) > Why wouldn't I see the regression ? Was "accidentally" fixed by another recent commit (see bug 398289 comment 5 for details), so it's now working again, also for cert manager... Kai, are you fine with re-enabling persistence again?

(In reply to comment #37) > Kai, are you fine with re-enabling persistence again? I support the idea to test without then uncheck this patch :->

Speaking of testing, and given that this has regressed and unregressed multiple times, can we get a test, either automatic or litmus, to catch it reliably in the future?

(In reply to comment #39) > Speaking of testing, and given that this has regressed and unregressed multiple > times, can we get a test, either automatic or litmus, to catch it reliably in > the future? In the meantime, a reftest has been added when bug 398289 was resolved - I've verified that it reliably detects a regression for persist="selectedIndex". So, can we revert the change from attachment 283229 [details] [diff] [review], please? (Does backing out need approval, too?)

(In reply to comment #40) > So, can we revert the change from attachment 283229 [details] [diff] [review], please? I'm ok with backing it out, r=kengert I've tested it locally, and it works for me, too. > (Does backing out need approval, too?) Not sure :-) We don't have "blocking1.9" So I conclude: Yes, we need approval to back it out.

Comment on attachment 283229 [details] [diff] [review] Patch to disable persistent selection and make cert manager usable v2 [Backout: Comment 50] Requesting approval to undo this change, because the real problem got fixed elsewhere.

I believe approval is not needed ... to undo a patch that was supposed to fix something which was/is not broken (at the checkin time).

Thanks to dsicore for giving approval for the backout. I backed out, re-enabling persistence. Marking fixed.

(In reply to comment #44) > Marking fixed. Thanks for reverting the change, Kai! I hope you don't mind me reopening the bug, since one (minor) issue still awaits a fix, cf. line 70 in certManager.xul: <!-- FIXME Add selected="true" to ca_tab when 373525 gets fixed. --> <tab id="ca_tab" label="&certmgr.tab.ca;"/> (I.e. undoing the change from attachment 270038 [details] [diff] [review], which was supposed to be a temporary workaround. Note that selected="true" will only apply to the very first time Cert Manager is opened, after that selectedIndex kicks in. You might have to clean localstore.rdf to see the effect.) Neil's proposed patch in bug 386202 indeed fixes this (separate) issue, so it would be nice to undo the "Patch v4, workaround", too. Gavin, would it be possible for you to review Neil's patch (bug 386202 attachment 276796 [details] [diff] [review]) "relatively soon"?

(In reply to comment #45) > Gavin, would it be possible for you to review Neil's patch (bug 386202 > attachment 276796 [details] [diff] [review]) "relatively soon"? I'll try to get to it today.

Comment on attachment 283229 [details] [diff] [review] Patch to disable persistent selection and make cert manager usable v2 [Backout: Comment 50] Resetting all approval1.9+ flags on bugs that have not been checked in by Oct 22 11:59 PM PDT. Please re-request approval if needed.

Comment on attachment 283229 [details] [diff] [review] Patch to disable persistent selection and make cert manager usable v2 [Backout: Comment 50] Landed by kaie on 2007-10-03 10:21.

Comment on attachment 283229 [details] [diff] [review] Patch to disable persistent selection and make cert manager usable v2 [Backout: Comment 50] Reset approval flag to + as it was already checked in.

(In reply to comment #48) > (From update of attachment 283229 [details] [diff] [review]) > Landed by kaie on 2007-10-03 10:21. (For the record: ... and backed out by '2007-10-09 15:32 kaie%kuix.de', = before Oct 22 too ;-))

The remaining issue (see comment 45) blocks for final release, but not M9.

Release triaging requested a priority for this bug. I think the remaining problem (comment 45) is not critical and should not block the release. I'd even propose to open a separate bug to deal with the remaining issue, so we can differ between "cert manager completely broken" (the initial problem) and "the right tab should be opened by default" (remaining problem).

Comment on attachment 270038 [details] [diff] [review] Patch v4, workaround [Backout: Comment 57] 'approval1.9=?': To be reverted, per comment 45.

let's be careful and let's wait a while for the code from bug 286202 to stabilize. they backed it out again.

(In reply to comment #54) > let's be careful and let's wait a while for the code from bug 286202 to > stabilize. they backed it out again. I think it's been long enough. Time to revert?

Ok, let's try. Reed, do you want to check it in? For the record, my comment 54 refered to bug 386202 (not 286202).

Backed out. Checking in security/manager/pki/resources/content/certManager.xul; /cvsroot/mozilla/security/manager/pki/resources/content/certManager.xul,v <-- certManager.xul new revision: 1.44; previous revision: 1.43 done

Verified fix on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2) Gecko/2007121120 Firefox/3.0b2.