> Did this work before? N/A
It works on Chromium 67.0.3396.62.

Able to reproduce the issue on Windows 10, mac 10.13.3 and Ubuntu 17.10 using chrome reported version #68.0.3440.15 and latest canary #69.0.3466.0.

Bisect Information:
=====================
Good build: 68.0.3405.0
Bad Build : 68.0.3406.0

Change Log URL: 
https://chromium.googlesource.com/chromium/src/+log/90807f86967ccb016b4958300fb642c454a988ae..ad1435b289845b2faab59c2423071642889871c3

From the above change log suspecting below change
Change-Id: I6aedca91ff0369c541891c96336125dff9adeac0
Reviewed-on: https://chromium-review.googlesource.com/1021573

vabr@ - Could you please check whether this is caused with respect to your change, if not please help us in assigning it to the right owner. 
Also ccing the reviewer of the issue as the author vabr@ chromium id is buing used 50% only in 2018.
Note: Adding stable blocker for M-68 as it seems to be a recent regression. Please feel free to remove the same if not appropriate.

Thanks...!!

Thanks for the report!

The bisect is correct, this was changed by r553059.

At the moment I am trying to understand a few things. I will dump them here not in the hope of having them answered, but keep a record for myself (and make progress more transparent). When I refer to old parser and new parser, I'm referring to whether I run Chrome with chrome://flags/#new-password-form-parsing disabled or enabled, respectively.

* r553059 should only have affected form parsing before the page is displayed, and it does not seem the case here

* The old parser should have chosen the visible field based on its proximity to the password. Don't know why it chose the more distant (in the order of the nodes inside the <form>) "openid.usernamesecret" yet.

* The new parser utilises crowd-sourced hints. Unfortunately, because the page reuses the same form for both a login and a sign-up form, the server-provided hints apply to the sign-up form. So the new parser does not choose the "openid.usernamesecret" but it does choose the "username" one (which is only visible in the sign-up mode). This reuse of the form also causes other issues (bug 855385), and I'm not sure what to do about that yet.


For fixing this, focusing on visibility indeed seems best. Just reverting r553059 is not a good option, though, because there were other significant downsides to the state before it.

I can now answer the middle question:

> The old parser should have chosen the visible field based on its proximity to the password.

Before deciding on what is the username based on its position relative to the password, Chrome also tries to get the hint from the fields' labels and names. It preferred the wrong one, because its name contained "username", while the correct one is named "email". This check is, by mistake, not combined with the visibility. I will fix that.

There was also a related issue with the old parser considering also non-text fields (e.g., radio buttons) for username. This was masked by the above issue, and it is getting fixed in https://crrev.com/c/1111849.

I think I understand the causes now.

A correction to #6 above -- actually, the hint coming from labels and names is combined with visibility. But having a user-typed or autofilled value trumps even the visibility. And this is how the "openid.usernamesecret" got to be kept in the shortlist:
 (1) On initial parsing, when Blink did not render the page and hence visibility could not be checked, "openid.usernamesecret" made it among the candidates despite being empty and invisible.
 (2) Thanks the "username" in its name, it was picked by the username classifier as the top username candidate.
 (3) As a result, it was filled.
 (4) Later, when Blink rendered the page and parsing could be repeated with the visibility information included, "openid.usernamesecret" already had its value autofilled and hence was considered good enough despite being invisible.

This explains the first two questions I had in #5. In particular, despite the effect of r553059 is only caused before Blink renders the page, it is sticky due to the autofilled value.

So far I see not filling before Blink renders as the best solution. Filling before rendering is not seen by the user and messes up how we parse the forms. We still want to understand that there are forms on the page to request stored data early from PasswordStore, but we should first fill after Blink renders. I'll see if I can do this without breaking anything.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1c6c3805f43604e78c23fdd48ad73b20b004804c

commit 1c6c3805f43604e78c23fdd48ad73b20b004804c
Author: Vaclav Brozek <vabr@chromium.org>
Date: Fri Jun 22 13:26:33 2018

Only consider text fields in password forms

The old (renderer-process) FormData->PasswordForm parser failed to
ignore non-text inputs, such as radio buttons.

This CL adds filtering non-text fields out.

Note that the new parser (browser-process) does filter out non-text
field already.

Bug:  851808 
Change-Id: I60b121cb5cc15bdae0d96dba9119c2d704d079b2
Reviewed-on: https://chromium-review.googlesource.com/1111849
Reviewed-by: Dominic Battré <battre@chromium.org>
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569591}
[modify] https://crrev.com/1c6c3805f43604e78c23fdd48ad73b20b004804c/components/autofill/content/renderer/password_form_conversion_utils.cc
[modify] https://crrev.com/1c6c3805f43604e78c23fdd48ad73b20b004804c/components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc

