Closed
Bug 1205216
Opened 9 years ago
Closed 9 years ago
API 23: SYSTEM_ALERT_WINDOW permission is not granted automatically and can't be requested
Categories
(Firefox for Android Graveyard :: Overlays, defect)
Firefox for Android Graveyard
Overlays
Tracking
(firefox46 fixed)
RESOLVED
FIXED
Firefox 46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: sebastian, Assigned: sebastian)
References
Details
Attachments
(9 files)
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
antlam
:
feedback+
|
Details |
(deleted),
text/x-review-board-request
|
nalexander
:
review+
|
Details |
(deleted),
image/png
|
antlam
:
feedback+
|
Details |
(deleted),
text/x-review-board-request
|
nalexander
:
review+
|
Details |
(deleted),
image/png
|
antlam
:
feedback+
|
Details |
(deleted),
text/x-review-board-request
|
nalexander
:
review+
|
Details |
When targetSdkVersion is set to API 23 (new permission system!) then SYSTEM_ALERT_WINDOW is not granted automatically anymore (On devices running Android 6+). We use this permission to draw on other apps (Tab queues).
Unfortunately this permission can't be requested at runtime. Instead the user has to manually(!) enable this for the app in the system settings: Settings -> Apps -> (gear icon) -> Draw over other apps -> Select app -> Enable "Permit drawing over other apps".
We will need to guide Android 6+ users through this process when they want to enable tab queues. It's possible to use the Settings.ACTION_MANAGE_OVERLAY_PERMISSION Intent action to send the user directly to the settings page for a specific application.
More:
https://medium.com/@rotxed/drawing-over-other-apps-marshmallow-edition-987eff9f99a9
http://www.androidpolice.com/2015/09/07/android-m-begins-locking-down-floating-apps-requires-users-to-grant-special-permission-to-draw-on-other-apps/
Assignee | ||
Comment 1•9 years ago
|
||
I recently filed bug 1197189. Currently we crash if you revoke this permission on Android M.
Assignee | ||
Updated•9 years ago
|
Component: General → Overlays
Assignee | ||
Updated•9 years ago
|
Blocks: targetSdkVersion23
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Blocks: targetSdkVersion23
Assignee | ||
Comment 2•9 years ago
|
||
Anthony: Do we have final mockups for this? This is the last piece I need to fix before I can start with runtime permissions. :)
Flags: needinfo?(alam)
Comment 3•9 years ago
|
||
Ah, this was the comment I had left in Aha FENN-321 but I should've posted it here.
From what I've seen other apps do, I don't think our Helper UI needs to change. We do however, need to write some copy to use in the dialog as well as the heads up notification.
Here's a rough UX flow with some placeholder text. I'll need to noodle on the copy a bit more.
I've triggered this a few times in other apps but can't seem to find an example right now.
Flags: needinfo?(alam)
Comment 4•9 years ago
|
||
Here's a rough mock of the Heads up notification as well if we need it.
Flags: needinfo?(s.kaspari)
Assignee | ||
Comment 5•9 years ago
|
||
Perfect. I'll start with this and post some screenshots. We can then start to tweak things if needed.
Flags: needinfo?(s.kaspari)
Assignee | ||
Comment 6•9 years ago
|
||
Facebook now shows the following screen when you want to enable chat heads on Android 6 (Previously they have hidden the setting).
Comment 7•9 years ago
|
||
Actually, I was thinking about using our Helper UI here as well.
Since we already have one for "Opening multiple links?" we could just forego the "Opt in dialog" screen and combine the messaging in the same Helper UI. That would save us 1 screen/ tap too!
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•9 years ago
|
||
If we need an uplift-able patch then we could:
* Disable the prompt after opening X external URLs on Android 6+ (To limit the number of people opting in until we have a good flow)
* Just use the toast-without-button fallback, that we already have in place, for users enabling the feature in our settings
=> This doesn't need strings.
To implement a better solution we need to (Android 6+):
* attachment 8682272 [details] - Tell the user about the setting that needs to be changed. Then launch an intent to the specific page in Android settings.
* Do the same from within our settings
* Only enable the feature if the user has enabled the setting
=> Needs UI and strings
Assignee | ||
Comment 9•9 years ago
|
||
Additionally:
* Show a notification whenever the feature is used but the permission is missing!
Assignee | ||
Comment 10•9 years ago
|
||
This is a first version of an adapted prompt.
* If we do have the permission for drawing over other apps then the prompt looks and behaves like it does now.
* If we do not have the permission then I show a slightly different text ("Turn on Permit drawing over other apps") and change the button to "Go to settings".
* When switching to settings I show "Turn on Permit drawing over other apps" as a toast again
* if the user enabled the setting and goes back then I enable tab queues and the user can continue to browse
* If the user did not enable the setting then the prompt will still be there waiting.
Attachment #8706953 -
Flags: feedback?(alam)
Assignee | ||
Comment 11•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/30545/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30545/
Assignee | ||
Comment 12•9 years ago
|
||
Comment on attachment 8706958 [details]
MozReview Request: Bug 1205216 - Tab queue prompt: Ask user to enable "Draw over other apps" if needed. r=nalexander
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30545/diff/1-2/
Assignee | ||
Comment 13•9 years ago
|
||
This is how the notification would look like in a situation where we do not have the permission but tab queue is enabled. The notification will be shown as soon as a tab is queued.
Attachment #8706984 -
Flags: feedback?(alam)
Assignee | ||
Comment 14•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/30669/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30669/
Assignee | ||
Comment 15•9 years ago
|
||
The last entry point for enabling tab queues is settings: A simple and quick solution is to reuse our tab queue prompt here. We could later go ahead and improve this.
Attachment #8707467 -
Flags: feedback?(alam)
Assignee | ||
Comment 16•9 years ago
|
||
Comment on attachment 8706958 [details]
MozReview Request: Bug 1205216 - Tab queue prompt: Ask user to enable "Draw over other apps" if needed. r=nalexander
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30545/diff/2-3/
Assignee | ||
Comment 17•9 years ago
|
||
Comment on attachment 8707382 [details]
MozReview Request: Bug 1205216 - Show notification if tab queue is enabled but permission is missing. r=nalexander
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30669/diff/1-2/
Assignee | ||
Comment 18•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/30717/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30717/
Comment 19•9 years ago
|
||
Comment on attachment 8706953 [details]
tabqueues_prompt_draw_over_apps.png
NICE! these diagrams are amazingly clear.
Attachment #8706953 -
Flags: feedback?(alam) → feedback+
Comment 20•9 years ago
|
||
Comment on attachment 8706984 [details]
tab_queue_notification_permission.png
Perfect!
Attachment #8706984 -
Flags: feedback?(alam) → feedback+
Comment 21•9 years ago
|
||
Comment on attachment 8707467 [details]
settings-prompt.png
Wfm!
Attachment #8707467 -
Flags: feedback?(alam) → feedback+
Assignee | ||
Updated•9 years ago
|
Attachment #8706958 -
Flags: review?(nalexander)
Assignee | ||
Updated•9 years ago
|
Attachment #8707382 -
Flags: review?(nalexander)
Assignee | ||
Updated•9 years ago
|
Attachment #8707468 -
Flags: review?(nalexander)
Assignee | ||
Comment 22•9 years ago
|
||
Comment 23•9 years ago
|
||
Comment on attachment 8706958 [details]
MozReview Request: Bug 1205216 - Tab queue prompt: Ask user to enable "Draw over other apps" if needed. r=nalexander
https://reviewboard.mozilla.org/r/30545/#review27739
I have nits and questions, but nothing that blocks landing. I'll leave it to you to respond and request a second pass if you feel it's necessary, or to land if there are no issues.
::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:114
(Diff revision 3)
> +import android.provider.Settings;
nit: extraneous?
::: mobile/android/base/java/org/mozilla/gecko/tabqueue/TabQueuePrompt.java:35
(Diff revision 3)
> + private static final int SETTINGS_REQUEST_CODE = 1;
Can we make this more likely to be unique? I worry about such things.
::: mobile/android/base/java/org/mozilla/gecko/tabqueue/TabQueuePrompt.java:82
(Diff revision 3)
> + okButton.setVisibility(canDrawOverlays ? View.VISIBLE : View.GONE);
Consider making this an `if` block, at your discretion. I understand this is shorter, but it's also harder to read.
::: mobile/android/base/java/org/mozilla/gecko/tabqueue/TabQueuePrompt.java:156
(Diff revision 3)
> + Toast.makeText(this, R.string.tab_queue_prompt_permit_drawing_over_apps, Toast.LENGTH_LONG).show();
nit: kill blank line.
::: mobile/android/base/java/org/mozilla/gecko/tabqueue/TabQueuePrompt.java:167
(Diff revision 3)
> + // User enabled the setting
nit: trailing period.
Also, let's use "permission" consistently when talking about this. It's possible to get here if the user does *not* enable the permission, right? (I.e., they hit hardware back and return with `RESULT_CANCEL`) or similar. What UI do they see in this case? Is the prompt still shown, with the same button to go to settings? If so, how do they cancel the prompt?
::: mobile/android/base/java/org/mozilla/gecko/tabqueue/TabQueuePrompt.java:168
(Diff revision 3)
> + setResult(TabQueueHelper.TAB_QUEUE_YES);
Should we be calling `onConfirmButtonPressed`, to get the nice animations?
Also, we're losing the telemetry calls. I think we want them, since otherwise it'll look like no Android M users use Tab Queues.
::: mobile/android/base/locales/en-US/android_strings.dtd:248
(Diff revision 3)
> +<!ENTITY tab_queue_prompt_permit_drawing_over_apps "Turn on Permit drawing over other apps">
Consider running this string by antlam; I don't know what the copy should be here.
::: mobile/android/base/locales/en-US/android_strings.dtd:253
(Diff revision 3)
> +<!ENTITY tab_queue_prompt_settings_button "Go to settings">
nit: emphasize that it's an Android *permission* in the localization note, and not part of Fennec's settings. Is Settings right? Can we say "Android Settings", to distinguish from Fennec's preferences?
Also, I see that we use "Settings" in `tab_queue_prompt_tip_text2` and "settings" in `tracking_protection_prompt_tip_text`. Can you make the capitalization uniform while you're here? (There is no need to rev the entity names for a cosmetic change like this.)
::: mobile/android/base/resources/layout/tab_queue_prompt.xml:76
(Diff revision 3)
> + tools:text="Turn on Permit drawing over other apps" />
Nice! (Although, I have a ticket -- languishing -- that should make this no longer necessary. Thanks for the reminder. (Bug 1074258))
Attachment #8706958 -
Flags: review?(nalexander) → review+
Comment 24•9 years ago
|
||
Comment on attachment 8707382 [details]
MozReview Request: Bug 1205216 - Show notification if tab queue is enabled but permission is missing. r=nalexander
https://reviewboard.mozilla.org/r/30669/#review27741
I see no issues. I wish it was easier to do this "in App": i.e., show a persistent snackbar or doorhanger under the URL bar when searching about things that are in bad state. We could surface bad configurations like this one; but also, Sync and Firefox Account error states, etc. Oh well, not blocking this.
Attachment #8707382 -
Flags: review?(nalexander) → review+
Comment 25•9 years ago
|
||
Comment on attachment 8707468 [details]
MozReview Request: Bug 1205216 - GeckoPreferences: Ask user to enable "Draw over other apps" if needed. r=nalexander
https://reviewboard.mozilla.org/r/30717/#review27743
One major concern, which I trust you to address.
::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java:1245
(Diff revision 1)
> + startActivityForResult(promptIntent, REQUEST_CODE_TAB_QUEUE);
As discussed on IRC, it's not clear to me that the instance that invokes `startActivityForResult` is the same instance that receives the `onActivityResult` callback. Therefore, let's be safe and always maintain the `tabQueuePreference`, regardless of whether we've interacted with this preference. That way we know that preference is never null. (Of course, we'll never release it either.)
::: mobile/android/base/java/org/mozilla/gecko/tabqueue/TabQueueHelper.java:295
(Diff revision 1)
> + return enabled;
You could also `return resultCOde == TAB_QUEUE_YES`, at your discretion.
Attachment #8707468 -
Flags: review?(nalexander) → review+
Assignee | ||
Comment 26•9 years ago
|
||
https://reviewboard.mozilla.org/r/30545/#review27739
> Can we make this more likely to be unique? I worry about such things.
This only needs to be unique for *this* activity, doesn't it?
> nit: trailing period.
>
> Also, let's use "permission" consistently when talking about this. It's possible to get here if the user does *not* enable the permission, right? (I.e., they hit hardware back and return with `RESULT_CANCEL`) or similar. What UI do they see in this case? Is the prompt still shown, with the same button to go to settings? If so, how do they cancel the prompt?
Yes, the prompt is still shown. They can dismiss it by pressing back or pressing almost anywhere else.
> Should we be calling `onConfirmButtonPressed`, to get the nice animations?
>
> Also, we're losing the telemetry calls. I think we want them, since otherwise it'll look like no Android M users use Tab Queues.
I didn't call onConfirmButtonPressed to avoid showing the animation while there's already the animation from dismissing Android's settings activity.
But I added the missing telemetry call!
Assignee | ||
Comment 27•9 years ago
|
||
Comment on attachment 8706958 [details]
MozReview Request: Bug 1205216 - Tab queue prompt: Ask user to enable "Draw over other apps" if needed. r=nalexander
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30545/diff/3-4/
Attachment #8706958 -
Attachment description: MozReview Request: Bug 1205216 - Tab queue prompt: Ask user to enable "Draw over other apps" if needed. r? → MozReview Request: Bug 1205216 - Tab queue prompt: Ask user to enable "Draw over other apps" if needed. r=nalexander
Assignee | ||
Comment 28•9 years ago
|
||
Comment on attachment 8707382 [details]
MozReview Request: Bug 1205216 - Show notification if tab queue is enabled but permission is missing. r=nalexander
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30669/diff/2-3/
Attachment #8707382 -
Attachment description: MozReview Request: Bug 1205216 - Show notification if tab queue is enabled but permission is missing. r? → MozReview Request: Bug 1205216 - Show notification if tab queue is enabled but permission is missing. r=nalexander
Assignee | ||
Comment 29•9 years ago
|
||
Comment on attachment 8707468 [details]
MozReview Request: Bug 1205216 - GeckoPreferences: Ask user to enable "Draw over other apps" if needed. r=nalexander
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30717/diff/1-2/
Attachment #8707468 -
Attachment description: MozReview Request: Bug 1205216 - GeckoPreferences: Ask user to enable "Draw over other apps" if needed. r? → MozReview Request: Bug 1205216 - GeckoPreferences: Ask user to enable "Draw over other apps" if needed. r=nalexander
Assignee | ||
Comment 30•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/c9d99a2255f50b45d7572c8e8ee4b441ae9ccffa
Bug 1205216 - Tab queue prompt: Ask user to enable "Draw over other apps" if needed. r=nalexander
https://hg.mozilla.org/integration/fx-team/rev/6e5c16c8d3e79fca9cefcfb6a8d7161468b45cf1
Bug 1205216 - Show notification if tab queue is enabled but permission is missing. r=nalexander
https://hg.mozilla.org/integration/fx-team/rev/05078e713365050c2a06e5e3e8728fd72c075b64
Bug 1205216 - GeckoPreferences: Ask user to enable "Draw over other apps" if needed. r=nalexander
Assignee | ||
Updated•9 years ago
|
Comment 31•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c9d99a2255f5
https://hg.mozilla.org/mozilla-central/rev/6e5c16c8d3e7
https://hg.mozilla.org/mozilla-central/rev/05078e713365
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Comment 32•9 years ago
|
||
The quotes in the notification don't show, dagnabbit http://developer.android.com/guide/topics/resources/string-resource.html#scaping_quotes, needs \".
Thanks to Sebastian for testing.
Probably needs a follow-up, but no ID change.
Updated•9 years ago
|
QA Contact: teodora.vermesan
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•