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

RESOLVED FIXED in Firefox 14

Status

()

RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Margaret, Assigned: Margaret)

Tracking

Trunk
Firefox 15
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox14+ fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

7 years ago
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)
(Assignee)

Comment 1

7 years ago
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+
(Assignee)

Comment 3

7 years ago
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 #621817 - Attachment is obsolete: true
Attachment #622802 - Flags: review?(bugmail.mozilla)
Attachment #622802 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 4

7 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4da3ac9ee729
Target Milestone: --- → Firefox 15
https://hg.mozilla.org/mozilla-central/rev/4da3ac9ee729
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Comment 6

7 years ago
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?
(Assignee)

Updated

7 years ago
Blocks: 769566

Comment 7

7 years ago
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+
(Assignee)

Comment 8

7 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/6ce3b0a5b41e
status-firefox14: --- → fixed
(Assignee)

Updated

6 years ago
Duplicate of this bug: 769566

Updated

6 years ago
tracking-firefox14: --- → +
You need to log in before you can comment on or make changes to this bug.