1890194 - Putting in a suggested login deletes the other logins from the list

Status: NEW

(Firefox for Android :: Autofill, defect, P2)

Description

[eclaudiu64]

User Agent: Mozilla/5.0 (Android 11; Mobile; rv:125.0) Gecko/125.0 Firefox/125.0

Steps to reproduce:

Several logins are saved, we select one of the suggested logins and look again in the list, the other logins are deleted from the list.

Actual results:

The other suggested login data disappears.

Expected results:

All logins should remain after we select one of them.

Comment 1

[eclaudiu64]

Attachment: mobizen_20240407_192100.mp4

Reproducing the error.

Comment 2

[eclaudiu64]

can you take a look ?

Comment 3

[Chris Peterson [:cpeterson]]

Setting UX Fundamentals bugs to priority P2 because we want to fix them this year.

Comment 4

[Chris Peterson [:cpeterson]]

Alexandra, are you still working on this bug? This bug is on our UX Fundamentals bug list and it's assigned to you. If you're not working on it, we can unassign it so another squad 1 engineer might pick it up.

Comment 5

[Alexandra Virvara]

The login list is received together with the request for the SavedLogins prompt. This is sent from GV whenever the username field gets focus . Android is not aware of what the pages' username fields contain, it just displays the data that is being sent. I tried to find a workaround and keep the full list of saved logins, but it complicates the code with unnecessary logic and I think that this problem should be best handled on the GV side.

Comment 6

[Dimi Lee [:dimi]]

