Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change permission UI on Desktop to enable / expose "permission until X" ability #14127

Closed
pes10k opened this issue Feb 12, 2021 · 26 comments · Fixed by brave/brave-core#7982
Closed
Assignees
Labels
design/needs-mock-up needs-mockup A feature which needs design mockup to be implemented. OS/Desktop privacy/feature User-facing privacy- & security-focused feature work. privacy/permissions privacy features related to limiting, lifetime or other permissions privacy privacy-pod Feature work for the Privacy & Web Compatibility pod QA Pass-Linux QA Pass-macOS QA Pass-Win64 QA/Test-All-Platforms QA/Yes release-notes/exclude

Comments

@pes10k
Copy link
Contributor

pes10k commented Feb 12, 2021

Description

This issue is follow up work for #14126

This issue is specifically to track the design and implementation of UI for desktop Brave, to enable / expose the new capabilities defined in #14126

The current (from upstream / default Chromium) UI currently looks like this:
Screen Shot 2021-02-11 at 8 19 15 PM

Design

image

Dropdown options:

  • until I close this page (default selection)
  • for 24 hours
  • for 1 week
  • forever

Links:

  • "Site permissions" links to brave://settings/content.
  • "Learn more" links to post covering site permissions (to be written)

Assets

Figma: https://www.figma.com/file/pVwKlNQJU9wTVOa9tImKpo/Browser-Dialogs?node-id=26%3A51
Use button component: https://www.figma.com/file/z9wmg2FCwuXx9FLbDo5avJ/Platform-UI-Brave-desktop?node-id=1011%3A0
Use link component: https://www.figma.com/file/z9wmg2FCwuXx9FLbDo5avJ/Platform-UI-Brave-desktop?node-id=766%3A9370

@pes10k pes10k added OS/Desktop privacy privacy-pod Feature work for the Privacy & Web Compatibility pod privacy/feature User-facing privacy- & security-focused feature work. privacy/permissions privacy features related to limiting, lifetime or other permissions design/needs-mock-up needs-mockup A feature which needs design mockup to be implemented. labels Feb 12, 2021
@iefremov
Copy link
Contributor

cc @karenkliu we would need assistance from the design team

@goodov
Copy link
Member

goodov commented Feb 17, 2021

I made a simple implementation for the permission lifetime selector to use while the feature is in the development state. If it's okay, I can push it to nightly and then we can update it to the proper UI when it's ready.
Thanks to our already existing Widevine additions to the permissions UI dialog, it costs us 0 conflict score.

l27h0PmfC5.mp4

@iefremov
Copy link
Contributor

while in development we can merge it covered with a feature flag, so please make a PR

@karenkliu
Copy link

karenkliu commented Feb 19, 2021

@pes10k @iefremov How about some options like this? User can click "Allow" and show a dropdown with a couple options. Block will block the request forever.

image

@iefremov
Copy link
Contributor

@karenkliu personally I would prefer to keep the ability to allow in one click, with some reasonable default.

@karenkliu
Copy link

karenkliu commented Feb 22, 2021

@iefremov @pes10k How's this?

image

"Allow" will allow until the page is closed. "Remember my decision" remembers Allow or Block forever.

@pes10k
Copy link
Contributor Author

pes10k commented Feb 24, 2021

I think that sounds good, though I think it'd be good to help curious users know more if they wanted. Could we add a "help" (or similar) link to a wiki or community support page? Then either I or someone from #support could write that up.

@karenkliu
Copy link

Designs updated!

@goodov
Copy link
Member

goodov commented Mar 2, 2021

@karenkliu The actual UI looks like this:
image

"site permissions" links to brave://settings/content. "Learn more" currently links to https://support.brave.com/.
There are some differences with the mock you did, I'll try to explain all of them.

  1. The buttons order. MacOS/Windows versions historically have different button order, the screenshot was taken from a Windows version.
  2. Highlight of the "Allow" button. Highlight is actually define a "default" action for a bubble, but the Permission bubble grants permissions, and it is known that some websites abused this default action in the past, that's why it's explicitly disabled: https://github.com/chromium/chromium/blob/90.0.4427.6/chrome/browser/ui/views/permission_bubble/permission_prompt_bubble_view.cc#L59-L62
    https://bugs.chromium.org/p/chromium/issues/detail?id=619429
    I think I can try to enable the highlight without adding the default action, but not sure if it's the right thing to do.
  3. The width of the "Allow" button. The UI logic tries to keep the same width for buttons up to some point, so it's expected behavior. If we put a little longer text into "Block" button, then this logic turns off and "Allow" button use its own size.
    https://github.com/chromium/chromium/blob/90.0.4427.6/ui/views/window/dialog_client_view.cc#L394-L396
    I can lower DISTANCE_BUTTON_MAX_LINKABLE_WIDTH value if it looks not as good as we would like to.

Please share you thoughts if we should improve it a little more or it's currently fine. Thanks!

@karenkliu
Copy link

@goodov Thanks for walking me through it! 👍

