1153285 - Put "Open In New Tab" back as a context menu item

Status: RESOLVED FIXED

(Firefox for iOS :: General, defect)

Description

[Aaron Train [:aaronmt]]

We should match Firefox for Android behaviour here, long tap on links, 'Open in New Tab' addTab() should open them in the background

Comment 1

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

After looking at this for a while, mostly reading WebKit code, I don't think there is a simple solution for this that will not involve private APIs and patching UIKit/WebKit methods.

I think we should leave this menu as it is now and file a bigger bug for post v1 to re-implement it in our own code, fixing all the regressions that we encountered when we previously tried.

I am marking this as tracking-? so that we can discuss this during triage.

Comment 2

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

More technical:

Using the Open action from the action sheet that appears when you long press on a link results in the  - webView:decidePolicyForNavigationAction:decisionHandler: being called.

But, from the WKNavigationAction we cannot see *how* that link was opened. We don't know if it was a regular tap on a link, or if the user went through the action sheet. We miss that context.

We can *maybe* hack around that by patching UIAlertView or WKWebView and inject our own code, but I think that is fragile and probably 'private API usage' in App Store terms.

Comment 3

[Aaron Train [:aaronmt]]

This hurts, but let's discuss.

Comment 4

[Wesley Johnston (:wesj)]

st3fan, just to be clear what's the NavigationType for the decidePolicyForNavigation [1]? I would expect its Other, but your message makes it sound like its actually LinkActivated?

https://developer.apple.com/library/ios/documentation/WebKit/Reference/WKNavigationAction_Ref/index.html#//apple_ref/swift/enum/WKNavigationType

Comment 5

[Wesley Johnston (:wesj)]

Still investigating, but we'll track it.

Comment 6

[Brian Nicholson (:bnicholson)]

I'll take a look at this.

Comment 7

[Brian Nicholson (:bnicholson)]

Well, bad news: user content scripts are not injected into iframes, so we'll still have bug 1127606. It looks like we can't do much better than the first implementation we tried in bug 1109666.

Comment 8

[Brian Nicholson (:bnicholson)]

Not sure how I missed it, but Stefan pointed me to initWithSource:injectionTime:forMainFrameOnly:, which is exactly what I was looking for.

Comment 9

[Brian Nicholson (:bnicholson)]

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

Comment 10

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

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

I tested a bunch. Works great! Just some questions in the PR comments.

Comment 11

[Brian Nicholson (:bnicholson)]

Going to look into element highlighting/Open In Background as follow-ups.

https://github.com/mozilla/firefox-ios/commit/ab9b821104a4b32f067f7d4b8ae6d22f0c54e0b1