Closed Bug 752759 Opened 10 years ago Closed 10 years ago

Get rid of excessive mFormAssistPopup.hide() calls in PanZoomController

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox14+ fixed)

RESOLVED FIXED
Firefox 15
Tracking Status
firefox14 + fixed

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
I'm not quite sure of the reasoning behind all the mFormAssistPopup.hide() calls we put in PanZoomController, but I'd guess there wasn't a great plan behind it. I believe the only reason we need to be messing around in here at all is that we had to use a RelativeLayout instead of a PopupWindow for this popup in order to position it properly, but that made us lose out on the "click outside to dismiss" functionality of PopupWindow. Really, we just want to be hiding FormAssistPopup whenever the user touches outside it, so we can just do that in onTouchStart, instead of checking for a slew of different cases.

Wes, if you know of a way for us to listen for this, we could just do it in FormAssistPopup itself, and we'd be decoupled from PanZoomController!
Attachment #621817 - Flags: review?(wjohnston)
Comment on attachment 621817 [details] [diff] [review]
patch

Wes is on vacation - Kats, can you review this?

See also my comment asking if there's a way to just listen for onTouchStart from FormAssistPopup.
Attachment #621817 - Flags: review?(wjohnston) → review?(bugmail.mozilla)
Comment on attachment 621817 [details] [diff] [review]
patch

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

The patch looks ok to me in that it improves the current code, but I think there's a situation that's not handled properly.

If you touch down on a page and the content calls preventDefault on the touchstart event, PanZoomController will not get the event at all. To protect against this it makes sense to move the hide() call into LayerView.java's onTouchEvent() call. If you want to do it more modularly you could try registering a View.OnTouchListener on the view, but I'm not sure what order the touch event gets delivered to listeners/views in so you'd have to make sure that both dismissing the autocomplete and normal panning work properly.
Attachment #621817 - Flags: review?(bugmail.mozilla) → review+
Attached patch patvh v2Splinter Review
This patch moves the hide() call to LayerView as you suggested. I added a null check because mFormAssistPopup gets initialized after mLayerController, but I'm not sure how much of a real threat this would be.
Attachment #621817 - Attachment is obsolete: true
Attachment #622802 - Flags: review?(bugmail.mozilla)
Attachment #622802 - Flags: review?(bugmail.mozilla) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/4da3ac9ee729
Target Milestone: --- → Firefox 15
https://hg.mozilla.org/mozilla-central/rev/4da3ac9ee729
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment on attachment 622802 [details] [diff] [review]
patvh v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): this patch itself is just a cleanup patch, but for some reason uplifting bug 765407 without it caused bug 769566
User impact if declined: see bug 769566
Testing completed (on m-c, etc.): landed on m-c almost 2 months ago
Risk to taking this patch (and alternatives if risky): low-risk cleanup patch
String or UUID changes made by this patch: n/a
Attachment #622802 - Flags: approval-mozilla-beta?
Blocks: 769566
Comment on attachment 622802 [details] [diff] [review]
patvh v2

[Triage Comment]
Approved in support of .N+ blocker bug 769566.
Attachment #622802 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Duplicate of this bug: 769566
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.