Closed Bug 877782 Opened 7 years ago Closed 7 years ago

Reposition form autocomplete popup if dynamic toolbar changes visibility

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

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

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

Attachments

(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]
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.
Attachment #764239 - Flags: review?(bugmail.mozilla) → review+
Depends on: 884851
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
Attachment #764239 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/31ef25bb8b28
Status: NEW → RESOLVED
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.