766275 - Default context menu for webapps

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect, P1)

Description

[Wesley Johnston (:wesj)]

We currently show the same context menu options as Fennec for Webapps. We should show remove some unnecessary options. Right now we have

"Open in New tab"
"Share link"
"Bookmark link"
"Full screen" - video only
"Save Image"
"Copy"
"Copy All"
"Paste"
"Select All"
"Change Input Method"
"Add Search Engine"

Of those, I think only "Full Screen" and the Select/Copy/Paste/Input Method options should appear by default? Share I'm not sure about. OT, but for anyone who's reading along, context menu API for pages is bug 617528 which I blocked on prompt service tests (bug 757481) (i.e. we could move it forward faster if I gave up on those).

Comment 1

[Jason Smith [:jsmith]]

k9o nomination - we need provide context that we are in an app, not a tabbed browser (and shouldn't show UI for tabbed browsing as a result).

Comment 2

[Asa Dotzler [:asa]]

we need to figure out how far to take this. will we still have default items or leave it up to the app to implement all context menus.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch

This moves the context menu item creation to a new function in BrowserApp, since its sorta a set of Browser functions. I'm killing:

openInNewTab, shareLink, bookmarkLink, fullScreen (video), shareImage, saveImage, and addSearchEngine

We'll still have copy, paste, select, select input method, and openinHelperApps menu items (shown on the same elements they are in Fennec). I'm also killing off initializing search engines for webapps here because we don't need it.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 644050 [details] [diff] [review]
Patch

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+  addBrowserContextMenuItems: function ba_addBrowserContextMenuItems() {

Just a naming nit: Since we are in BrowserApp, let's just name this initContextMenu()

Comment 5

[Jason Smith [:jsmith]]

(In reply to Wesley Johnston (:wesj) from comment #3)
> Created attachment 644050 [details] [diff] [review]
> Patch
> 
> This moves the context menu item creation to a new function in BrowserApp,
> since its sorta a set of Browser functions. I'm killing:
> 
> openInNewTab, shareLink, bookmarkLink, fullScreen (video), shareImage,
> saveImage, and addSearchEngine

Hmm. Should we keep fullscreen video though? Desktop web apps I know supports this.

Comment 6

[Wesley Johnston (:wesj)]

I feel like apps have a fullscreen api and can provide that themselves... pushed as is, but leave a note if we need to fix the video bit.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1cbf95c4c4f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e09c52e2c73

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1cbf95c4c4f1
https://hg.mozilla.org/mozilla-central/rev/6e09c52e2c73

Comment 8

[Aaron Train [:aaronmt]]

Aurora nomination?

Comment 9

[Wesley Johnston (:wesj)]

Comment on attachment 644050 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New webapps stuff
User impact if declined: Context menu shows things like "Open in New Tab" that shouldn't be there for webapps
Testing completed (on m-c, etc.): Has been on mc for a few weeks.
Risk to taking this patch (and alternatives if risky): Low risk. Webapps only. Probably holds up other context menu patches from 16.
String or UUID changes made by this patch: None. Just moving code around.

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

Do we need to take this or can it just ride the trains - afaik webapps aren't being enabled in 16.

Comment 11

[Jason Smith [:jsmith]]

(In reply to Lukas Blakk [:lsblakk] from comment #10)
> Do we need to take this or can it just ride the trains - afaik webapps
> aren't being enabled in 16.

I thought web apps were being enabled for 16? I haven't heard plans to not do this yet.

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 644050 [details] [diff] [review]
Patch

Sorry, I forgot about my discussion with Wesley yesterday -- we're going to be trying to get more MTD users on Aurora to test this and since there are no string changes, approving for Aurora.

Comment 13

[Alex Keybl [:akeybl]]

Comment on attachment 644050 [details] [diff] [review]
Patch

Never landed on Aurora 16, and doesn't need to land for Beta 16 (web apps are disabled).

Comment 14

[Alex Keybl [:akeybl]]

Rather, the web apps menu item is disabled. If this is still necessary, please request beta approval.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Alex Keybl [:akeybl] from comment #14)
> Rather, the web apps menu item is disabled. If this is still necessary,
> please request beta approval.

Not needed for Fx16. WebApps are really disabled on Fx16.