Created attachment 621817 [details] [diff] [review] 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!
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+
Created attachment 622802 [details] [diff] [review] 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.
Attachment #622802 - Flags: review?(bugmail.mozilla) → review+
Target Milestone: --- → Firefox 15
Status: NEW → RESOLVED
Last Resolved: 7 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?
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+
status-firefox14: --- → fixed
You need to log in before you can comment on or make changes to this bug.