convert to satchel from wallet

RESOLVED FIXED

Status

SeaMonkey
UI Design
--
enhancement
RESOLVED FIXED
12 years ago
7 years ago

People

(Reporter: wsmwk, Assigned: neil@parkwaycc.co.uk)

Tracking

(Blocks: 2 bugs)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(14 attachments, 5 obsolete attachments)

6.19 KB, patch
Andrew Schultz
: review+
jag (Peter Annema)
: superreview+
Details | Diff | Splinter Review
11.46 KB, patch
Andrew Schultz
: review+
jag (Peter Annema)
: superreview+
Details | Diff | Splinter Review
4.07 KB, patch
mano
: review+
jag (Peter Annema)
: superreview+
Details | Diff | Splinter Review
16.36 KB, patch
Details | Diff | Splinter Review
3.73 KB, patch
Andrew Schultz
: review+
jag (Peter Annema)
: superreview+
Details | Diff | Splinter Review
1.15 KB, patch
jag (Peter Annema)
: review+
jag (Peter Annema)
: superreview+
Details | Diff | Splinter Review
5.81 KB, patch
Andrew Schultz
: review+
jag (Peter Annema)
: superreview+
Details | Diff | Splinter Review
10.36 KB, patch
Andrew Schultz
: review+
Details | Diff | Splinter Review
4.38 KB, patch
Andrew Schultz
: review+
jag (Peter Annema)
: superreview+
Details | Diff | Splinter Review
11.07 KB, patch
Details | Diff | Splinter Review
15.64 KB, patch
neil@parkwaycc.co.uk
: review-
Neil Deakin (not available until Aug 9)
: review+
Details | Diff | Splinter Review
7.67 KB, patch
Details | Diff | Splinter Review
1.19 KB, patch
Robert Kaiser
: review+
Details | Diff | Splinter Review
567 bytes, patch
ted
: review+
Details | Diff | Splinter Review
satchel would be a nice improvement to forthcoming SM 1.1.
(backport satchel from Firefox?)

Comment 1

11 years ago
*** Bug 356773 has been marked as a duplicate of this bug. ***

Comment 2

11 years ago
*** Bug 356773 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 3

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

Comment 4

11 years ago
Created attachment 255487 [details] [diff] [review]
Fix selectBy
Attachment #255487 - Flags: review?(jag)
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?
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.
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

11 years ago
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).
Attachment #255487 - Flags: superreview+
Attachment #255487 - Flags: review?(jag)
Attachment #255487 - Flags: review+
(Assignee)

Comment 9

11 years ago
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.
Attachment #255487 - Attachment is obsolete: true
Attachment #256477 - Flags: superreview?(jag)

Comment 10

11 years ago
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?
Attachment #256477 - Flags: superreview?(jag) → superreview+
(Assignee)

Comment 11

11 years ago
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.
Attachment #256477 - Attachment is obsolete: true
Attachment #257013 - Flags: superreview?(jag)

Updated

10 years ago
Blocks: 286110
(Assignee)

Updated

10 years ago
Attachment #257013 - Flags: review?(ajschult)

Updated

10 years ago
Blocks: 380917

Comment 12

10 years ago
Comment on attachment 257013 [details] [diff] [review]
Moved aReverse and aPage into getNextIndex

r=ajschult
Attachment #257013 - Flags: review?(ajschult) → review+

Comment 13

10 years ago
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

10 years ago
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

10 years ago
Thunderbird has bug 239131 about this, btw - just as a reference.

Comment 16

10 years ago
(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

10 years ago
KaiRo see: Firefox Bug 376668 (Improve discoverability of PW autofill with multiple accounts)

Updated

10 years ago
Attachment #257013 - Flags: superreview?(jag) → superreview+
(Assignee)

Comment 18

10 years ago
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 ;-)
Attachment #266935 - Flags: superreview?(jag)
Attachment #266935 - Flags: review?(ajschult)

Comment 19

10 years ago
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.
Attachment #266935 - Flags: superreview?(jag) → superreview+
(Assignee)

Comment 20

10 years ago
(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

10 years ago
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
Attachment #266935 - Flags: review?(ajschult) → review+

Updated

10 years ago
Assignee: jag → neil
(Assignee)

Comment 22

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

Comment 23

10 years ago
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.
Attachment #267342 - Flags: superreview?(jag)
Attachment #267342 - Flags: review?(mano)
(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.
Status: NEW → ASSIGNED

Comment 25

10 years ago
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

10 years ago
Comment on attachment 267342 [details] [diff] [review]
Fix Mac themeing

sr=jag
Attachment #267342 - Flags: superreview?(jag) → superreview+

Comment 27

10 years ago
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.
Depends on: 380931
Comment on attachment 267342 [details] [diff] [review]
Fix Mac themeing

r=mano
Attachment #267342 - Flags: review?(mano) → review+

Updated

10 years ago
Blocks: 375881
No longer blocks: 375881
Bug 375881 dependency was accidentally removed by me.  Fixing it.  Sorry for the problem.
Blocks: 375881
(Assignee)

Comment 30

10 years ago
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

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

Comment 32

10 years ago
Created attachment 268722 [details] [diff] [review]
Change getOverrideValue() to overrideValue

From nsIAutoCompletePopup.idl
readonly attribute AString overrideValue;
Attachment #268722 - Flags: superreview?(jag)
Attachment #268722 - Flags: review?(ajschult)

Updated

10 years ago
Attachment #268722 - Flags: superreview?(jag) → superreview+
(Assignee)

Comment 33

10 years ago
Created attachment 268783 [details] [diff] [review]
Minor __AUTOCOMPLETE_BOX__ simplification

A subsequent patch will remove the other references to __AUTOCOMPLETE_BOX__
Attachment #268783 - Flags: superreview?(jag)
Attachment #268783 - Flags: review?(jag)

Updated

10 years ago
Attachment #268783 - Flags: superreview?(jag)
Attachment #268783 - Flags: superreview+
Attachment #268783 - Flags: review?(jag)
Attachment #268783 - Flags: review+
(Assignee)

Comment 34

10 years ago
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).
Attachment #269137 - Flags: superreview?(jag)
Attachment #269137 - Flags: review?(ajschult)

Comment 35

10 years ago
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 ()
Attachment #268722 - Flags: review?(ajschult) → review+

Comment 36

10 years ago
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?
(Assignee)

Comment 37

10 years ago
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 ;-)
Attachment #269137 - Attachment is obsolete: true
Attachment #269188 - Flags: superreview?(jag)
Attachment #269188 - Flags: review?(ajschult)
Attachment #269137 - Flags: superreview?(jag)
Attachment #269137 - Flags: review?(ajschult)
Blocks: 380641
(Assignee)

