1171189 - Custom context menu fires when closing the banner on Twitter

Status: RESOLVED FIXED

(Firefox for iOS :: General, defect)

Description

[Wesley Johnston (:wesj)]

Twitter shows a "Get the app" banner on top (no matter how many times you hide it, they keep showing it). Tapping it now always causes the context menu to appear.

Comment 1

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

This seems to be a banner that Twitter adds. Not the standard banner that Apple can add on top of a page.

You can see the difference between the two by for example going to http://m.telegraaf.nl - note the UI is a little different, it for example shows the number of stars and full app name and publisher.

Comment 2

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

Also, the 'Get the app' banner does not seem to show in the Simulator.

So if in doubt if the bar is something the website is doing or if it is the native iOS provided one, run the app in the simulator and see what happens there.


So I think for this bug we can ignore the fact that the banner keeps showing on Twitter, I assume they do it on purpose, and instead we should focus on the context menu showing on a non-long-press.

Comment 3

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

Attachment: PR: https://github.com/mozilla/firefox-ios/pull/657

The banner is a Twitter specific banner and not the built-in one that apps can ask to be shown in the `WKWebView`.

The underlying problem here is that we also try to show the context menu on script links that are handled internally by the application. This patch has a basic fix for that where I check if the link's `href` is set to `#`. I don't know the web platform well enough if this is the best check to do.

*Note that this fix is not twitter specific. This will work on any script-backed link.*

Comment 4

[Brian Nicholson (:bnicholson)]

Comment on attachment 8627804 [details] [review]
PR: https://github.com/mozilla/firefox-ios/pull/657

I think the bigger issue here is the fact that the context menu shows at all with a regular tap -- we should only be showing the context menu for long taps. What's happening is that the element is removed on touchdown, so when you release within the 500ms timeout period, touchend is never called to cancel the context menu dialog.

wesj pointed out that attaching the touchend/touchmove listeners directly to the elements (rather than having global capture on the window) will correctly fire the touchend/touchmove events, even if the element is removed from the document. So, in short, the "proper" way to fix this would be to attach the touchend/touchmove listeners to event.target directly, doing so inside of touchstart.

I created a simple page here to help with testing (which also works in the simulator): https://people.mozilla.org/~bnicholson/test/touchend.html

Comment 5

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

Thanks :bnicholson that is super useful.

Comment 6

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

Attachment: PR: https://github.com/mozilla/firefox-ios/pull/691

Comment 7

[Brian Nicholson (:bnicholson)]

Comment on attachment 8631210 [details] [review]
PR: https://github.com/mozilla/firefox-ios/pull/691

Looking good -- left a couple more comments in the PR.

Comment 8

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

Comment on attachment 8631210 [details] [review]
PR: https://github.com/mozilla/firefox-ios/pull/691

Brian, this now seems to work well on your test page and on m.twitter.com. Can you take a quick peek to see if I did anything crazy. JavaScript is not my greatest skill :-)

Comment 9

[Brian Nicholson (:bnicholson)]

Comment on attachment 8631210 [details] [review]
PR: https://github.com/mozilla/firefox-ios/pull/691

Nice, looks good to me!