Closed Bug 1365868 Opened 7 years ago Closed 7 years ago

Support minimal context menu functionality in GeckoView-based custom tabs

Categories

(GeckoView :: General, enhancement, P1)

All
Android
enhancement

Tracking

(firefox57 verified)

VERIFIED FIXED
mozilla57
Tracking Status
firefox57 --- verified

People

(Reporter: JanH, Assigned: cnevinchen)

References

Details

(Whiteboard: [FNC][SPT57.3][INT])

Attachments

(1 file)

Things that work at the moment and won't work automatically after switching to GeckoView:
- context menus
- context menu entries added by add-ons
- "Open Link in New (Private) Tab" opens a new tab in Firefox and shows the usual snackbar - pressing the "Switch" button on that then switches to our normal BrowserApp-based UI to show that tab
- "Parent tab" functionality: Pressing the back button on a normal tab that was opened from a custom tab/web app closes that tab again and returns to the original custom tab/web app activity. For completeness, at the moment closing such a tab from the tabs tray also returns to the original activity, but we might want to rethink that particular bit of behaviour (compare bug 1363049).
(In reply to Jan Henning [:JanH] from comment #0)
> - "Parent tab" functionality: Pressing the back button on a normal tab that
> was opened from a custom tab/web app closes that tab again and returns to
> the original custom tab/web app activity. For completeness, at the moment
> closing such a tab from the tabs tray also returns to the original activity,
> but we might want to rethink that particular bit of behaviour (compare bug
> 1363049).

Current state of things after bug 1363049 is now that we return to a web app/custom tab "parent tab" only when using the back button.
Blocks: 1285858
Depends on: 1381983
Component: General → GeckoView
We now have a context menu API in GeckoView. We can't hook this up to the Fennec context menu code, however, because that's all implemented in JS. I think we need someone to do it using regular 'ol Java. Nevin, can you guys do this?
Flags: needinfo?(cnevinchen)
Hi Joe, Wesly
Please help prioritize this.

Hi snorp
Do you know where I can find the GeckoView context menu API?


Thanks!
Flags: needinfo?(wehuang)
Flags: needinfo?(snorp)
Flags: needinfo?(jcheng)
Flags: needinfo?(cnevinchen)
Unless Joe has other say, I think we have reserved some time in this sprint for potential photon/Leanplum QA-bug, so we should be able to prioritize this after you complete your MVP and the 2 important BL items.
Flags: needinfo?(wehuang)
Below are data I receive in the callback. I'll need to check if below information is enough for showing the corresponding context menus.
 
Long Click on Image:
view = {GeckoView@5813} "org.mozilla.gecko.GeckoView{d32c57a VFE...... .F...... 0,147-1080,1731 #7f0f00c4 app:id/gecko_view}"
screenX = 158
screenY = 93
uri = null
elementSrc = "https://www.google.com.tw/images/branding/searchlogo/1x/googlelogo_tablet_tier1_hp_color_183x64dp.png"

Long Click on Youtube:
view = ......
screenX = 115
screenY = 205
uri = "https://m.youtube.com/watch?v=h4s0llOpKrU"
elementSrc = null

Long Click on URL Link:
view = .....
screenX = 118
screenY = 274
uri = "http://m.dior.com/zh_tw/minisite/th/miss_dior.html?utm_campaign=missdior_tw_sep17&utm_content=hp_masthead_displaymobile&utm_medium=display&utm_source=youtube"
elementSrc = null
Below is what I got at PromptServie[1]
We need to port JS implementation[2]~[3] and [4]~[5] to Java.
Moving from browser.js to Java is a great approach but I can't complete this in my current time frame. 

D: handleMessage() called with: geckoBundle = [{showAsActions={uri=http://www.rapidtables.com/web/html/mailto.htm#how, title=How to create mailto link in HTML?}, id=6, icon=drawable://ic_menu_share, label=Share Link}]
D: handleMessage() called with: geckoBundle = [{id=1, label=Open Link in New Tab}]
D: handleMessage() called with: geckoBundle = [{id=2, label=Open Link in Private Tab}]
D: handleMessage() called with: geckoBundle = [{id=3, label=Copy Link}]
D: handleMessage() called with: geckoBundle = [{id=11, label=Bookmark Link}]
D: handleMessage() called with: event = [Prompt:Show], message = [{listitems=[Lorg.mozilla.gecko.util.GeckoBundle;@4da2c8, type=null, async=true, tabId=0, title=http://www.rapidtables.com/web/html/mailto.htm#how}], callback = [org.mozilla.gecko.EventDispatcher$NativeCallbackDelegate@7576f61]


D: handleMessage() called with: geckoBundle = [{showAsActions={uri=https://m.youtube.com/watch?v=cd3HriqJH98, title=0:36}, id=6, icon=drawable://ic_menu_share, label=Share Link}]
D: handleMessage() called with: geckoBundle = [{id=1, label=Open Link in New Tab}]
D: handleMessage() called with: geckoBundle = [{id=2, label=Open Link in Private Tab}]
D: handleMessage() called with: geckoBundle = [{id=3, label=Copy Link}]
D: handleMessage() called with: geckoBundle = [{id=11, label=Bookmark Link}]
D: handleMessage() called with: geckoBundle = [{id=26, label=Open With YouTube App}]
D: handleMessage() called with: event = [Prompt:Show], message = [{listitems=[Lorg.mozilla.gecko.util.GeckoBundle;@375258a, type=null, async=true, tabId=0, title=https://m.youtube.com/watch?v=cd3HriqJH98}], callback = [org.mozilla.gecko.EventDispatcher$NativeCallbackDelegate@d0d37fb]


[1] https://searchfox.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/prompts/PromptService.java#42
initialize
[2] https://searchfox.org/mozilla-central/source/mobile/android/chrome/content/browser.js#582
[3] https://searchfox.org/mozilla-central/source/mobile/android/chrome/content/browser.js#976
logic
[4] https://searchfox.org/mozilla-central/source/mobile/android/chrome/content/browser.js#2413
[5] https://searchfox.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3112
Flags: needinfo?(wehuang)
(In reply to Wesly Huang (Fennec Frontend/Firefox EPM) from comment #4)
> Unless Joe has other say, I think we have reserved some time in this sprint
> for potential photon/Leanplum QA-bug, so we should be able to prioritize
> this after you complete your MVP and the 2 important BL items.

Yes Nevin please secure the Leanplum Push notification testing toward the pre-beta signoff first as agreed with Product, thanks.
Flags: needinfo?(wehuang)
Priority: -- → P2
Per discussion with team, for 57 MVP, we'd want to at least respond to long press on links/images with only one option (Open in "Firefox")
Flags: needinfo?(jcheng)
Priority: P2 → P1
The "Open in XXX" here will open the target telement in Fennec (The browser APP). While you click "Oepein in XXX" on the menu, it will open the original page in Fennec.
Assignee: nobody → cnevinchen
Comment on attachment 8905394 [details]
Bug 1365868 - Add basic context menu: Open in Firefox.

https://reviewboard.mozilla.org/r/177196/#review182662

::: mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java:623
(Diff revision 2)
> +                OpenInFennec(content, CustomTabsActivity.this);
> +            }
> +        });
> +    }
> +
> +    void OpenInFennec(final String content, final Context context) {

Do we prefer method name with uppercase 'O' in this class? or it's a kotlin style? :P

::: mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java:633
(Diff revision 2)
> +            public void onPromptFinished(final GeckoBundle result) {
> +
> +                final int itemId = result.getInt("button", -1);
> +
> +                if (itemId == -1) {
> +

Add a comment to explicit indicate this is the error case. or early return before below startActivity

::: mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java:653
(Diff revision 2)
> +
> +    }
> +
> +    boolean isValidURL(String urlString) {
> +        try {
> +            URL url = new URL(urlString);

Why do we need to parse as URL and toURI, instead of URI.parseURI()?

Or is it better if we change `content` from String to URI and parse only once use this?

org.mozilla.gecko.util.URIUtils#uriOrNull
Attachment #8905394 - Flags: review?(max) → review+
Blocks: 1398067
Comment on attachment 8905394 [details]
Bug 1365868 - Add basic context menu: Open in Firefox.

https://reviewboard.mozilla.org/r/177196/#review182764

::: mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java:611
(Diff revision 5)
>  
>      @Override
>      public void onContextMenu(GeckoView view, int screenX, int screenY,
> -                              String uri, String elementSrc) {}
> +                              final String uri, final String elementSrc) {
> +
> +        final String content = uri != null ? uri : elementSrc != null ? elementSrc : "";

Note: this would "hide" nested images and media elements (e.g, <a><img></a>).

Should we want to allow actions on the nested element, we will need to consider both URIs and offer some split view akin to Fennec.
Thanks Eugen. We should defintely consider a more complete scenario. This patch here is just for quick fix for 57. I've open bug 1398197 for it.
oops.. just found Jan had opened bug 1398067 for comment 20. We should do it there.
Summary: Support context menu and "tab" related functionality in GeckoView-based custom tabs/web apps → Support minimal context menu functionality in GeckoView-based custom tabs
Pushed by nechen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4c9a3b09cfc1
Add basic context menu: Open in Firefox. r=maliu
https://hg.mozilla.org/mozilla-central/rev/4c9a3b09cfc1
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Whiteboard: [FNC][SPT57.3][INT]
Verified as fixed in 57.0b9.
Devices: Motorola Nexus 6 (Android 7.1.1) and Sony Xperia Z5 Premium (Android 6.0.1).
"Open in Firefox Beta" message is displayed when long tap on a link opened in Custom Tab.
Is this bug fixed in Nightly builds? Because it's still reproducible. Long tap on a link opens selection menu.
Flags: needinfo?(cnevinchen)
(In reply to Sorina Florean [:sorina] from comment #25)
> Is this bug fixed in Nightly builds? Because it's still reproducible. Long
> tap on a link opens selection menu.

There's a GeckoView problem here, bug 1411529.
Flags: needinfo?(cnevinchen)
Status: RESOLVED → VERIFIED
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 57 → mozilla57
You need to log in before you can comment on or make changes to this bug.