To implement "not filling before Blink" renders I'd ideally like to remove PasswordAutofillAgent::DidFinishDocumentLoad. I need to clarify whether this is doable without causing more breakages (there is also a related, although non-blocking  issue 855598 ). This might take some time.

login.ubuntu.org needed to put together a combination of breakages:
(1) Squash a login form and a sign-up form into one to render server hints useless.
(2) Use a hidden field to detect autocompletion and name it with something hinting at username.

I'd still like to fix this for M68. I don't think this should block the release, though, because we cannot block Chrome development on sites being uncooperative.

A solution seems coming, more updates on this problem at  https://crbug.com/855598#c14 .

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ff6d76db8c032fa23bc8c215a87b53e99304ec5a

commit ff6d76db8c032fa23bc8c215a87b53e99304ec5a
Author: Vaclav Brozek <vabr@chromium.org>
Date: Tue Jul 17 23:58:02 2018

Remove autofill::form_util::ScopedLayoutPreventer

ScopedLayoutPreventer was used to avoid detecting visibility of form
elements in situations when it was believed to cause pre-mature layout
(see https://crbug.com/595078). Visibility is approximated through
calling IsFocusable on the WebNodes corresponding to forms and their
fields.

Looking at recent documentation, though, the WebNode-based API seems
robust enough to prevent such dangerous calls:
  (1) The contract of Element::IsFocusable() only states the need to
      call Document::UpdateStyleAndLayoutTree() beforehand.
  (2) Document::UpdateStyleAndLayoutTree() does not cause the layout.
  (3) WebNode::IsFocusable() causes calling
      Document::UpdateStyleAndLayoutTree() as necessary before
      delegating to Element::IsFocusable().
See  https://crbug.com/855598#c14  for more context.

As a result, ScopedLayoutPreventer blocked visibility checks
unnecessarily, leading to bugs like  https://crbug.com/851808 .

Therefore this CL removes ScopedLayoutPreventer.

Bug:  855598 , 851808 ,595078
Change-Id: Ic415919be5c40821dfa965eeb16372ae92ba92d5
Reviewed-on: https://chromium-review.googlesource.com/1136531
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: Stefan Zager <szager@chromium.org>
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575859}
[modify] https://crrev.com/ff6d76db8c032fa23bc8c215a87b53e99304ec5a/components/autofill/content/renderer/form_autofill_util.cc
[modify] https://crrev.com/ff6d76db8c032fa23bc8c215a87b53e99304ec5a/components/autofill/content/renderer/form_autofill_util.h
[modify] https://crrev.com/ff6d76db8c032fa23bc8c215a87b53e99304ec5a/components/autofill/content/renderer/form_autofill_util_browsertest.cc
[modify] https://crrev.com/ff6d76db8c032fa23bc8c215a87b53e99304ec5a/components/autofill/content/renderer/password_autofill_agent.cc

r575859 above fixed the bug for Chrome 69.

There is a new form parser coming to Chrome (currently behind chrome://flags/#new-password-form-parsing), for which the page is still broken, and I have a fix at https://crrev.com/c/1141873. I intend to land or merge that in M69 as well.

However, I'd rather not merge the fixes to Chrome 68, because the fix in r575859 had some level of uncertainty and we might need the full release circle to deal with potential crashes it might cause.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0355195267df26728e5f9c2df28ac679435ad195

commit 0355195267df26728e5f9c2df28ac679435ad195
Author: Vaclav Brozek <vabr@chromium.org>
Date: Wed Jul 18 15:10:44 2018

Improve visibility handling in FormData->PasswordForm parser

When the new FormData->PasswordForm parser looks for the username with
its basic heuristics, it only considers the most likely fields the
user would interact with (visible > with some value > the rest).

This filter is on purpose not applied to server hints and autocomplete
attribute analysis.

But it should be applied to the local HTML classifier.

This CL makes that happen by reordering the analysis steps to do basic
heuristics before the HTML classifier, because the heuristics need to
compute the bar for interactability. With that bar computed, the
parser additionally runs the HTML classifier on fields meeting that
bar.

Bug:  851808 
Change-Id: I0e54b6c1c6387b6130d297131b0dcb83fe52e265
Reviewed-on: https://chromium-review.googlesource.com/1141873
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576063}
[modify] https://crrev.com/0355195267df26728e5f9c2df28ac679435ad195/components/password_manager/core/browser/form_parsing/form_parser.cc
[modify] https://crrev.com/0355195267df26728e5f9c2df28ac679435ad195/components/password_manager/core/browser/form_parsing/form_parser_unittest.cc

This issue should be completely fixed in Chrome 69, starting with Canary in a day or two. As explained in #12, I won't be merging it to 68.