Closed Bug 981941 Opened 6 years ago Closed 6 years ago

don't show the password autocomplete popup for about:accounts

Categories

(Firefox :: Sync, defect, P2)

defect

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 + verified
firefox30 --- verified

People

(Reporter: ckarlof, Assigned: Gavin)

References

Details

Attachments

(1 file, 2 obsolete files)

Seeing the password manager dialog on about:accounts is weird and it prompts them to set up sync which is confusing.
Component: Firefox Sync: Backend → Sync
Product: Mozilla Services → Firefox
Summary: respect autocomplete=off on about:accounts → don't show the password autocomplete popup for about:accounts
Attached patch hack patch (obsolete) — Splinter Review
Bug 956906 kind of screwed our plan for handling this issue.

This is one simple but ugly (very specific) way of accomplishing it.
Attachment #8388936 - Flags: feedback?(dolske)
Attached patch slightly less hacky patch (obsolete) — Splinter Review
This is another approach I thought of that is slightly more generic and thus maybe more tolerable. The "also check topmost window" bit is kind of arbitrary and specific to the about:accounts case. It doesn't work as-is because _getPasswordOrigin isn't compatible with "about:accounts" URIs, but you get the general idea.
Attachment #8388937 - Flags: feedback?(dolske)
Priority: -- → P2
Comment on attachment 8388936 [details] [diff] [review]
hack patch

Review of attachment 8388936 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ +419,5 @@
> +            if (topWin.document && topWin.document.documentURI == "about:accounts") {
> +                log("(form submission ignored -- about:accounts)");
> +                return;
> +            }
> +        }

I'd kill the pref. Shouldn't |win.top.document| always be true, since we're in a form-submit handler?

That should make the code shorter than your comment. Which could also be briefer. :)
Attachment #8388936 - Flags: feedback?(dolske) → feedback+
Comment on attachment 8388937 [details] [diff] [review]
slightly less hacky patch

Review of attachment 8388937 [details] [diff] [review]:
-----------------------------------------------------------------

I'd just as soon keep the hack simple; if we find needing to do this on more pages that are comfortably handled in an "if(a||b)" then we can extensify it.
Attachment #8388937 - Flags: feedback?(dolske) → feedback-
Attached patch patchSplinter Review
Attachment #8388936 - Attachment is obsolete: true
Attachment #8388937 - Attachment is obsolete: true
Attachment #8390884 - Flags: review?(dolske)
Comment on attachment 8390884 [details] [diff] [review]
patch

Review of attachment 8390884 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ +406,5 @@
>  
> +        // Somewhat gross hack - we don't want to show the "remember password"
> +        // notification on about:accounts for Firefox.
> +        let topWin = win.top;
> +        if (/^about:accounts($|\?)/i.test(topWin.document.documentURI)) {

blech.
Attachment #8390884 - Flags: review?(dolske) → review+
https://hg.mozilla.org/integration/fx-team/rev/943e3906d00f
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 30
Attachment #8390884 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/943e3906d00f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Keywords: verifyme
QA Contact: twalker
I was able to reproduce the issue with Nightly build from March 10, on the about:accounts page, when signing in with existing account or signing up with new account.

The "remember password" notification no longer displays on about:accounts, either when trying to sign in or when trying to sign up. Verified with latest 30 Aurora (Build ID: 20140425004002) and 29 RC (Build ID: 20140421221237), on Win 7 x64, Mac OS X 10.7.5, Ubuntu 12.10 x86.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.