Fixes to BackForwardListViewController

RESOLVED FIXED

Status

()

Firefox for iOS
Browser
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bnicholson, Assigned: bnicholson)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Playing around with our back/forward list, I've run across a few problems:
* Entries in the list are empty if the page doesn't have a title
* Whenever long pressing the back/forward buttons, the following warning appears in the console:

2015-01-30 10:44:11.324 Client[36645:1389340] Warning: Attempt to present <Client.BackForwardListViewController: 0x7fe7b2279400> on <Client.BrowserViewController: 0x7fe7b2229130> whose view is not in the window hierarchy!

This happens because UIGestureRecognizer callbacks are fired multiple times in different states (Possible, Began, Changed, Ended, etc.), and we're firing the delegate multiple times. Whenever handling a UIGestureRecognizer event, we need to check its state. I have a feeling we're doing this wrong in other places we use UIGestureRecognizer, so I'll file more bugs for any I find.
(Assignee)

Comment 1

3 years ago
Created attachment 8557246 [details] [review]
Pull request
Attachment #8557246 - Flags: review?(sarentz)
(Assignee)

Updated

3 years ago
Attachment #8557246 - Flags: review?(wjohnston)
Is the problem here that we are adding a gesturerecognizer on top of a button, which already has its own click handling?

Is that what you are seeing? That both the long press gesturerecognizer is fired, and also the normal button back/forward action on TouchUpInside?
Comment on attachment 8557246 [details] [review]
Pull request

Looks like checking the state for Began is a common way to do this when having a UILongPressGestureRcoginzier on a UIButton. Looks good.
Attachment #8557246 - Flags: review?(sarentz) → review+
(Assignee)

Comment 4

3 years ago
(In reply to Stefan Arentz [:st3fan] from comment #2)
> Is the problem here that we are adding a gesturerecognizer on top of a
> button, which already has its own click handling?
> 
> Is that what you are seeing? That both the long press gesturerecognizer is
> fired, and also the normal button back/forward action on TouchUpInside?

Nope, this isn't related to the fact that it's a button; the problem is that the gesture recognizer callback is called multiple times for multiple states. From what I can tell, a gesture recognizer callback will always be called at least twice: once for the Began state, and once for the Ended state. It can be called an indefinite number of times between those for the Changed state, too, when dragging around.
(Assignee)

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Updated

3 years ago
Attachment #8557246 - Flags: review?(wjohnston)
Blocks: 1127016
You need to log in before you can comment on or make changes to this bug.