(In reply to Alexandra Virvara from comment #5)

The login list is received together with the request for the SavedLogins prompt. This is sent from GV whenever the username field gets focus . Android is not aware of what the pages' username fields contain, it just displays the data that is being sent. I tried to find a workaround and keep the full list of saved logins, but it complicates the code with unnecessary logic and I think that this problem should be best handled on the GV side.

Hi, I'd like to confirm the following behaviors for Fenix to better understand what adjustments Gecko might need:

1. When users select one of the entries, should we display an empty list or a full list?
2. When users select an entry and then change the content of the <input>, should we display only logins that match the updated content, an empty list or all the logins?
3. When users select an entry and then clear the text in the input, should we immediately display all the logins?
   Thanks!

Comment 7

[Segun Famisa [:segun]]

Thanks for the comment. Having looked at the related code on Fenix, I can take a stab at answering. I'm still quite new to the project, so I will still invite the more familiar folks to respond.

The overarching theme is that - I think we should match what is in the input field, but to answer:

1. When users select one of the entries, should we display an empty list or a full list?

I think we should show the matching entry (which will typically be the selected item). As is done on desktop.

2. When users select an entry and then change the content of the <input>, should we display only logins that match the updated content, an empty list or all the logins?

I would say yes. We should display the logins that match the entered content at all times - as the user changes the content of the <input>. This is also the behavior on desktop.

3. When users select an entry and then clear the text in the input, should we immediately display all the logins?

In Fenix code, it looks like we dismiss the current prompt every time we receive a new select login prompt. So visually to the user, it will be collapsed, but when they expand it, I believe that they should see all the logins.

Comment 8

[Dimi Lee [:dimi]]

Hi Segun, thank you for the answers, that is helpful!
Could you also share with me what the current result is in Fenix for the those scenarios? thanks!

Comment 9

[Segun Famisa [:segun]]

Attachment: scenario 1

Comment 10

[Segun Famisa [:segun]]

Attachment: scenario 2

Comment 11

[Segun Famisa [:segun]]

Attachment: scenario 3

Comment 12

[Segun Famisa [:segun]]

Hey :dimi, thank you.

I have attached 3 recordings to the bug that demonstrate the current behavior in those different scenarios.

Please let me know if it properly shows what you are asking.

Thank you.

Comment 13

[Dimi Lee [:dimi]]

(In reply to Segun Famisa [:segun] from comment #12)

Hey :dimi, thank you.

I have attached 3 recordings to the bug that demonstrate the current behavior in those different scenarios.

Thanks, that is very helpful!
The problem is that right now we only send the list when users focus on the input field.
What we can do from the gecko side is sending an the updated list whenever users change the content of the <input>. However, I'll need your help to design the API in GeckoViewAutocomplete.sys.mjs so we can call it when the list is changed. Does that sound good to you?

Comment 14

[Segun Famisa [:segun]]

(In reply to Dimi Lee [:dimi] from comment #13)

Thanks, that is very helpful!
The problem is that right now we only send the list when users focus on the input field.
What we can do from the gecko side is sending an the updated list whenever users change the content of the <input>. However, I'll need your help to design the API in GeckoViewAutocomplete.sys.mjs so we can call it when the list is changed. Does that sound good to you?

Hey :dimi,
Sure, happy to help with the design.

so we can call it when the list is changed

Quick question about this part. Are we creating an API to be called when the login list changes, or when the content of the <input> changes?

Comment 15

[Dimi Lee [:dimi]]

(In reply to Segun Famisa [:segun] from comment #14)

so we can call it when the list is changed

Quick question about this part. Are we creating an API to be called when the login list changes, or when the content of the <input> changes?

How about starting with calling the API when the content of the <input> is changed? If we feel we do need to optimize to only send the notification when list changed, we can update accordingly.

Comment 16

[Segun Famisa [:segun]]

(In reply to Dimi Lee [:dimi] from comment #15)

How about starting with calling the API when the content of the <input> is changed? If we feel we do need to optimize to only send the notification when list changed, we can update accordingly.

Sounds good. So how do we do this?

I'm not too sure, but it seems to me that in AutoCompleteParent.sys.mjs, we already do this when we receive the AutoComplete:MaybeOpenPopup event. We then call GeckoViewAutoComplete.delegateSelection(), but we do an early return because we have implemented it to only show one prompt at a time with this check. And that's why the login list does not get updated until the user focuses out of the input field (essentially dismissing the login prompt) and then re-focus.

Comment 17

[Dimi Lee [:dimi]]

Attachment: WIP: Bug 1890194 - Putting in a suggested login deletes the other logins from the list

Comment 18

[Dimi Lee [:dimi]]

(In reply to Segun Famisa [:segun] from comment #16)

I'm not too sure, but it seems to me that in AutoCompleteParent.sys.mjs, we already do this when we receive the AutoComplete:MaybeOpenPopup event. We then call GeckoViewAutoComplete.delegateSelection(), but we do an early return because we have implemented it to only show one prompt at a time with this check. And that's why the login list does not get updated until the user focuses out of the input field (essentially dismissing the login prompt) and then re-focus.

Hi Segun, i just uploaded a WIP patch which shows how gecko would notify. The delegateInvalidate function in the WIP patch will be called whenever the content of <input> is changed. Let me know if you have any question.

Comment 19

[Segun Famisa [:segun]]

(In reply to Dimi Lee [:dimi] from comment #18)

Hi Segun, i just uploaded a WIP patch which shows how gecko would notify. The delegateInvalidate function in the WIP patch will be called whenever the content of <input> is changed. Let me know if you have any question.

Hi :dimi

Thanks for sharing that.

1. I am assuming that the TODO will be then updated to send the updated login list correct?
2. Is delegateInvalidate() going to be called only on "login" types of <input> or every kind? I'm asking because then we will need to ensure that we only send an updated login list when the input type is a login field, right? (Just thinking out loud here - my Gecko knowledge is still pretty limited :) )

Thank you.

Comment 20

[Jeff Boek [:boek]]

Hey :dimi,

Taking a look at this it looks like we're just bailing out after a prompt is already visible. But after looking at the prompt code it looks like there was an ability to update the prompt added at some point that we never used here. This will then dispatch a GeckoView:Prompt:Update which then gets handled in GeckoView.

I haven't taken a look at how desktop handles this flow yet which we will investigate a little tomorrow. But it looks like this may just work if we can wire up this update flow?

Comment 21

[Jeff Boek [:boek]]

Attachment: WIP: Bug 1890194 - WIP: Wires up prompt update flow

Comment 22

[Jeff Boek [:boek]]

:dimi,

We did a proof of concept wiring up that update flow and everything seemed to work. I will reach out to the GeckoView guild and see what it will take to get this into a sihppable state.

Comment 23

[[:owlish] 🦉 PST]

Hey Dimi, looking into your proof of concept patch - I have a question.

Will the AutoComplete:Invalidate message get sent on Android? I see it's being sent from here https://searchfox.org/mozilla-central/rev/944ab16b6e8bcd8da2384a73f189265f44d7fc70/toolkit/actors/AutoCompleteChild.sys.mjs#136-141 IIRC - at least this was true when I last worked on autofill 2 years ago - this._popupOpen is never changed for Android. Desktop renders the popup itself, and has a way of tracking that easily. We delegate rendering of the popup to Android OS, and so Gecko doesn't know if it's open.

Comment 24

[Dimi Lee [:dimi]]

Hi Owlish, Jeff, Segun,
I'm really sorry for the super late reply! I was distracted by something else and forgot to reply your questions :(

(In reply to [:owlish] 🦉 PST from comment #23)

Hey Dimi, looking into your proof of concept patch - I have a question.

Will the AutoComplete:Invalidate message get sent on Android? I see it's being sent from here https://searchfox.org/mozilla-central/rev/944ab16b6e8bcd8da2384a73f189265f44d7fc70/toolkit/actors/AutoCompleteChild.sys.mjs#136-141 IIRC - at least this was true when I last worked on autofill 2 years ago - this._popupOpen is never changed for Android. Desktop renders the popup itself, and has a way of tracking that easily. We delegate rendering of the popup to Android OS, and so Gecko doesn't know if it's open.

You are right. this._popupOpen will be false for android. We will also need to add DELEGATE_AUTOCOMPLETE like this to send the 'Invalidate` message to android. I'll update my WIP.

Thanks for sharing that.

1. I am assuming that the TODO will be then updated to send the updated login list correct?
   yes
2. Is delegateInvalidate() going to be called only on "login" types of <input> or every kind? I'm asking because then we will need to ensure that we only send an updated login list when the input type is a login field, right? (Just thinking out loud here - my Gecko knowledge is still pretty limited :) )

This is a great question. I think it’s possible that Gecko calls delegateInvalidate for fields that Android doesn’t care about since Gecko doesn’t maintain state for what is being displayed on Android. I’ll include the inputElementIdentifier, as we do now when opening the popup, so GeckoView can use it to identify the relevant input and react accordingly. Does that make sense?

Comment 25

[Dimi Lee [:dimi]]

(In reply to Jeff Boek [:boek] from comment #22)

:dimi,

We did a proof of concept wiring up that update flow and everything seemed to work. I will reach out to the GeckoView guild and see what it will take to get this into a sihppable state.

Ah, I see. I’m not sure if my understanding is correct - do you mean we don’t need the WIP patch I uploaded because Gecko already calls lazy.GeckoViewAutocomplete.delegateSelection whenever the content of <input> is changed? I didn’t test this, but I think it’s possible because Gecko doesn’t maintain state for Android’s popup, so it assumes the popup is never displayed and keeps sending the AutoComplete:MaybeOpenPopup message.

Comment 26

[Jeff Boek [:boek]]

going to drop this to S3 as there is an easy work around for users.