Matthew N. [:MattN] Reporter
	Description • 8 years ago

STR:
1) Load http://www.tmngo.ca/hbocanada/
2) Click Sign In
3) Choose Bell
4) Right-click in the username field and choose to Fill one of your saved logins.

Expected result:
My login is filled

Actual result:
> LoginManagerContent: fillForm: There is no login form anymore. The form may have been removed or the document may have changed. LoginManagerContent.jsm:469
is logged and the login isn't filled.

This is likely a bug from the initial implementation.

	Matthew N. [:MattN] Reporter
	Comment 1 • 8 years ago

The log message is because `this.stateForDocument(topDocument).loginFormForFill` is null which may be a bug but really I think that block should be skipped altogether when `inputElement` is truthy.

> if (!topState.loginFormForFill) {

should be

> if (!inputElement && !topState.loginFormForFill) {

in LoginManagerContent.jsm.

A test can be added to toolkit/components/passwordmgr/test/browser/browser_context_menu.js though we'll need to make sure it fails without the patch as I'm not sure what the exact conditions are that cause `loginFormForFill` to be null at this stage. Once we know that cause it would probably be worth filing a separate bug on fixing `loginFormForFill`.

	Saad Quadri [:saadq] Assignee
	Comment 2 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/58356/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58356/

	Saad Quadri [:saadq] Assignee
	Comment 3 • 8 years ago

Comment on attachment 8760964 [details]
Bug 1278158 - Allowing filling login forms with an inputElement but without loginFormForFill.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58356/diff/1-2/

	Saad Quadri [:saadq] Assignee
	Comment 4 • 8 years ago

Comment on attachment 8760964 [details]
Bug 1278158 - Allowing filling login forms with an inputElement but without loginFormForFill.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58356/diff/2-3/

	Matthew N. [:MattN] Reporter
	Comment 5 • 8 years ago

Comment on attachment 8760964 [details]
Bug 1278158 - Allowing filling login forms with an inputElement but without loginFormForFill.

https://reviewboard.mozilla.org/r/58356/#review55486

Nit: The commit message ideally describes what is being changed/fixed instead of the problem (which the bug summary usually describes). Example:
Bug 1278158 - Allowing filling login forms with an inputElement but without loginFormForFill. r=MattN

::: toolkit/components/passwordmgr/test/browser/browser_context_menu_iframe.js:1
(Diff revision 3)
> +/*

It seems like the `hg copy` metadata was lost so this isn't showing up as a copy anymore.

::: toolkit/components/passwordmgr/test/browser/browser_context_menu_iframe.js:9
(Diff revision 3)
> +
> +"use strict";
> +
> +// The hostname for the test URIs.
> +const TEST_HOSTNAME = "https://example.com";
> +const IFRAME_PAGE_PATH = "/browser/toolkit/components/passwordmgr/test/browser/form_basic_iframe.html";

Nit: Add a note that this test needs to run with the only login form being in a frame to test when loginFormForFill is null.

	Saad Quadri [:saadq] Assignee
	Comment 6 • 8 years ago

Comment on attachment 8760964 [details]
Bug 1278158 - Allowing filling login forms with an inputElement but without loginFormForFill.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58356/diff/3-4/

	Saad Quadri [:saadq] Assignee
	Comment 7 • 8 years ago

Comment on attachment 8760964 [details]
Bug 1278158 - Allowing filling login forms with an inputElement but without loginFormForFill.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58356/diff/4-5/

	Saad Quadri [:saadq] Assignee
	Comment 8 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1cf24df601b6

	Pulsebot
	Comment 9 • 8 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/9694e3713635
Allowing filling login forms with an inputElement but without loginFormForFill. r=MattN

	Carsten Book [:Tomcat]
	Comment 10 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/9694e3713635

	Matthew N. [:MattN] Reporter
	Comment 12 • 8 years ago

Comment on attachment 8760964 [details]
Bug 1278158 - Allowing filling login forms with an inputElement but without loginFormForFill.

Approval Request Comment
[Feature/regressing bug #]: bug 433238
[User impact if declined]: The login fill context menu item is broken in subframes.
[Describe test coverage new/current, TreeHerder]: New browser-chrome test and a month of baking.
[Risks and why]: Low risk trivial change to a single `if` condition which was obviously wrong by inspection
[String/UUID change made/needed]: None

	Gerry Chang [:gchang]
	Comment 13 • 8 years ago

Comment on attachment 8760964 [details]
Bug 1278158 - Allowing filling login forms with an inputElement but without loginFormForFill.

This patch fixes a UI issue where the user login is not filled. Take it in 49 aurora.

	Matthew N. [:MattN] Reporter
	Comment 14 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/b76912ba6b66

	Cornel Ionce [:noni] [Hubs QA]
	Comment 15 • 8 years ago

Reproduced this issue using Fx48.0RC build 2 on Windows 10 x64.

Confirming the fix across platforms using:
- latest 50.0a1 Nightly, build ID 20160728030208
- latest 49.0a2 Aurora, build ID 20160728004010.