Default context menu for webapps

VERIFIED FIXED in Firefox 17

Status

()

Firefox for Android
Web Apps
P1
normal
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: wesj, Assigned: wesj)

Tracking

Trunk
Firefox 17
Points:
---

Firefox Tracking Flags

(blocking-kilimanjaro:+, firefox16 affected, firefox17 verified)

Details

(Whiteboard: [blocking-webrtandroid1+])

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
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).
(Assignee)

Updated

6 years ago
Blocks: 766259
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).
blocking-kilimanjaro: --- → ?

Comment 2

6 years ago
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.
blocking-kilimanjaro: ? → +

Updated

6 years ago
No longer blocks: 766259

Updated

6 years ago
Blocks: 766259
Priority: -- → P1

Updated

6 years ago
QA Contact: aaron.train
(Assignee)

Comment 3

6 years ago
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

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.
Assignee: nobody → wjohnston
Attachment #644050 - Flags: review?(mark.finkle)
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()
Attachment #644050 - Flags: review?(mark.finkle) → review+
(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.
(Assignee)

Comment 6

6 years ago
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
https://hg.mozilla.org/mozilla-central/rev/1cbf95c4c4f1
https://hg.mozilla.org/mozilla-central/rev/6e09c52e2c73
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17

Updated

6 years ago
Whiteboard: [qa+]

Updated

6 years ago
status-firefox16: --- → affected

Updated

6 years ago
Whiteboard: [qa+] → [qa+], [blocking-webrtandroid1+]
Aurora nomination?
Status: RESOLVED → VERIFIED
status-firefox17: --- → verified

Updated

6 years ago
Whiteboard: [qa+], [blocking-webrtandroid1+] → [blocking-webrtandroid1+]
(Assignee)

Comment 9

6 years ago
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.
Attachment #644050 - Flags: approval-mozilla-aurora?
Do we need to take this or can it just ride the trains - afaik webapps aren't being enabled in 16.
(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 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.
Attachment #644050 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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).
Attachment #644050 - Flags: approval-mozilla-aurora+ → approval-mozilla-aurora-
Rather, the web apps menu item is disabled. If this is still necessary, please request beta approval.
(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.
You need to log in before you can comment on or make changes to this bug.