Updated

10 years ago
No longer blocks: 380641
(Assignee)

Comment 38

10 years ago
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].
Attachment #269298 - Flags: superreview?(jag)
Attachment #269298 - Flags: review?(ajschult)

Updated

10 years ago
Attachment #269188 - Flags: review?(ajschult) → review+

Comment 39

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

Comment 40

10 years ago
(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.

Updated

10 years ago
Attachment #269188 - Flags: superreview?(jag) → superreview+

Comment 41

10 years ago
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
Attachment #269298 - Flags: superreview?(jag) → superreview+
(Assignee)

Comment 42

10 years ago
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.
Attachment #269298 - Attachment is obsolete: true
Attachment #271893 - Flags: review?(ajschult)
Attachment #269298 - Flags: review?(ajschult)

Comment 43

10 years ago
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.
Attachment #271893 - Flags: review?(ajschult) → review+
(Assignee)

Comment 44

10 years ago
Created attachment 272193 [details] [diff] [review]
Actually implement nsIAutoCompletePopup
Attachment #272193 - Flags: superreview?(jag)
Attachment #272193 - Flags: review?(ajschult)
(Assignee)

Comment 45

10 years ago
Created attachment 272194 [details] [diff] [review]
Actually use the popup

This patch depends on attachment 272193 [details] [diff] [review] of course.
Attachment #272194 - Flags: review?(kairo)

Comment 46

10 years ago
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

10 years ago
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.
Attachment #272193 - Flags: review?(ajschult) → review+
(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

10 years ago
Comment on attachment 272193 [details] [diff] [review]
Actually implement nsIAutoCompletePopup

sr=jag
Attachment #272193 - Flags: superreview?(jag) → superreview+
(Assignee)

Comment 50

10 years ago
(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

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

10 years ago
(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

10 years ago
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).
Attachment #272194 - Flags: review?(kairo) → review+
(Assignee)

Comment 56

10 years ago
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.
Attachment #274290 - Flags: superreview?(jag)
Attachment #274290 - Flags: review?(gavin.sharp)
Attachment #274290 - Flags: review?(enndeakin)
Comment on attachment 272194 [details] [diff] [review]
Actually use the popup

r=me on the change to toolkit/components/Makefile.in .
Attachment #272194 - Flags: review+
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.
Attachment #274290 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 59

10 years ago
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.
Attachment #274290 - Flags: superreview?(jag)
Attachment #274290 - Flags: review?(gavin.sharp)
Attachment #274290 - Flags: review-
(Assignee)

Comment 60

10 years ago
Created attachment 274309 [details] [diff] [review]
Addressed review comments

Including panel[type="autocomplete"], preferences and packaging.
Attachment #272194 - Attachment is obsolete: true

Comment 61

10 years ago
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 :)
Blocks: 390021

Updated

10 years ago
Blocks: 390025

Updated

10 years ago
Blocks: 390028
Will the disappearing of the search option on the URL bar be addressed by some of these follow-ups above?

(Assignee)

Comment 63

10 years ago
Created attachment 274382 [details] [diff] [review]
Search engine bustage fix
Attachment #274382 - Flags: review?(kairo)

Updated

10 years ago
Attachment #274382 - Flags: review?(kairo) → review+
Blocks: 390158
Blocks: 390198
(Assignee)

Comment 64

10 years ago
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.
Attachment #274994 - Flags: review?
(Assignee)

Comment 65

10 years ago
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.
Attachment #274994 - Flags: review? → review?(ted.mielczarek)
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?
Attachment #274994 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 67

10 years ago
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.
Attachment #274994 - Flags: approval1.9?
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 .
Attachment #274994 - Flags: approval1.9?
Duplicate of this bug: 394806
(Assignee)

Updated

10 years ago
Depends on: 397788

Updated

10 years ago
Duplicate of this bug: 361803

Comment 71

10 years ago
Is there a reason why this bug is still open? IIRC, satchel has been working fine in SeaMonkey trunk for some time now...
(Assignee)

Comment 72

10 years ago
I guess not...
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Blocks: 213376

Updated

8 years ago
QA Contact: ui-design

Comment 73

8 years ago
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?
(Assignee)

Comment 74

8 years ago
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

8 years ago
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...
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

7 years ago
Lastpass is also a common alternative.
You need to log in before you can comment on or make changes to this bug.