Last Comment Bug 304309 - convert to satchel from wallet
: convert to satchel from wallet
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: Trunk
: All All
: -- enhancement with 2 votes (vote)
: ---
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
: 356773 361803 394806 (view as bug list)
Depends on: 255807 380931 397788
Blocks: 213376 390028 205881 286110 375881 380917 390021 390025 390158 390198
  Show dependency treegraph
 
Reported: 2005-08-11 05:55 PDT by Wayne Mery (:wsmwk, NI for questions)
Modified: 2010-03-20 13:35 PDT (History)
34 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Fix selectBy (2.03 KB, patch)
2007-02-17 13:55 PST, neil@parkwaycc.co.uk
jag-mozilla: review+
jag-mozilla: superreview+
Details | Diff | Review
Fixes urlbar too (d'oh) (4.48 KB, patch)
2007-02-26 09:51 PST, neil@parkwaycc.co.uk
jag-mozilla: superreview+
Details | Diff | Review
Moved aReverse and aPage into getNextIndex (6.19 KB, patch)
2007-03-02 05:27 PST, neil@parkwaycc.co.uk
ajschult: review+
jag-mozilla: superreview+
Details | Diff | Review
Fix selectedIndex (11.46 KB, patch)
2007-06-01 13:02 PDT, neil@parkwaycc.co.uk
ajschult: review+
jag-mozilla: superreview+
Details | Diff | Review
Fix Mac themeing (4.07 KB, patch)
2007-06-05 15:27 PDT, neil@parkwaycc.co.uk
asaf: review+
jag-mozilla: superreview+
Details | Diff | Review
WIP patch for the real wallet -> pwdmgr/satchel switch, v2 (16.36 KB, patch)
2007-06-06 04:06 PDT, Robert Kaiser (not working on stability any more)
no flags Details | Diff | Review
Change getOverrideValue() to overrideValue (3.73 KB, patch)
2007-06-17 16:37 PDT, neil@parkwaycc.co.uk
ajschult: review+
jag-mozilla: superreview+
Details | Diff | Review
Minor __AUTOCOMPLETE_BOX__ simplification (1.15 KB, patch)
2007-06-18 05:10 PDT, neil@parkwaycc.co.uk
jag-mozilla: review+
jag-mozilla: superreview+
Details | Diff | Review
Change column names (5.98 KB, patch)
2007-06-20 16:11 PDT, neil@parkwaycc.co.uk
no flags Details | Diff | Review
Change column names, take two (5.81 KB, patch)
2007-06-21 02:23 PDT, neil@parkwaycc.co.uk
ajschult: review+
jag-mozilla: superreview+
Details | Diff | Review
Rework the selectedIndex handling (10.31 KB, patch)
2007-06-21 16:41 PDT, neil@parkwaycc.co.uk
jag-mozilla: superreview+
Details | Diff | Review
Rework the selectedIndex handling (10.36 KB, patch)
2007-07-11 13:19 PDT, neil@parkwaycc.co.uk
ajschult: review+
Details | Diff | Review
Actually implement nsIAutoCompletePopup (4.38 KB, patch)
2007-07-13 09:55 PDT, neil@parkwaycc.co.uk
ajschult: review+
jag-mozilla: superreview+
Details | Diff | Review
Actually use the popup (3.24 KB, patch)
2007-07-13 09:57 PDT, neil@parkwaycc.co.uk
kairo: review+
gavin.sharp: review+
Details | Diff | Review
Draft patch for disabling wallet's form fill (11.07 KB, patch)
2007-07-26 14:31 PDT, Mark Banner (:standard8)
no flags Details | Diff | Review
Convert autocomplete-result-popup to type="panel" and disablehistory="false" to enablehistory="true" (15.64 KB, patch)
2007-07-28 10:39 PDT, neil@parkwaycc.co.uk
neil: review-
enndeakin: review+
Details | Diff | Review
Addressed review comments (7.67 KB, patch)
2007-07-28 16:04 PDT, neil@parkwaycc.co.uk
no flags Details | Diff | Review
Search engine bustage fix (1.19 KB, patch)
2007-07-29 12:21 PDT, neil@parkwaycc.co.uk
kairo: review+
Details | Diff | Review
satchel doesn't actually need passwordmgr (567 bytes, patch)
2007-08-02 13:24 PDT, neil@parkwaycc.co.uk
ted: review+
Details | Diff | Review

Description Wayne Mery (:wsmwk, NI for questions) 2005-08-11 05:55:53 PDT
satchel would be a nice improvement to forthcoming SM 1.1.
(backport satchel from Firefox?)
Comment 1 Andrew Schultz 2006-10-16 16:34:58 PDT
*** Bug 356773 has been marked as a duplicate of this bug. ***
Comment 2 Andrew Schultz 2006-10-16 16:52:41 PDT
*** Bug 356773 has been marked as a duplicate of this bug. ***
Comment 3 neil@parkwaycc.co.uk 2007-02-17 13:51:15 PST
1.5 will have satchel, I guess. I don't know whether we'll have dropped the xpfe autocomplete widget by then, but it only seems to need a few tweaks to work with satchel, which expects an nsIAutoCompletePopup; four of the functions don't exist and would need to be copied from the toolkit widget, while selectBy would need to be tweaked to work with the new signature and selectedIndex would need to be tweaked to use -1 as the sentinel instead of null.
Comment 4 neil@parkwaycc.co.uk 2007-02-17 13:55:11 PST
Created attachment 255487 [details] [diff] [review]
Fix selectBy
Comment 5 Mark Banner (:standard8) 2007-02-23 14:59:35 PST
It seems like to pick up satchel we also need to pick up password manager, just trying to pick up satchel there appears to be internal dependencies for nsFormFillController.cpp on toolkit's pm:

nsPasswordManager::AutoCompleteSearch
nsFormFillController.o: In function `nsSingleSignonPrompt':
/mozilla/smtest/mozilla/toolkit/components/satchel/src/../../passwordmgr/base/nsSingleSignonPrompt.h:64: undefined reference to `vtable for nsSingleSignonPrompt'

The second could probably be fixed, but I don't think the first can.

The question would therefore be can we pick up toolkit's pm with xpfe's autocomplete?
Comment 6 Mark Banner (:standard8) 2007-02-24 05:29:20 PST
I've just done another build - xpfe autocomplete, satchel + toolkit password manager, no wallet.

Password Manager seems to work ok, though I haven't checked multiple logins to the same web page. Satchel doesn't work as the autocompletepopup doesn't implement nsIAutoCompletePopup - as Neil commented in comment 3.

Other than that, we *may* be able to take both satchel + pm for xpfe trunk builds as well as sr ones.
Comment 7 Mark Banner (:standard8) 2007-02-24 05:35:36 PST
Something I've just noticed that would function differently with toolkit's password manager is even when you've saved the password, it will still prompt for a password again - this could be a bit annoying to the user in mail/address book, but perhaps we'd just have to fix that ;-)
Comment 8 jag (Peter Annema) 2007-02-26 09:13:31 PST
Comment on attachment 255487 [details] [diff] [review]
Fix selectBy

