Support added for loading local files through file:// handler #605

keianhzo · 2018-10-05T12:49:20Z

Fixes #594 Added support for general app permissions (outside Gecko) handling and support for loading content from external storage upon onLoadRequest.

bluemarvin

This looks good. But the permission request wording needs to be clarified. For:
Will you allow %1$s to read you external storage?
The file name is what is replacing the %1$s. I think it should be more along the lines of:
Will you allow <app name> to read <file name> from external storage?
or maybe:
Will you allow <app name> to request permission to read <file name> from external storage?
Or maybe even just go straight to the system permission prompt? Not sure what is best.

MortimerGoro · 2018-10-09T12:10:11Z

app/src/common/shared/org/mozilla/vrbrowser/PermissionDelegate.java

 import android.app.Activity;
 import android.content.Context;
 import android.content.pm.PackageManager;
+import android.support.annotation.NonNull;
+import android.support.v4.content.ContextCompat;


I don't think that we need to use android.support.v4 because our min API is v24.

keianhzo · 2018-10-09T15:06:10Z

@MortimerGoro fixed the ContextCompat issue.
@bluemarvin I have replaced the text for "Will you allow Firefox Reality to read you external storage?" as in this case is the app who is asking for the permission not the web site.

cvan · 2018-10-05T17:39:14Z

app/src/main/res/values/strings.xml

@@ -15,6 +15,8 @@
    <string name="permission_location">Will you allow %1$s to access your location?</string>
    <!-- Parameter will be replaced with the app name -->
    <string name="permission_notification">Will you allow %1$s to send notifications?</string>
+    <!-- Parameter will be replaced with the app name -->
+    <string name="permission_read_external_storage">Will you allow %1$s to read you external storage?</string>


small typo: you ->your

Thanks! Fixed

app/src/common/shared/org/mozilla/vrbrowser/ui/NavigationBarWidget.java

bluemarvin · 2018-10-09T19:12:54Z

app/src/main/res/values/strings.xml

@@ -15,6 +15,8 @@
    <string name="permission_location">Will you allow %1$s to access your location?</string>
    <!-- Parameter will be replaced with the app name -->
    <string name="permission_notification">Will you allow %1$s to send notifications?</string>
+    <!-- Parameter will be replaced with the app name -->
+    <string name="permission_read_external_storage">Will you allow %1$s to read your external storage?</string>


I guess I still find the wording confusing because the FxR dialog isn't actually granting permission, it is only asking the user if they want FxR to ask for permission. Then a system dialog is show (at least on Oculus) that requests that the user grant permission.

what is the benefit/rationale to introducing the initial permission prompt?

ideally, we should follow the Permissions API standard, correct?

The Android recommended flow for permissions request includes showing a dialog so you have a chance to explain to the users why you need that permissions before they are prompted by the system.

In our case there are two different sets of permissions, the ones requested by the website and the ones requested by the app.
Site permissions are requested per site. Gecko will grant permission to each site based on our custom dialog response. The system dialog is only shown once if the permission is already granted.
App permissions are only shown once for the whole app so for example if the user wants to access the internal storage to load a page it will prompted only once in case the user grants the permission.
In both cases we show the same dialog (that requires redesign btw), we just change the requester:
"Will you allow X to Y?" where X can be either "Firefox Reality" or a Website URL.

Oculus Browser does the same thing as us. It shows a banner at the bottom of the page saying that "xyz.com want's to use your location" if you say "Ok" the system dialog is shown.

Samsung Internet request permissions upon first start and doesn't let you use the browser unless you grant both external storage and location (for all sites) which is not the recommended flow as you should only request permissions when the are needed.

thanks for writing this all up. this is all helpful information.

ah, I understand now. and, you're right, Samsung Internet does prompt upon a fresh install -> launch for the File read permissions.

see my review comments, though, because I have the accepted the File read permissions.

