Closed Bug 877782 Opened 7 years ago Closed 7 years ago

Reposition form autocomplete popup if dynamic toolbar changes visibility


(Firefox for Android :: General, defect)

Not set



Firefox 24
Tracking Status
firefox23 --- fixed
firefox24 --- verified
fennec 23+ ---


(Reporter: Margaret, Assigned: Margaret)




(1 file, 1 obsolete file)

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.
tracking-fennec: ? → 23+
Assignee: nobody → margaret.leibovic
Attached patch patch (obsolete) — Splinter Review
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).
Attachment #764236 - Flags: review?(bugmail.mozilla)
Attached patch patchSplinter Review
Whoops, wrong version.
Attachment #764236 - Attachment is obsolete: true
Attachment #764236 - Flags: review?(bugmail.mozilla)
Attachment #764239 - Flags: review?(bugmail.mozilla)
Comment on attachment 764239 [details] [diff] [review]

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/
@@ +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.
Attachment #764239 - Flags: review?(bugmail.mozilla) → review+
Depends on: 884851
Comment on attachment 764239 [details] [diff] [review]

[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
Attachment #764239 - Flags: approval-mozilla-aurora?
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Attachment #764239 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
verified on:
Galaxy Nexus - Android 4.3 - Firefox 24.0b2
You need to log in before you can comment on or make changes to this bug.