I assume you'll be updating urlbarBindings.xml too.

I would replace the ?: in |var reverse| with && || (idiom, and it took me a while to verify that the ?: did the same thing).
Comment 9 neil@parkwaycc.co.uk 2007-02-26 09:51:36 PST
Created attachment 256477 [details] [diff] [review]
Fixes urlbar too (d'oh)

Although I was wondering whether to move the dir/amount code into getNextIndex.

Unrelated bug: when you down arrow from the search highlight it wraps to the first result but when you up arrow from the first result the highlight cancels.
Comment 10 jag (Peter Annema) 2007-02-26 10:17:24 PST
Comment on attachment 256477 [details] [diff] [review]
Fixes urlbar too (d'oh)

+          var amount = aPage ? this.parentNode.pageCount : 1;

? this.parentNode.pageCount - 1 : 1, no?
Comment 11 neil@parkwaycc.co.uk 2007-03-02 05:27:40 PST
Created attachment 257013 [details] [diff] [review]
Moved aReverse and aPage into getNextIndex

I also stomped on the bug which makes down arrow from the search engine skip to selecting the first autocomplete result to match the up arrow behaviour.
Comment 12 Andrew Schultz 2007-05-16 23:52:28 PDT
Comment on attachment 257013 [details] [diff] [review]
Moved aReverse and aPage into getNextIndex

r=ajschult
Comment 13 Robert Kaiser (not working on stability any more) 2007-05-22 05:46:22 PDT
OK, I have the patch from in here applied and did some modifications that should go most of the way to get pwdmgr+satchel in. Now I'm running into an exception - we're missing an autocomplete interface, it seems.

This is what I get:

Fehler: [Exception... "Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsISupports.QueryInterface]"  nsresult: "0x80004002 (NS_NOINTERFACE)"  location: "JS frame :: chrome://global/content/bindings/browser.xml :: attachFormFill :: line 379"  data: no]
Quelldatei: chrome://global/content/bindings/browser.xml
Zeile: 379

The missing interface is nsIAutoCompletePopup, see http://mxr.mozilla.org/mozilla/source/toolkit/content/widgets/browser.xml#379
Comment 14 Robert Kaiser (not working on stability any more) 2007-05-22 06:05:39 PDT
http://ctemp.kairo.at/doc/hacking/bug304309-wip-v1.diff is a WIP patch that gets me to the point where I get the exception in comment #13

BTW, while digging into that, I saw that Thunderbird seems to still build wallet as well - that might explain why I see no password saving in mailnews (checkbox missing on password entering dialogs, no prefilled passwords used).

With the patch, passwords in browser are prefilled if you have only one name/pwd saved for that site (if you once triggered the "save passwasswords?" dialog for any site), if you have multiple names/passwords, you need to enter the name and on tabbing out of the name field, the password gets prefilled. No dropdown is shown on either username or form prefilling (probably connected with that exception).
Comment 15 Robert Kaiser (not working on stability any more) 2007-05-22 06:19:08 PDT
Thunderbird has bug 239131 about this, btw - just as a reference.
Comment 16 Philip Chee 2007-05-22 07:41:08 PDT
(In reply to comment 14)
> With the patch, passwords in browser are prefilled if you have only one
> name/pwd saved for that site (if you once triggered the "save passwasswords?"
> dialog for any site), if you have multiple names/passwords, you need to enter
> the name and on tabbing out of the name field, the password gets prefilled. No
> dropdown is shown on either username or form prefilling (probably connected
> with that exception).

No that's standard Firefox behaviour.
Comment 17 Philip Chee 2007-05-22 07:58:12 PDT
KaiRo see: Firefox Bug 376668 (Improve discoverability of PW autofill with multiple accounts)
Comment 18 neil@parkwaycc.co.uk 2007-06-01 13:02:42 PDT
Created attachment 266935 [details] [diff] [review]
Fix selectedIndex

Since the selected index has to be an integer, this is basically an s/null/-1/ exercise. However, there are a couple of enhancements:
1. Rather than manually invalidating and firing select events, I use the tree selection to do it all for me. This saves a bunch of code, and will eventually let me s/menuactive/selected/ in the CSS, thus eliminating the menuactive atom.
2. getRowAt already returns -1 which we were originally converting to null ;-)
Comment 19 jag (Peter Annema) 2007-06-01 21:33:11 PDT
Comment on attachment 266935 [details] [diff] [review]
Fix selectedIndex

There's some tabs in the selection getter and setter in autocomplete.xml, and you left a dump() in the textbox property getter. sr=jag with those fixed.
Comment 20 neil@parkwaycc.co.uk 2007-06-02 07:46:52 PDT
(In reply to comment #19)
>There's some tabs in the selection getter and setter in autocomplete.xml
Oops. Wrong .vimrc :-( Fixed locally.
Comment 21 Andrew Schultz 2007-06-04 19:21:06 PDT
Comment on attachment 266935 [details] [diff] [review]
Fix selectedIndex

>Index: xpfe/components/autocomplete/resources/content/autocomplete.xml
>           setTree: function(aTree)
>           {

The now-defunct mBoxObject is still referenced in this method.  With that fixed, r=ajschult
Comment 22 neil@parkwaycc.co.uk 2007-06-05 01:08:27 PDT
(In reply to comment #21)
>(From update of attachment 266935 [details] [diff] [review])
>>Index: xpfe/components/autocomplete/resources/content/autocomplete.xml
>>           setTree: function(aTree)
>>           {
>The now-defunct mBoxObject is still referenced in this method.
Oops... what happened was that originally I tried to remove mAtomSelected too but I changed my mind and decided to do that in a separate patch, however I restored too much code by mistake ;-)
Comment 23 neil@parkwaycc.co.uk 2007-06-05 15:27:21 PDT
Created attachment 267342 [details] [diff] [review]
Fix Mac themeing

I discovered that the autocomplete highlighting doesn't work in Mac Suiterunner, and by extension in Mac Thunderbird. The previous patch to autocomplete.xml accidentally fixed that by making the ::-moz-tree-XXX(selected) styles work so we can remove the old menuactive support and switch to selected throughout.
Comment 24 Henrik Skupin (:whimboo) 2007-06-05 23:50:52 PDT
(In reply to comment #23)
> I discovered that the autocomplete highlighting doesn't work in Mac
> Suiterunner, and by extension in Mac Thunderbird. The previous patch to
> autocomplete.xml accidentally fixed that by making the
> ::-moz-tree-XXX(selected) styles work so we can remove the old menuactive
> support and switch to selected throughout.

Neil, as I mentioned on IRC yesterday it is working fine for my trunk debug build. But there is one small nit. The last entry within the autocomplete list is cut-off at the bottom and all entries are too narrow to each other. Dunno how that was before on Mac because I have it at least for 3 month now.

Would this patch also be possible for Thunderbird 2.0.0.x without big modifications? For the 1.8 branch the autocomplete is also broken for Mac.
Comment 25 Robert Kaiser (not working on stability any more) 2007-06-06 04:06:04 PDT
Created attachment 267401 [details] [diff] [review]
WIP patch for the real wallet -> pwdmgr/satchel switch, v2

This is my current WIP version of the patch for the real switch from wallet to pwdmgr/satchel (parts of the patch are by Standard8). Note that if the implements I noted in autocomplete.xml is actually set on this line, SeaMonkey crashes when you focus a HTML textbox.
Comment 26 jag (Peter Annema) 2007-06-06 15:42:28 PDT
Comment on attachment 267342 [details] [diff] [review]
Fix Mac themeing

sr=jag
Comment 27 Robert Kaiser (not working on stability any more) 2007-06-11 03:13:16 PDT
Note to self or whoever will finish up the real switch: bug 380931 has moved most LoginManager prefs to all.js, so we don't need those in browser-prefs.js any more.
Comment 28 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-06-11 14:35:16 PDT
Comment on attachment 267342 [details] [diff] [review]
Fix Mac themeing

r=mano
Comment 29 :Ehsan Akhgari (busy, don't ask for review please) 2007-06-13 07:00:07 PDT
Bug 375881 dependency was accidentally removed by me.  Fixing it.  Sorry for the problem.
Comment 30 neil@parkwaycc.co.uk 2007-06-16 15:39:14 PDT
Comment on attachment 267401 [details] [diff] [review]
WIP patch for the real wallet -> pwdmgr/satchel switch, v2

>+ifdef MOZ_XUL
>+DIRS += \
>+	passwordmgr \
>+	satchel \
>+	$(NULL)
>+
> ifndef MOZ_SUITE
> # XXX Suite doesn't want these just yet
>-ifdef MOZ_XUL
> DIRS += \
> 	autocomplete \
Actually we will need all of autocomplete. (Currently we just export public.)

What happened to the "Log Out" menuitem?
Comment 31 Robert Kaiser (not working on stability any more) 2007-06-16 16:14:57 PDT
(In reply to comment #30)
> Actually we will need all of autocomplete.

OK, will include all of toolkit autocomplete in further WIP patches.

> What happened to the "Log Out" menuitem?

I don't know where/how to call it in toolkit. Firefox apparently doesn't have it. If it's really that useful, we should work on adding it in a followup bug.
Comment 32 neil@parkwaycc.co.uk 2007-06-17 16:37:54 PDT
Created attachment 268722 [details] [diff] [review]
Change getOverrideValue() to overrideValue

From nsIAutoCompletePopup.idl
readonly attribute AString overrideValue;
Comment 33 neil@parkwaycc.co.uk 2007-06-18 05:10:08 PDT
Created attachment 268783 [details] [diff] [review]
Minor __AUTOCOMPLETE_BOX__ simplification

A subsequent patch will remove the other references to __AUTOCOMPLETE_BOX__
Comment 34 neil@parkwaycc.co.uk 2007-06-20 16:11:02 PDT
Created attachment 269137 [details] [diff] [review]
Change column names

Toolkit expects the funky treecolAutoCompleteXXX names which I guess are less likely to collide with other elements (we have to use ids because of the way trees work). I also rewrote the way the comment column is shown, as toolkit seems to want to be able to set it dynamically (although I see no callers yet).
Comment 35 Andrew Schultz 2007-06-20 22:35:21 PDT
Comment on attachment 268722 [details] [diff] [review]
Change getOverrideValue() to overrideValue

>Index: suite/browser/urlbarBindings.xml
>-            return this.mSearchBox.getOverrideValue();
>+            return this.mSearchBox.overrideValue();

no ()
Comment 36 Andrew Schultz 2007-06-20 22:57:36 PDT
Comment on attachment 269137 [details] [diff] [review]
Change column names

>Index: xpfe/components/autocomplete/resources/content/autocomplete.xml
>-        this.tree.__AUTOCOMPLETE_BOX__ = this.textbox;
>-        this.treebody.__AUTOCOMPLETE_BOX__ = this.textbox;

__AUTOCOMPLETE_BOX__ is still referenced in autocomplete-tree and autocomplete-treebody.  Did you intend to also remove those?
Comment 37 neil@parkwaycc.co.uk 2007-06-21 02:23:28 PDT
Created attachment 269188 [details] [diff] [review]
Change column names, take two

Oops, the __AUTOCOMPLETE_BOX__ and selectedIndex changes were supposed to be part of the next patch, which reworks the way selectedIndex works ;-)
Comment 38 neil@parkwaycc.co.uk 2007-06-21 16:41:27 PDT
Created attachment 269298 [details] [diff] [review]
Rework the selectedIndex handling

* Moves the selectedIndex handling from the "view" to the resultsPopup
* Added a method to the "view" to handle a result click (toolkit uses this)
* Makes the "view" a property on the resultsPopup as well as the tree
* Removed the remaining __AUTOCOMPLETE_BOX__ variables
* Removed unused bindings

Note: This patch applies on top of attachment 269188 [details] [diff] [review].
Comment 39 Andrew Schultz 2007-07-04 07:47:14 PDT
Comment on attachment 269298 [details] [diff] [review]
Rework the selectedIndex handling

>Index: toolkit/content/xul.css

toolkit's autcomplete.xml still references the bits you're removing

>Index: xpfe/components/autocomplete/resources/content/autocomplete.xml
>+        this.tree.view = this.textbox.view;

this line was added in the previous patch
Comment 40 neil@parkwaycc.co.uk 2007-07-04 09:51:52 PDT
(In reply to comment #39)
>(From update of attachment 269298 [details] [diff] [review])
>>Index: toolkit/content/xul.css
>toolkit's autcomplete.xml still references the bits you're removing
Sorry for not using a bigger -u but that's inside a suite/tb #ifdef

>>Index: xpfe/components/autocomplete/resources/content/autocomplete.xml
>>+        this.tree.view = this.textbox.view;
>this line was added in the previous patch
Then that patch was incorrect, I think. Sorry.
Comment 41 jag (Peter Annema) 2007-07-07 13:25:48 PDT
Comment on attachment 269298 [details] [diff] [review]
Rework the selectedIndex handling

Looks ok to me. Remember to remove the this.tree.view = this.textbox.view line from the other patch.

sr=jag
Comment 42 neil@parkwaycc.co.uk 2007-07-11 13:19:51 PDT
Created attachment 271893 [details] [diff] [review]
Rework the selectedIndex handling

Like attachment 269298 [details] [diff] [review] but with extra null-testing required because of a bug currently marked as security-sensitive.
Comment 43 Andrew Schultz 2007-07-12 21:16:19 PDT
Comment on attachment 271893 [details] [diff] [review]
Rework the selectedIndex handling

>Index: xpfe/components/autocomplete/resources/content/autocomplete.xml
>-            return this.selectedIndex = this.getNextIndex(aReverse, aPage, this.selectedIndex, this.textbox.view.rowCount - 1);
>+            return this.selectedIndex = this.getNextIndex(aReverse, aPage, this.selectedIndex, this.view.rowCount - 1);

This same change in urlbarBindings.xml, right?  Or (with the other changes to autocomplete.xml here) you could change that to this.tree.view.rowCount, which seems toolkit-compatible.
Comment 44 neil@parkwaycc.co.uk 2007-07-13 09:55:20 PDT
Created attachment 272193 [details] [diff] [review]
Actually implement nsIAutoCompletePopup
Comment 45 neil@parkwaycc.co.uk 2007-07-13 09:57:10 PDT
Created attachment 272194 [details] [diff] [review]
Actually use the popup

This patch depends on attachment 272193 [details] [diff] [review] of course.
Comment 46 Andrew Schultz 2007-07-16 10:17:01 PDT
Comment on attachment 272193 [details] [diff] [review]
Actually implement nsIAutoCompletePopup

>-        <xul:panel type="autocomplete" ignorekeys="true" anonid="popup" class="autocomplete-result-popup" hidden="true" xbl:inherits="for=id,nomatch"/>
>+        <xul:panel type="autocomplete" anonid="popup" class="autocomplete-result-popup" xbl:inherits="for=id,nomatch"/>

Hmm.  removing ignorekeys doesn't seem to break anything, but AFAICT, it's not related to toolkit migration.  Is ignorekeys just obsolete (it seems only used by toolkit/xpfe autocomplete widgets)?
Comment 47 Andrew Schultz 2007-07-16 22:13:46 PDT
Comment on attachment 272193 [details] [diff] [review]
Actually implement nsIAutoCompletePopup

>+            this.width = aWidth;

This needs to be setAttribute or style.width.  r=me with that fixed assuming you have an answer to comment 46.
Comment 48 Mark Banner (:standard8) 2007-07-17 04:53:32 PDT
(In reply to comment #45)
> Created an attachment (id=272194) [details]
> Actually use the popup
> This patch depends on attachment 272193 [details] [diff] [review] of course.

I'm not 100% sure (having not tried building it) but won't this patch also need to incorporate some changes to the packaging files as we're not building libxul yet?

I'd also half expect to see wallet being disabled as you're enabling password manager and satchel - or is there still a bit more work to get those actually in use?
Comment 49 jag (Peter Annema) 2007-07-21 19:08:49 PDT
Comment on attachment 272193 [details] [diff] [review]
Actually implement nsIAutoCompletePopup

sr=jag
Comment 50 neil@parkwaycc.co.uk 2007-07-22 09:53:43 PDT
(In reply to comment #46)
>Is ignorekeys just obsolete
I believe it is now, but only for Enn's new <xul:panel> element.

(In reply to comment #47)
>(From update of attachment 272193 [details] [diff] [review])
>>+            this.width = aWidth;
>This needs to be setAttribute or style.width.
No it doesn't, nsXULElement::SetWidth calls SetAttr internally. See
NS_IMPL_XUL_STRING_ATTR(Width, width)

(In reply to comment #48)
>I'm not 100% sure (having not tried building it) but won't this patch also need
>to incorporate some changes to the packaging files as we're not building libxul
>yet?

>I'd also half expect to see wallet being disabled as you're enabling password
>manager and satchel - or is there still a bit more work to get those actually
>in use?
Probably yes to both, but I don't know enough about either aspect. Attachment 267401 [details] [diff] also seems to contain some other changes Kairo felt necessary.
Comment 51 Robert Kaiser (not working on stability any more) 2007-07-25 18:07:58 PDT
For removing wallet, we also need to replace password manager. I hope even the new login manager is a drop-in replacement for browser functionality, but mailnews needs some work, see bug 239131.
Password manager is still not nice if you have multiple passwords saved for a page, but we're on par with Firefox there - and bug 376668 targets to improve that.

I still need to test the real switch patch a bit more, perhaps there's a way to use the satchel part for the moment and wait for bug 239131 with the password part.
The good thing is that the patches here (I think the part of building toolkit autocomplete might also be needed) make the urlbar autocomplete work with places history (latest patch from bug 382187 + setting MOZ_PLACES=1).
Comment 52 Mark Banner (:standard8) 2007-07-26 00:55:57 PDT
(In reply to comment #51)
> Password manager is still not nice if you have multiple passwords saved for a
> page, but we're on par with Firefox there - and bug 376668 targets to improve
> that.

We also have bug 382437 that means the mailnews dialogs don't display correctly.

> I still need to test the real switch patch a bit more, perhaps there's a way to
> use the satchel part for the moment and wait for bug 239131 with the password
> part.

I'm fairly sure we could rip out the form fill part of wallet from its extension and keep the passowrd manager for now, especially as we're the only ones using that part of wallet. I'll try and have a look in the next couple of days if you want.
Comment 53 Mark Banner (:standard8) 2007-07-26 14:31:29 PDT
Created attachment 274055 [details] [diff] [review]
Draft patch for disabling wallet's form fill

This is what I think we need to do for disabling wallet's form fill. I've not tested it yet (my build is still going), but its hopefully a good first draft.

We need to be slightly careful wrt camino - I've found that they appear to use wallet.properties (I'm not sure how it gets there, but it seems to), and I think from that they may use some other wallet code as well.
Comment 54 Robert Kaiser (not working on stability any more) 2007-07-26 15:47:39 PDT
(In reply to comment #53)
> We need to be slightly careful wrt camino

Argh, yes, Camino-like-Firefox is using wallet - not sure what for, though (if only passwords or also form prefilling). If they depend on the form prefilling stuff, you should just make it #ifdef MOZ_CAMINO or whatever they are using. You cannot remove the MOZ_XUL_APP ifdefs, as they are still xpfe-based as well :(
Comment 55 Robert Kaiser (not working on stability any more) 2007-07-27 16:16:16 PDT
Comment on attachment 272194 [details] [diff] [review]
Actually use the popup

>Index: suite/browser/navigator.xul
>===================================================================
>+  <panel type="autocomplete" class="autocomplete-result-popup" id="PopupAutoComplete"/>

The idea to set that class here is nice, but I think I'd like better if we add
panel[type="autocomplete"]
as an additional selector to that rule in xul.css, because that gives us toolkit compat, which some extensions might expect.

r=me if you add that additional change:

Index: suite/browser/browser-prefs.js
===================================================================
 pref("signon.rememberSignons",              true);
 pref("signon.expireMasterPassword",         false);
+pref("browser.formfill.enable",             true);

-pref("wallet.captureForms",                 true);
+pref("wallet.captureForms",                 false);
 pref("wallet.enabled",                      true);


With this, wallet form prefilling should be disabled and satchel's activated.
I think we can get this in right now and defer the password manager changes and the real removal to when it's ready (I hope we convinced dolske enough to do the LoginManager changes we need for mailnews).
Comment 56 neil@parkwaycc.co.uk 2007-07-28 10:39:43 PDT
Created attachment 274290 [details] [diff] [review]
Convert autocomplete-result-popup to type="panel" and disablehistory="false" to enablehistory="true"

Enn because I'm removing some CSS popup workarounds that might not be relevant.
Comment 57 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-07-28 11:15:14 PDT
Comment on attachment 272194 [details] [diff] [review]
Actually use the popup

r=me on the change to toolkit/components/Makefile.in .
Comment 58 Neil Deakin 2007-07-28 11:32:50 PDT
Comment on attachment 274290 [details] [diff] [review]
Convert autocomplete-result-popup to type="panel" and disablehistory="false" to enablehistory="true"


>+.autocomplete-history-popup {
>+  -moz-binding: url("chrome://global/content/bindings/autocomplete.xml#autocomplete-history-popup");
>+}

The class doesn't seem to be used in toolkit. I don't mind this being here, but I notice that there are references in the theme to it which we could remove, or remove in a follow up bug.

> .autocomplete-history-dropmarker {
>-  -moz-binding: url("chrome://global/content/autocomplete.xml#history-dropmarker");
>+  display: none;
>+}
>+
>+.autocomplete-history-dropmarker {
>+  display: -moz-box;
>+  -moz-binding: url("chrome://global/content/bindings/autocomplete.xml#history-dropmarker");

Two uses of 'display' here, but neither should be needed as dropmarker is -moz-box already.

I think that removing the popupset hackery should be ok too as popups don't lay out when they are closed any more.
Comment 59 neil@parkwaycc.co.uk 2007-07-28 11:56:58 PDT
Comment on attachment 274290 [details] [diff] [review]
Convert autocomplete-result-popup to type="panel" and disablehistory="false" to enablehistory="true"

KaiRo pointed out that I built toolkit and xpfe in the wrong order so it looked like this worked when it doesn't.
Comment 60 neil@parkwaycc.co.uk 2007-07-28 16:04:52 PDT
Created attachment 274309 [details] [diff] [review]
Addressed review comments

Including panel[type="autocomplete"], preferences and packaging.
Comment 61 Robert Kaiser (not working on stability any more) 2007-07-28 19:13:33 PDT
OK, with this in and satchel activated and working (yay!) I think we should split off all remaining work into followups and close this as FIXED.

I'll file followups for password manager and for the RTL stuff I had in my WIP patch once I'm really awake again.

Neil, I'll leave it to you to mark this bug resolved and file any followup autocomplete issues :)
Comment 62 Caio Tiago Oliveira (asrail) 2007-07-29 10:48:13 PDT
Will the disappearing of the search option on the URL bar be addressed by some of these follow-ups above?

Comment 63 neil@parkwaycc.co.uk 2007-07-29 12:21:05 PDT
Created attachment 274382 [details] [diff] [review]
Search engine bustage fix
Comment 64 neil@parkwaycc.co.uk 2007-08-02 13:24:37 PDT
Created attachment 274994 [details] [diff] [review]
satchel doesn't actually need passwordmgr

After a closer examination of the code satchel only calls passwordmgr without null-checking if passwordmgr has already called satchel.
Comment 65 neil@parkwaycc.co.uk 2007-08-02 13:25:59 PDT
Comment on attachment 274994 [details] [diff] [review]
satchel doesn't actually need passwordmgr

After a closer examination of the code satchel only calls passwordmgr without null-checking if passwordmgr has already called satchel.
Comment 66 Ted Mielczarek [:ted.mielczarek] 2007-08-02 14:58:05 PDT
Comment on attachment 274994 [details] [diff] [review]
satchel doesn't actually need passwordmgr

Is there a bug number you could put in that comment for the work to fix that XXX?
Comment 67 neil@parkwaycc.co.uk 2007-08-02 15:41:31 PDT
Comment on attachment 274994 [details] [diff] [review]
satchel doesn't actually need passwordmgr

Playing safe because I can't figure out how the rules apply to changes in toolkit Makefiles that only affect SeaMonkey.

(In reply to comment #66)
>Is there a bug number you could put in that comment for the work to fix that XXX?
That would be bug 239131.
Comment 68 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-08-04 18:35:43 PDT
Comment on attachment 274994 [details] [diff] [review]
satchel doesn't actually need passwordmgr

You don't need approval for toolkit patches at this point (let alone toolkit patches that only affect SeaMonkey), see http://groups.google.com/group/mozilla.dev.planning/browse_thread/thread/f569a23ce266a451/265344ae5291dee0 .
Comment 69 Mark Banner (:standard8) 2007-09-04 00:12:40 PDT
*** Bug 394806 has been marked as a duplicate of this bug. ***
Comment 70 zug_treno 2008-01-29 13:59:16 PST
*** Bug 361803 has been marked as a duplicate of this bug. ***
Comment 71 Robert Kaiser (not working on stability any more) 2008-02-06 05:50:26 PST
Is there a reason why this bug is still open? IIRC, satchel has been working fine in SeaMonkey trunk for some time now...
Comment 72 neil@parkwaycc.co.uk 2008-02-06 05:54:39 PST
I guess not...
Comment 73 Gene Pavlovsky 2009-10-15 14:33:09 PDT
Hi! Just got Seamonkey 2RC1 to try it out, and noticed form manager's gone. 
With the previous Form Manager, I was able to quickly fill out standard fields like Name, Email, Address etc. in long forms by just doing tools > form manager > fill form data (or smth to that effect). Is this feature now gone?
Comment 74 neil@parkwaycc.co.uk 2009-10-15 15:45:47 PDT
The feature to fill in whole forms at once has gone, yes. Without the aid of an add-on the best you can do is to double-click each field in turn and pick the (preferred) value from the list each time. While it would be possible to write an add-on that would do this for all input fields automatically I don't actually know whether such an add-on exists.
Comment 75 Gene Pavlovsky 2009-10-15 15:50:15 PDT
Thought so. And the old manager is totally gone, I suppose. The new one is nice, sometimes, but it's sad when a new version of software loses some of the nice features the previous version had...
Comment 76 Henrik Skupin (:whimboo) 2009-10-15 16:05:41 PDT
I believe Roboform is an application/add-on most of the people are using now to store passwords and form data. See http://www.roboform.com/.
Comment 77 Wyatt Childers 2010-03-20 13:35:09 PDT
Lastpass is also a common alternative.

Note You need to log in before you can comment on or make changes to this bug.