another question: does Android conflate the "Storage" permission to mean both read and write access? because when I granted the Android permissions, I noticed that both permissions got granted (from running adb shell dumpsys package org.mozilla.vrbrowser):

runtime permissions: android.permission.READ_EXTERNAL_STORAGE: granted=true android.permission.WRITE_EXTERNAL_STORAGE: granted=true

I expected it to set only android.permission.READ_EXTERNAL_STORAGE.

Don't have an explanation for this but for some reason it seems to be Oculus related. It is not happening in the noapi or Daydream flavors. I've checked the merged manifests and I can't find a reason so it might be something that Oculus is doing under the hood.

cvan

Thanks for working on this! 👍 I have a couple notes and observations:

I was never able to get file:// URLs to load. I uninstalled, rebuilt, and reran FxR from Android Studio three separate times, but I still couldn't get it to work.

From a clean build, loading file:///, and having accepted both permission prompts:

From loading file:///sdcard/:

I ran adb shell dumpsys package org.mozilla.vrbrowser, and the permissions looked correct:
```
      runtime permissions:
        android.permission.READ_EXTERNAL_STORAGE: granted=true
        android.permission.WRITE_EXTERNAL_STORAGE: granted=true
```
While the Oculus Browser does not permit loading file:// URLs, Samsung Internet does (and without prompting, at both the chrome browser-permissions and the Android system-permissions levels):
The FxR permission modal is presented in a way that could make for easy clickjacking. Like the Settings modals are presented, the background of the browser should be dimmed. The white border colours also can look awkward when a user is browsing a 2D page with a white background color.
The icon used in the FxR permission modal is the icon to be used for Notifications.

If we decide to keep the FxR permission modal, we should probably use at the least use a file icon, like the Android permissions screen does:
What happens if a file:// URL is referenced or redirected to while the user is in Immersive Mode (WebVR content is being presented)? Do we kick out of Immersive Mode? Do we move the modal to be in front of the user? What if I am viewing content on a 2D page, but I am looking in a different direction? It's not obvious that the permissions modal is opened and needs my attention.
I was unable to regain the ability to load permissions after clicking the Don't ask again icon in the Android permissions modal (Allow Permissions). Even after relaunching FxR, rebuilding FxR, or using adb to re-grant the permissions, I was unable to ever load a different file:// URL (even for a different path, e.g., file:///sdcard/, file:///sdcard/file_local.html, etc.).

I ran adb shell dumpsys package org.mozilla.vrbrowser:
```
      runtime permissions:
        android.permission.READ_EXTERNAL_STORAGE: granted=false, flags=[ USER_FIXED ]
        android.permission.WRITE_EXTERNAL_STORAGE: granted=false, flags=[ USER_FIXED ]
```
I then ran adb shell pm grant org.mozilla.vrbrowser android.permission.READ_EXTERNAL_STORAGE:
```
      runtime permissions:
        android.permission.READ_EXTERNAL_STORAGE: granted=true, flags=[ USER_FIXED ]
        android.permission.WRITE_EXTERNAL_STORAGE: granted=false, flags=[ USER_FIXED ]
```
I re-built + ran FxR from Android Studio. I tried to load file:/// again, but I was shown the same error page from before:

keianhzo · 2018-10-11T15:52:47Z

@cvan

This is Gecko behavior, it seems that for:

file:/// if permission is granted it return a ERROR_FILE_NOT_FOUND onLoadError
File:///sdcard/ (that gets redirected to file:///storage/emulated/0 and Gecko does nothing so it seems to be a Gecko issue

In both cases above I think it should return an ERROR_FILE_NOT_FOUND. I'm going to open a GV issue to address that issue: https://bugzilla.mozilla.org/show_bug.cgi?id=1498270

Going to add a dim to avoid clickjacking.
I'll ask Nadja for a new design for this dialog as this UI is quite old, that should fix the white border and icon issues Update permissions dialog UI design #612
This might be an issue and even could pause immersive mode. Will test and open an issue with the result Review UI widgets popup handling while in Immersive mode #613
If the user clicks on "Don't ask again" the only way of re-enabling the permission is manually via the app settings or uninstalling the app. There is also no way of knowing if the user click on that checkbox.

