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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

Attachment: patch

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!

Comment 1

[:Margaret Leibovic]

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.

Comment 2

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

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.

Comment 3

[:Margaret Leibovic]

Attachment: patvh v2

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.

Comment 4

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4da3ac9ee729

Comment 5

[Joe Drew (not getting mail)]

https://hg.mozilla.org/mozilla-central/rev/4da3ac9ee729

Comment 6

[:Margaret Leibovic]

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

Comment 7

[Alex Keybl [:akeybl]]

Comment on attachment 622802 [details] [diff] [review]
patvh v2

[Triage Comment]
Approved in support of .N+ blocker bug 769566.

Comment 8

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-beta/rev/6ce3b0a5b41e