1221972 - Incorrect display of context menu on iPads

Status: RESOLVED FIXED

(Firefox for iOS :: General, defect)

Description

[Catalin Suciu [:csuciu]]

Attachment: Screenshot

Build: Beta 1113
Device: iPad Air 2 (9.1)

1. Visit www.bbc.com
2. Tap on a link. Quickly, before the article opens, long tap on a link/image

Result: The context menu triggered by the long tap is displayed on the newly opened page in the top left corner.img

Comment 1

[Richard Newman [:rnewman]]

Let's investigate, see if this is widespread or easily reproduced. If not, we can punt it into the future.

Comment 2

[Stephan Leroux [:sleroux]]

I've been able to reproduce in the simulator if I'm really quick. What's happening is the popover controller is getting set to position (0, 0) when the long press event fires:

https://github.com/mozilla/firefox-ios/blob/master/Client/Frontend/Browser/BrowserViewController.swift#L2149

The docs say that locationInView finds out the gesture's touch point in the context of the given view:

https://developer.apple.com/library/ios/documentation/UIKit/Reference/UIGestureRecognizer_Class/index.html#//apple_ref/occ/instm/UIGestureRecognizer/locationInView:

My guess is that in this transitional state, the gestureRecognizer in the previous page is firing but the page the user is viewing is now the new page which doesn't map to the recognizer anymore. In this scenario, we probably should disable the popover if we're long pressing during a page load since when the page does finish loading, the context menu won't be pointing to the correct item even if we could find the correct position.

Comment 3

[Stephan Leroux [:sleroux]]

Attachment: Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1242

Patch for ignoring context menu when page is loading.

Comment 4

[Stephan Leroux [:sleroux]]

Not 100% sure if this is a solution that we want. I'm playing with Safari and I think it manages to transfer the long press gesture to the next page's context but I might not be fast enough in triggering it.

Comment 5

[Brian Nicholson (:bnicholson)]

Haven't tried this yet, but I assume this means we can't use the context menu until the page has completely finished loading. I wonder if that will be a problem, particularly on slower connections?

A couple other ideas that come to mind:
* Keep track of the web view state using the navigation delegates so that we can get a more fine-grained load state other than just "loading" or "not loading". We could, for example, update some state variable for didStart, didCommit, didFinish, etc., and then only show the menu if it's didCommit or later.
* Bail if the position given by locationInView is (0, 0). This will only work if the position is always (0, 0) in these cases, and it's a bit hackier since we'd be basing the fix on this indirect state, but it would be pretty straightforward (and more reliable during page load) if it worked.

Comment 6

[Stephan Leroux [:sleroux]]

I did some more investigation in this bug and found another issue:

1. Navigate to www.bbc.com and select and article link
2. Start a long press gesture right afterwards
3. Hold down long press until after page loads
4. Let go of long press.

The context menu appears again at the top left at position 0, 0 but the contents of the context menu is that of the new page and not on the originating element. This is probably being caused by how we trigger the long press events through our JS. The touch start event fires in the new page's JS and when the long press is fired it reads the new content. Unfortunately I don't see a fix for this since we're limited to using JS to handle this sort of thing. From looking at the WebKit gesture handling code, it looks pretty robust with checks to make sure the point that is touch is valid and returns the correct element info. It would be great if we can latch onto that event if possible.

Comment 7

[Brian Nicholson (:bnicholson)]

Do you think the second option in comment 5 would work? That is, bail if `gestureRecognizer.locationInView(view)` is (0, 0).

Comment 8

[Brian Nicholson (:bnicholson)]

Attachment: Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1257

Thought I'd give this approach a try, and it seems to work well enough.

Comment 9

[Stephan Leroux [:sleroux]]

Comment on attachment 8686833 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1257

Works for me. Good find though on the touchstart firing twice.

Comment 10

[Brian Nicholson (:bnicholson)]

https://github.com/mozilla/firefox-ios/commit/1293dcaa1d31add3c716e8e656186af46605693f