1432906 - Display notice in about:preferences when Policies are in use

Status: RESOLVED FIXED

(Firefox :: Settings UI, enhancement, P1)

Description

[:Felipe Gomes (needinfo for replies!)]

When the Policies engine is in use (if Services.policies.status == Services.policies.ACTIVE), we should display some message in about:preferences letting the user know about that.

This is to let them know why:
 - some features might be missing from the browser
 - some preferences might be locked

And to mitigate the risk of malware using this feature to change default settings in the browser

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

felipe, can you put a priority on this?

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Marking as P1 because we really want this for Firefox 60.  We need a string and a design on this.. My original idea is something like a yellow warning notice on top of about:preferences.

Suggestion for the wording:

"Notice: Some features and preferences are inaccessible because they have been locked by your system administrator."

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Amin, have you given this some thoughts?

Comment 4

[Amin Al Hazwani [:amin] (Firefox UX)]

Hey Felipe, here you can find the proposal for the notice in preferences https://mozilla.invisionapp.com/share/QWFUIA593BH#/screens/279275422_Enterprise_Edition. We can use a non-dismissable message bar https://design.firefox.com/photon/components/message-bars.html#style and place it on the left side of the preferences search input.

I opted for a more subtle color palette (generic grey instead of yellow warning message bar) as nothing damaging or harmful is happening in the preferences page.

Nice to have is, considering resources and time, a more contextual message bar that is next to the blocked/disabled preferences sections or options.

We need feedback from Brian Jones regarding the string to used there, I needinfo him.

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Attachment: mockup.png

Here's the current mockup

Comment 6

[:Felipe Gomes (needinfo for replies!)]

There were conversations about not having a Learn More link in the blocked pages, so for simplicity purposes, and taking advantage of that, I didn't implement a Learn More/Contact Admin parts here for the moment. I'll post a screenshot of how it looks like with my current patch.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Attachment: Current implementation

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Attachment: Current implementation with longer text that wraps

In this case, the box will stretch until the edge of main pane, and then it wraps.

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Amin, can you take a look at these screenshots and tell me what you think? Is this good enough for now? Any other changes that you recommend?

One thought that I have about the string is that it says that features are disabled, but in some cases it's not about being disabled, but just locked (e.g. homepage). And there are even cases about being force-enabled (e.g. there's a policy to enforce the "Remember logins and passwords" checkbox being checked).

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1432906 - Display notice in about:preferences when Policies are in use.

Review commit: https://reviewboard.mozilla.org/r/223728/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/223728/

Comment 11

[:Felipe Gomes (needinfo for replies!)]

Jared, with the caveat that we might still do some more UX changes, I think this is close enough to get started on review, at least to see if I'm doing things the right way

Comment 12

[Amin Al Hazwani [:amin] (Firefox UX)]

Hey Felipe,

Thanks for reaching out. I have a couple of feedback :) 

- Would be possible to position the message bar to the top left corner like in the mock https://mozilla.invisionapp.com/share/QWFUIA593BH#/screens/279275422_Enterprise_Edition instead of aligning it to the right next to the search input?
- The icon of the message bar should be this one: https://design.firefox.com/icons/viewer/#identity%20icon 
- The color of the icon should be var(--grey-90-a80)
- The color of text var(--grey-90)
- The background color should be var(--grey-20)
- Text should be 13px and line-height 1.4

Color variables available here https://github.com/FirefoxUX/design-tokens/blob/master/photon-colors/photon-colors.css

I built quick codepen to display all these changes https://codepen.io/aminalhazwani/pen/wyRXvr?editors=1100. You can also see how the text flows when you resize the browser window.

You can also also find all the message bar specs here https://design.firefox.com/photon/components/message-bars.html. If there is anything missing or not clear on the design system website please feel free to open an issue https://github.com/FirefoxUX/photon/issues/new. We are putting a lot of efforts into creating a documentation that is valuable for both designers and engineers so that we can facilitate such conversations.

