877782 - Reposition form autocomplete popup if dynamic toolbar changes visibility

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

Follow-up to bug 867193. In that bug, Chris says:

we can listen to the metrics changing with onMetricsChanged (I think that's it - check GeckoLayerClient and BrowserApp, it listens for margin changes to move the toolbar)

We should track this for 23.

Comment 1

[:Margaret Leibovic]

Attachment: patch

I had to move around some of the logic to be able to just update the position of the popup without getting a new message from Java. This works, but there may be a more elegant way to do this (this code is kinda crufty to begin with).

Comment 2

[:Margaret Leibovic]

Attachment: patch

Whoops, wrong version.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 764239 [details] [diff] [review]
patch

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

Looks good! It would be nice if we turned the mIsAutoComplete bool into an enum instead, based on the way it's used. It might make a good first bug for a contributor.

::: mobile/android/base/FormAssistPopup.java
@@ +49,5 @@
>  
> +    private double mX = 0;
> +    private double mY = 0;
> +    private double mW = 0;
> +    private double mH = 0;

No need to initialize these to 0, that will happen automatically.

@@ +213,5 @@
> +    }
> +
> +    private void positionAndShowPopup() {
> +        ImmutableViewportMetrics metrics = GeckoAppShell.getLayerView().getViewportMetrics();
> +        positionAndShowPopup(metrics);

I would not bother with the temp variable here, but it's up to you.

@@ +221,4 @@
>          // Don't show the form assist popup when using fullscreen VKB
>          InputMethodManager imm =
>                  (InputMethodManager) GeckoAppShell.getContext().getSystemService(Context.INPUT_METHOD_SERVICE);
>          if (imm.isFullscreenMode())

It would be good to add a ThreadUtils.assertOnUiThread() call at the top of this function somewhere.

Comment 4

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/31ef25bb8b28

I filed bug 884851 as a follow-up.

Comment 5

[:Margaret Leibovic]

Comment on attachment 764239 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): dynamic toolbar
User impact if declined: form assist popup will be positioned incorrectly if the toolbar is shown/hidden while the popup is visible
Testing completed (on m-c, etc.): locally, just landed on inbound
Risk to taking this patch (and alternatives if risky): low-risk, reorganizes some logic, but changes are limited to the form assist popup
String or IDL/UUID changes made by this patch: n/a

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/31ef25bb8b28

Comment 7

[:Margaret Leibovic]

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

Comment 8

[Ioana Chiorean]

verified on:
Galaxy Nexus - Android 4.3 - Firefox 24.0b2