keianhzo · 2018-10-12T16:00:49Z

@cvan I've updated the code to dim the environment when a permission prompt is shown. I'll address the rest of the issues in the other follow-ups.

keianhzo requested review from bluemarvin and MortimerGoro October 5, 2018 12:49

larsbergstrom added the in progress label Oct 5, 2018

keianhzo changed the title ~~Support added for loading local files through file:// handler (OnLoadUri)~~ Support added for loading local files through file:// handler Oct 5, 2018

keianhzo force-pushed the load-from-file branch from 389dbe6 to dc9dcee Compare October 5, 2018 17:19

bluemarvin suggested changes Oct 8, 2018

View reviewed changes

MortimerGoro suggested changes Oct 9, 2018

View reviewed changes

keianhzo force-pushed the load-from-file branch 2 times, most recently from fc82d54 to 2460bc6 Compare October 9, 2018 15:05

cvan suggested changes Oct 9, 2018

View reviewed changes

bluemarvin reviewed Oct 9, 2018

View reviewed changes

bluemarvin approved these changes Oct 9, 2018

View reviewed changes

keianhzo force-pushed the load-from-file branch 2 times, most recently from 4bc0688 to 8fe0e75 Compare October 10, 2018 14:02

MortimerGoro approved these changes Oct 10, 2018

View reviewed changes

cvan suggested changes Oct 10, 2018

View reviewed changes

keianhzo force-pushed the load-from-file branch 2 times, most recently from 189fd9d to e6f95df Compare October 11, 2018 14:30

Support added for loading local files through file:// handler

7869090

keianhzo force-pushed the load-from-file branch from e85881e to 7869090 Compare October 12, 2018 15:57

keianhzo merged commit a3e6668 into master Oct 15, 2018

larsbergstrom removed the in progress label Oct 15, 2018

bluemarvin deleted the load-from-file branch November 13, 2018 23:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support added for loading local files through file:// handler #605

Support added for loading local files through file:// handler #605

keianhzo commented Oct 5, 2018 •

edited

Loading

bluemarvin left a comment

MortimerGoro Oct 9, 2018 •

edited

Loading

keianhzo commented Oct 9, 2018

cvan Oct 5, 2018

keianhzo Oct 9, 2018

bluemarvin Oct 9, 2018

cvan Oct 10, 2018

keianhzo Oct 10, 2018

cvan Oct 10, 2018 •

edited

Loading

keianhzo Oct 11, 2018

cvan left a comment

keianhzo commented Oct 11, 2018 •

edited

Loading

keianhzo commented Oct 12, 2018

Support added for loading local files through file:// handler #605

Support added for loading local files through file:// handler #605

Conversation

keianhzo commented Oct 5, 2018 • edited Loading

bluemarvin left a comment

Choose a reason for hiding this comment

MortimerGoro Oct 9, 2018 • edited Loading

Choose a reason for hiding this comment

keianhzo commented Oct 9, 2018

cvan Oct 5, 2018

Choose a reason for hiding this comment

keianhzo Oct 9, 2018

Choose a reason for hiding this comment

bluemarvin Oct 9, 2018

Choose a reason for hiding this comment

cvan Oct 10, 2018

Choose a reason for hiding this comment

keianhzo Oct 10, 2018

Choose a reason for hiding this comment

cvan Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

keianhzo Oct 11, 2018

Choose a reason for hiding this comment

cvan left a comment

Choose a reason for hiding this comment

keianhzo commented Oct 11, 2018 • edited Loading

keianhzo commented Oct 12, 2018

keianhzo commented Oct 5, 2018 •

edited

Loading

MortimerGoro Oct 9, 2018 •

edited

Loading

cvan Oct 10, 2018 •

edited

Loading

keianhzo commented Oct 11, 2018 •

edited

Loading