(In reply to :Felipe Gomes (needinfo me!) from comment #6)
> There were conversations about not having a Learn More link in the blocked
> pages, so for simplicity purposes, and taking advantage of that, I didn't
> implement a Learn More/Contact Admin parts here for the moment. I'll post a
> screenshot of how it looks like with my current patch.

Feel free to ping me when we are ready to work on this so that I can support you on the design side.

(In reply to :Felipe Gomes (needinfo me!) from comment #9)
> Amin, can you take a look at these screenshots and tell me what you think?
> Is this good enough for now? Any other changes that you recommend?
> 
> One thought that I have about the string is that it says that features are
> disabled, but in some cases it's not about being disabled, but just locked
> (e.g. homepage). And there are even cases about being force-enabled (e.g.
> there's a policy to enforce the "Remember logins and passwords" checkbox
> being checked).

Yes, this is exactly what I think Bryan was referring to a couple of comments ago. It is quite hard to craft an effective string if we don't know what and why admins are blocking/disabling/force-enabling certain features. Is there a way for us to know if they are blocking/disabling/force-enabling something so that we can adapt the string accordingly. Brian what do you think?

Comment 13

[:Felipe Gomes (needinfo for replies!)]

Attachment: Latest implementation

- updated icon
- font-size for notice and search box is 13px
- min-height 32px
- updated colors
- made the box stretch as much as possible on the right panel (stretching all the way to left would be too complicated right now)

Comment 14

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8954648 [details]
Bug 1432906 - Display notice in about:preferences when Policies are in use.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223728/diff/1-2/

Comment 15

[Brian Jones]

(In reply to :Felipe Gomes (needinfo me!) from comment #9)

> 
> One thought that I have about the string is that it says that features are
> disabled, but in some cases it's not about being disabled, but just locked
> (e.g. homepage). And there are even cases about being force-enabled (e.g.
> there's a policy to enforce the "Remember logins and passwords" checkbox
> being checked).

With that in mind, the string can be:
Your organization has disabled the ability to change or disable some preferences.


---
Just to make sure: do we know for certain that an org won't prevent users from enabling (instead of disabling) a pref?

Comment 16

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8954648 [details]
Bug 1432906 - Display notice in about:preferences when Policies are in use.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223728/diff/2-3/

Comment 17

[:Felipe Gomes (needinfo for replies!)]

(In reply to Brian Jones from comment #15)
> (In reply to :Felipe Gomes (needinfo me!) from comment #9)
> 
> > 
> > One thought that I have about the string is that it says that features are
> > disabled, but in some cases it's not about being disabled, but just locked
> > (e.g. homepage). And there are even cases about being force-enabled (e.g.
> > there's a policy to enforce the "Remember logins and passwords" checkbox
> > being checked).
> 
> With that in mind, the string can be:
> Your organization has disabled the ability to change or disable some
> preferences.

I dropped the "or disable" part, and went with "Your organization has disabled the ability to change some preferences.". I think that conveys the situation well.

> 
> ---
> Just to make sure: do we know for certain that an org won't prevent users
> from enabling (instead of disabling) a pref?

Most policies work like this: define a choice & lock it.. So indeed there are cases where a feature will be locked and disabled, some other cases locked and enabled

Comment 18

[Brian Jones]

(In reply to :Felipe Gomes (needinfo me!) from comment #17)
> (In reply to Brian Jones from comment #15)
> > (In reply to :Felipe Gomes (needinfo me!) from comment #9)
> > 
> > > 
> > > One thought that I have about the string is that it says that features are
> > > disabled, but in some cases it's not about being disabled, but just locked
> > > (e.g. homepage). And there are even cases about being force-enabled (e.g.
> > > there's a policy to enforce the "Remember logins and passwords" checkbox
> > > being checked).
> > 
> > With that in mind, the string can be:
> > Your organization has disabled the ability to change or disable some
> > preferences.
> 
> I dropped the "or disable" part, and went with "Your organization has
> disabled the ability to change some preferences.". I think that conveys the
> situation well.
> 
> > 
> > ---
> > Just to make sure: do we know for certain that an org won't prevent users
> > from enabling (instead of disabling) a pref?
> 
> Most policies work like this: define a choice & lock it.. So indeed there
> are cases where a feature will be locked and disabled, some other cases
> locked and enabled

So we're comfortable saying that "change" encompasses "change" and "enable" AND enable? I'm not sure that's by-the-dictionary true, but agree that it gets at the "spirit" of the thing.

Comment 19

[:Felipe Gomes (needinfo for replies!)]

Yeah, change in my view here apply to the "setting", not a "feature"..  So it's preventing from changing a setting, which sometimes will mean disabling something, sometimes enabling something.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8954648 [details]
Bug 1432906 - Display notice in about:preferences when Policies are in use.

https://reviewboard.mozilla.org/r/223728/#review230782

::: browser/components/preferences/in-content/preferences.xul:203
(Diff revision 3)
>        <key data-l10n-id="focus-search" modifiers="accel" id="focusSearch1" oncommand="gSearchResultsPane.searchInput.focus();"/>
>      </keyset>
>  
>      <vbox class="main-content" flex="1" align="start">
>        <vbox class="pane-container">
> -        <hbox class="search-container" pack="end">
> +        <hbox class="search-container" pack="end" align="top">

Can we rename this class to "header-container" now that it holds things other than the search box too? 

https://searchfox.org/mozilla-central/search?q=search-container&case=false&regexp=false&path=pref shows that there are only a few places to change.

::: browser/locales/en-US/browser/preferences/preferences.ftl:27
(Diff revision 3)
>  # is the name of the CSS property. It is intended only to adjust the element's width.
>  # Do not translate.
>  search-field =
>      .style = min-width: 15.4em
>  
> +policies-notice = { PLATFORM() ->

Can you follow the same line break style as https://searchfox.org/mozilla-central/source/browser/locales/en-US/browser/preferences/preferences.ftl#12-17 ?

::: browser/themes/shared/incontentprefs/preferences.inc.css:755
(Diff revision 3)
>  
> +#policies-container,
> +#searchInput {
> +  min-height: 32px;
> +  margin: 20px 0 30px 0px;
> +  border-radius: 2px;

I don't see where the specs say to put border-radius:2px on the #searchInput.

::: browser/themes/shared/incontentprefs/preferences.inc.css:761
(Diff revision 3)
> +}
> +
> +#policies-container {
> +  background-color: var(--grey-20);
> +  padding: 0px 8px;
> +  margin-right: 16px;

margin-inline-end

::: browser/themes/shared/incontentprefs/preferences.inc.css:784
(Diff revision 3)
>    width: calc(100% + 8px);
>    margin-left: -4px;
>    margin-right: -4px;
>    padding-left: 4px;
>    padding-right: 4px;
> +  font-size: 13px;

Can you use font-size: 90% here? Using fixed pixel sizes will cause issues if the user has Zoom Text Only enabled.

Comment 21

[Amin Al Hazwani [:amin] (Firefox UX)]

Comment on attachment 8954648 [details]
Bug 1432906 - Display notice in about:preferences when Policies are in use.

https://reviewboard.mozilla.org/r/223728/#review230782

> I don't see where the specs say to put border-radius:2px on the #searchInput.

Yes, that is correct. It's not in the specs but the `#searchInput` is the only input in preferences that misses the rounded corners, in both windows and mac platform. And now, with the message bar (with rounded corners on the left side) it looked even more evident that this style was missing.

> Can you use font-size: 90% here? Using fixed pixel sizes will cause issues if the user has Zoom Text Only enabled.

I agree with this change as long as the icon on the left side of the message bar is vertically aligned and centered with the text, thanks!

Comment 22

[Tim Nguyen :ntim]

Comment on attachment 8954648 [details]
Bug 1432906 - Display notice in about:preferences when Policies are in use.

https://reviewboard.mozilla.org/r/223728/#review230968

::: browser/themes/shared/incontentprefs/preferences.inc.css:755
(Diff revision 3)
>  
> +#policies-container,
> +#searchInput {
> +  min-height: 32px;
> +  margin: 20px 0 30px 0px;
> +  border-radius: 2px;

If the border radius is missing from the search input, it should be fixed in common.css as well.

::: browser/themes/shared/incontentprefs/preferences.inc.css:759
(Diff revision 3)
> +  margin: 20px 0 30px 0px;
> +  border-radius: 2px;
> +}
> +
> +#policies-container {
> +  background-color: var(--grey-20);

I wouldn't introduce this variable for now. I would prefer introducing them as a bunch (from a package or something) rather than individually like this. Can you just use the hardcoded color here ?

Comment 23

[:Felipe Gomes (needinfo for replies!)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #20)
> > -        <hbox class="search-container" pack="end">
> > +        <hbox class="search-container" pack="end" align="top">
> 
> Can we rename this class to "header-container" now that it holds things
> other than the search box too? 

I started doing this, but "header" already had a meaning in this part of the code, which represents the subsection headers ("Startup", "Homepage", etc..)

So instead I changed this class to "sticky-container". Let me know if you're ok with it


> > +#policies-container,
> > +#searchInput {
> > +  min-height: 32px;
> > +  margin: 20px 0 30px 0px;
> > +  border-radius: 2px;
> 
> I don't see where the specs say to put border-radius:2px on the #searchInput.

Ah yeah that came from a Slack conversation with Amin

> ::: browser/themes/shared/incontentprefs/preferences.inc.css:784
> (Diff revision 3)
> >    width: calc(100% + 8px);
> >    margin-left: -4px;
> >    margin-right: -4px;
> >    padding-left: 4px;
> >    padding-right: 4px;
> > +  font-size: 13px;
> 
> Can you use font-size: 90% here? Using fixed pixel sizes will cause issues
> if the user has Zoom Text Only enabled.

90% was yielding a final font-size 13.46px, which was making the text wrap even in normal conditions. I changed it to 85% and it yields 12.7px, which gives it a little bit of head room to not wrap even with a slightly longer phrase.


(In reply to Tim Nguyen :ntim from comment #22)
> If the border radius is missing from the search input, it should be fixed in
> common.css as well.

Ah, the search input was already getting border-radius: 2px from common.inc.css (xul|textbox), but there was an override to border-radius: 0 in preferences.inc.css, so I just removed that. Thanks for the tip.

> 
> > +#policies-container {
> > +  background-color: var(--grey-20);
> 
> I wouldn't introduce this variable for now. I would prefer introducing them
> as a bunch (from a package or something) rather than individually like this.
> Can you just use the hardcoded color here ?

Done

Comment 24

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8954648 [details]
Bug 1432906 - Display notice in about:preferences when Policies are in use.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223728/diff/3-4/

Comment 25

[:Felipe Gomes (needinfo for replies!)]

Attachment: Latest implementation, final string

Comment 26

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8954648 [details]
Bug 1432906 - Display notice in about:preferences when Policies are in use.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223728/diff/4-5/

Comment 27

[:Felipe Gomes (needinfo for replies!)]

I added a test and sent it to tryserver. Jaws had given the r+, but there were quite a few nits to address, so I'll give everyone an extra chance to look at the new changes while this is running on tryserver.

Comment 28

[Mozilla Autoland]

hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 6 changes to 6 files
remote: (ftl_check check enabled per config override)
remote: 
remote: ************************ ERROR *************************
remote: You are trying to commit a change to an FTL file.
remote: At the moment modifying FTL files requires a review from
remote: one of the L10n Drivers.
remote: Please, request review from either:
remote:     - Francesco Lodolo (:flod)
remote:     - Zibi Braniecki (:gandalf)
remote:     - Axel Hecht (:pike)
remote:     - Stas Malolepszy (:stas)
remote: ********************************************************
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.mozhooks hook failed
abort: push failed on remote

Comment 29

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8954648 [details]
Bug 1432906 - Display notice in about:preferences when Policies are in use.

https://reviewboard.mozilla.org/r/223728/#review231094

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d0074d179c01
Display notice in about:preferences when Policies are in use. r=gandalf,jaws

Comment 31

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d0074d179c01

Comment 32

[Abe - QA (:Abe_LV)]

We have tested this on latest nightly and about:preferences displays a notice at the top of the page when a policy is in use.
Test case and runs are here- https://testrail.stage.mozaws.net/index.php?/plans/view/7734