1532582 - Autofill: Popup not aligned with input field

Status: RESOLVED FIXED

(GeckoView :: IME, defect, P1)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

Reference Browser issue: https://github.com/mozilla-mobile/reference-browser/issues/636
Example screenshot: https://i.imgur.com/Iz3ZGEI.png

STR:

• Enable an autofill service with login data saved
• Go to Twitter.com in the reference browser
• Click on "Login"

Expected:
Autofill popup is shown next to the input field

Actual:
Autofill popup is shown at the bottom of the screen.

Comment 1

[Makoto Kato [:m_kato]]

Actually, we use dummyRect when calling notifyViewEntered. So I guess that it will be fixed when we set focused element's rect.

Comment 2

[Chris Peterson [:cpeterson]]

Emily said she would take a look at this bug. [geckoview:fenix:m3] because autofill is important for Fenix MVP.

Comment 3

[Emily Toop (:fluffyemily)]

Attachment: Bug 1532582 - Display autofill popup in correct location. r=#geckoview-reviewers

This autofill popover was being displayed in the incorrect place because the display rect we were proving to the AutofillManager was the rect for the GeckoView and not the rect for the HTML element that the autofill popover was relating to.

1. Add view dimensions to info passed to autofill in GeckoViewAutoFill.
2. Use those view dimensions to calculate the correct location on the screen using pageToScreenMatrix in GeckoSession.

The resulting locations were incorrect, as the values used by pageToScreenMatrix were out of date. The GeckoSession was only notified about updated metrics during first composite, which meant that when the metrics changed during zoom and scroll on soft keyboard presentation, GeckoSession was unaware of it.

3. Update GeckoSession with new screen metrics when they change and not only during first composite.

Despite this change ensuring that GeckoSession always had the correct values for the viewport size and location, the request to provide the autofill location was made before the zoom and scroll was complete, meaning that even then out of date values were used during the calculation. The intial solution was to fire an event once zoom was complete, but despite this event being fired after the new screen size had been calculcated in AsyncCompositionManager, GeckoSession did not receive the values until after the event had been processed (the calls were out by 0.024ms).

During the course of the above solution it became clear that the mSoftInputReentrancyGuard was failing to ensure that GeckoView:ZoomToInput and showSoftInput were called only once. When I removed the zoom complete notification in favour of (5.) I left the fix for mSoftInputReentrancyGuard in.

4. Ensure that GeckoView:ZoomToInput and showSoftInput are called only once when an HTML field is entered.
5. Fire a new event when screen metrics are updated and listen to this event from inside SessionTextInput. Call AutofillManager#notifyViewEntered only once this event has been recieved.

This was not my preferred solution to this, but timing issues meant I could not find/think of an alternative way of delaying the calculation of the autofill popover location until after GeckoSession had been updated.

This patch currently only fixes things on GV apps. It is still broken on Fennec.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b6f5942c42bb
Display autofill popup in correct location. r=geckoview-reviewers,snorp,kats

Comment 5

[Dorel Luca [:dluca]]

Backed out changeset b6f5942c42bb (bug 1532582) for Eslint failure. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=236907041&repo=autoland&lineNumber=280

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=236901055&revision=b6f5942c42bb26498c6b11778fceb281d0eb1a46

Backout:
https://hg.mozilla.org/integration/autoland/rev/55398270c9228538069d84ef478f2b02d0c37735

Comment 6

[Emily Toop (:fluffyemily)]

Attachment: Bug 1532582 - Display autofill popup in correct location. r=#geckoview-reviewers

This autofill popover was being displayed in the incorrect place because the display rect we were providing to the AutofillManager was the rect for the GeckoView and not the rect for the HTML element that the autofill popover was relating to.

1. Add view dimensions to info passed to autofill in GeckoViewAutoFill.
2. Use those view dimensions to calculate the correct location on the screen using pageToScreenMatrix in GeckoSession.

The resulting locations were incorrect, as the values used by pageToScreenMatrix were out of date. The GeckoSession was only notified about updated metrics during first composite, which meant that when the metrics changed during zoom and scroll on soft keyboard presentation, GeckoSession was unaware of it.

3. Update GeckoSession with new screen metrics when they change and not only during first composite.

Despite this change ensuring that GeckoSession always had the correct values for the viewport size and location, the request to provide the autofill location was made before the zoom and scroll was complete, meaning that even then out of date values were used during the calculation. The intial solution was to fire an event once zoom was complete, but despite this event being fired after the new screen size had been calculcated in AsyncCompositionManager, GeckoSession did not receive the values until after the event had been processed (the calls were out by 0.024ms).

5. Call new method onScreenMetricsUpdated inside SessionTextInput after screen metrics have been updated. Call AutofillManager#notifyViewEntered from this function.

This was not my preferred solution to this, but timing issues meant I could not find/think of an alternative way of delaying the calculation of the autofill popover location until after GeckoSession had been updated.

This patch currently fixes things on GV apps. Occasionally, on Fennec, the autofill view is out of alignment slightly. This needs further work.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7400f023dec1
Display autofill popup in correct location.

Comment 8

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/7400f023dec1

Comment 9

[Chris Peterson [:cpeterson]]

67=fix-optional. Neither Fenix MVP nor Firefox for Fire TV will use GeckoView 67, so we don't need to uplift this fix to 67 Beta unless you want it in Fennec 67.

Comment 10

[Chris Peterson [:cpeterson]]

Moving some input bugs to the new GeckoView::IME component.