Highlight of the "Allow" button.

No highlight is fine if we're not recommending either action.

The width of the "Allow" button.

It looks fine as is.

We'll have a separate effort to bring the overall dialog style in line with the design system - Poppins typography, icons from our icon library, the correct link color and so on. Don't worry about it for now.

@stephendonner
Copy link

Setting to QA/Blocked for now until we have both a test plan as well as all the necessary landings, before we can (fully) test; I've pinged @goodov in #brave-core on Slack for further clarification - if it's ready, please feel free to remove that and let us know! Thanks!

@stephendonner
Copy link

stephendonner commented Mar 31, 2021

I took a closer look at this today, using

Brave 1.23.57 Chromium: 89.0.4389.105 (Official Build) beta (x86_64)
Revision 14f44e21a9d539cd49c72468a29bfca4fa43f710-refs/branch-heads/4389_90@{#7}
OS macOS Version 11.2.3 (Build 20D91)
  1. It's enabled via brave://flags as the Permission Lifetime flag, which yields this UI when enabled.
  2. As @karenkliu mentions in Change permission UI on Desktop to enable / expose "permission until X" ability #14127 (comment) above, there will be a separate effort to bring its UI/UX in-line.
  3. To fully test this -- since it's all about the permissions lifetimes -- we'll need Add support for time limits for Web API permissions in brave-core #14126 landed.
  4. For now, I've checked:
  • the new UI is enabled/disabled (including setting back to default, which currently disables it correctly) via brave://flags
  • permissions-time options are in the correct order (from least amount of time to greatest)
  • options are spelled correctly
  1. The Block forever button correctly sets the value for Location in brave://settings/content to Block
  2. The Allow button correctly sets the value for Location in brave://settings/content to Allow
  3. Via the brave://settings/content permissions for a given site, setting back to Ask (default) behaves the same as without the flag toggled
  4. The site permission link at the bottom of the dialog links to brave://settings/content
  5. Learn more links to placeholder wiki page: https://github.com/brave/brave-browser/wiki/Web-API-Permissions

@LaurenWags @rebron @kjozwiak thoughts on "good enough" to verify, for now, since the real work will be done over in #14126?


Verified the above spot checks using

Brave	1.23.63 Chromium: 89.0.4389.114 (Official Build) beta (64-bit)
Revision	1ea76e193b4fadb723bfea2a19a66c93a1bc0ca6-refs/branch-heads/4389@{#1616}
OS	Linux
  1. UI is enabled via brave://flags as the Permission Lifetime flag
  2. As @karenkliu mentions in Change permission UI on Desktop to enable / expose "permission until X" ability #14127 (comment) above, there will be a separate effort to display expected designs
  3. To fully test this -- since it's all about the permissions lifetimes -- we'll need Add support for time limits for Web API permissions in brave-core #14126 landed.
  4. For now, I've checked:
  • the new UI can be enabled/disabled via brave://flags
Default/Disabled Enabled Enabled
Screen Shot 2021-04-06 at 11 10 18 AM Screen Shot 2021-04-06 at 11 11 00 AM Screen Shot 2021-04-06 at 11 11 12 AM
  • permissions-time options are in the correct order (from least amount of time to greatest)
  • options are spelled correctly
  1. The Block forever button correctly sets the value for Location in brave://settings/content to Block
  2. The Allow button sets the value for Location in brave://settings/content to Allow
  3. Via the brave://settings/content permissions for a given site, setting back to Ask (default) behaves the same as without the flag toggled
  4. The site permission link at the bottom of the dialog links to brave://settings/content
  5. Learn more links to placeholder wiki page: https://github.com/brave/brave-browser/wiki/Web-API-Permissions

Verification passed on

Brave | 1.23.65 Chromium: 89.0.4389.114 (Official Build) dev (64-bit)
-- | --
Revision | 1ea76e193b4fadb723bfea2a19a66c93a1bc0ca6-refs/branch-heads/4389@{#1616}
OS | Windows 10 OS Version 2004 (Build 19041.867)
  1. UI is enabled via brave://flags as the Permission Lifetime flag
  2. As @karenkliu mentions in Change permission UI on Desktop to enable / expose "permission until X" ability #14127 (comment) above, there will be a separate effort to display expected designs
  3. To fully test this -- since it's all about the permissions lifetimes -- we'll need Add support for time limits for Web API permissions in brave-core #14126 landed.
  4. For now, I've checked:
  • the new UI can be enabled/disabled via brave://flags
Default/Disabled Enabled Enabled
image image image
  • permissions-time options are in the correct order (from least amount of time to greatest)
  • options are spelled correctly
  1. The Block forever button correctly sets the value for Location in brave://settings/content to Block
  2. The Allow button sets the value for Location in brave://settings/content to Allow
  3. Via the brave://settings/content permissions for a given site, setting back to Ask (default) behaves the same as without the flag toggled
  4. The site permission link at the bottom of the dialog links to brave://settings/content
  5. Learn more links to placeholder wiki page: https://github.com/brave/brave-browser/wiki/Web-API-Permissions
  • Ensured that permission changed in browserleaks.com/geo in the normal window is reflected in PT
  • Ensured permissions changed in browserleaks.com/geo in Private tab isn't retained in normal window browserleaks.com/geo
  • If permission is enabled in PT and blocked in a normal window, enable permissions apply only in private session and in the Recent activity we can see permission allowed for a private session
    image

Logged #15180

@LaurenWags
Copy link
Member

@stephendonner this looks good to me but let's get confirmation from @rebron @kjozwiak @karenkliu before proceeding on other platforms 👍🏻

@karenkliu
Copy link

To clarify, the UX is in-line, the UI is not 😆 . It works as designed, but the buttons/dropdowns don't look like what's in our style guide. There will be a separate effort to clean that up!

I don't see any issues with carrying over this permissions control dialog to other platforms.

@stephendonner
Copy link

Thanks for making the correction on the distinction between UI/UX - I didn't mean to conflate the two, and my verification steps basically attest the UX is good :-)

@bridiver
Copy link
Contributor

bridiver commented May 3, 2021

@karenkliu I got this permission prompt for the first time and was really confused by it. I showed it to @bbondy and he assumed "Forever" was a mistake. I definitely don't feel like it's clear that the dropdown only applies to "Allow", but until @pes10k pointed me to this ticket (I didn't find anything in the original #14126) I had no idea if the @bbondy was right or if "Forever" was correct and the UI was just confusing.

@bridiver
Copy link
Contributor

bridiver commented May 3, 2021

is there a reason why the permission length doesn't apply to "block"?

@bbondy
Copy link
Member

bbondy commented May 3, 2021

I think it should just say Block as the timeframe is specified in the drop down. As long as functionality matches that.

@goodov
Copy link
Member

goodov commented May 4, 2021

is there a reason why the permission length doesn't apply to "block"?

I think it should just say Block as the timeframe is specified in the drop down. As long as functionality matches that.

I appreciate your concerns, but let me explain. The decision to change Block to Block forever and add Give permission label for the dropdown comes from the idea that Block button behavior shouldn't change so a user can block permission request (and browser should remember it!) just by clicking it right away. If we change it to use the dropdown then it will require 2 additional clicks to block a permission request forever for this site. The muscle memory is a thing and this may be quite irritating for users.

Is this behavior change and consequences clear for everyone and we're okay with that? @bridiver @bbondy @pes10k

@bridiver
Copy link
Contributor

bridiver commented May 4, 2021 via email

@goodov
Copy link
Member

goodov commented May 4, 2021

I've implemented a change to handle lifetime options in "Block" decision brave/brave-core#8693

We should also take into account the Android dialog. It's a little different from what we have on desktop and we have no many options to change it, so the #15603 will cover all confusions on desktop and it will apply to Android as well.

@karenkliu
Copy link

@bridiver and @bbondy Can you take a look at the two proposed designs in #15603 to fix this UI issue and let us know which one is preferred?

This design applies the time frame in the dropdown to both Allow and Block options:
image

The downside is that it might be unnecessary to provide other time frame options for Block - would someone really want to block a permission request only until the site is closed?

This design lets you block forever in a click, and only select an Allow time length after clicking Allow:
image

The downside is that this design requires two clicks to allow a permission request.

@bridiver
Copy link
Contributor

bridiver commented May 4, 2021

I guess I don't see why a temporary allow is any more useful than a temporary block. In both cases it saves users from accidental permanent block/allow since they may not know how to find permissions in the settings ui

@bridiver
Copy link
Contributor

bridiver commented May 4, 2021

an even better option imo would be the first option, but when "Forever" is selected we could show a checkbox to "make forever the default for all permission prompts"

@iefremov
Copy link
Contributor

iefremov commented May 5, 2021

The current default behaviour is to Block or Allow forever, so when users do block something, they don't expect it will nag them anymore.
So I'm not sure changing the default to Block until the site close would be useful. Almost nobody changes the default, 95% users just click Block reflexively. So the "blocked" notification will appear more and more.

So I think the currently implemented lifetimes UI improves privacy: it limits "Allowed" stuff, but doesn't change the default lifetime for blocked permissions. IMO making "block" bound to the site lifetime would not be beneficial neither for privacy nor for UX (users will encounter more notifications)

@iefremov
Copy link
Contributor

iefremov commented May 5, 2021

Maybe we could move "Allow" to the same line as the combobox, and leave Block forever below? Though still need something else for Android

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design/needs-mock-up needs-mockup A feature which needs design mockup to be implemented. OS/Desktop privacy/feature User-facing privacy- & security-focused feature work. privacy/permissions privacy features related to limiting, lifetime or other permissions privacy privacy-pod Feature work for the Privacy & Web Compatibility pod QA Pass-Linux QA Pass-macOS QA Pass-Win64 QA/Test-All-Platforms QA/Yes release-notes/exclude
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants