1472528 - Design and implementation of about:policies page

Status: VERIFIED FIXED

(Firefox :: Enterprise Policies, enhancement)

Comment 1

[Kanika Saini]

Attachment: Bug 1472528 - Design and implementation of about:policies page

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

Comment 2

[Kanika Saini]

:felipe
There are a lot of things that need to be fixed. Sending this in for review to let you know about the progress. 
Looking for suggestions on the patch so far. :)

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Sure thing! I'll take a look.

Can you attach some screenshots to this bug with the work in progress?

Comment 4

[Kanika Saini]

Attachment: aboutPoliciesPageWIP.png

Comment 5

[Kanika Saini]

Comment on attachment 8989239 [details]
Bug 1472528 - Design and implementation of about:policies page

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

Comment 6

[Kanika Saini]

"Active" tab correctly displays all the active polcies (name, value and status-active). 
"All" tab requires more work now. 
Should we read the schema to display all the possible policies? 
I was wondering how to avoid the duplication of those policies that are active in that case. 
Also, for the policies that failed during setup, we could display the error in the status column. Thinking how that can be achieved too.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8989239 [details]
Bug 1472528 - Design and implementation of about:policies page

Hi Kanika, the work in progress so far is looking good!

My feedback on the questions that you asked:

- I think to generate the table you should iterate through the schema first, to contain all policies. Then you generate one row (<td>) per policy, and check if it's also present in the object returned by getActivePolicies.

If it is, then you add the value to the table, otherwise you leave it blank. And then you can add a CSS class to the row to make it invisible, and make the "All/Active" sidebar to toggle their visibility.

- We could have another sidebar entry called "Documentation", that you display all policies, and not their values, but their schema and their description.

- I think displaying errors per policy will be too complicated, but there could be another sidebar entry called "Errors" that were logged by the engine. You can do this by using the ConsoleAPIStorage component, like this:
https://searchfox.org/mozilla-central/source/devtools/client/webconsole/test/mochitest/browser_console_consolejsm_output.js#112

- Have you thought about how to display the values for complex policies, like the Homepage, Search and Bookmarks?

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8989385 [details]
aboutPoliciesPageWIP.png

Hey Bram, this is the bug that I mentioned to you on the creation of an about:policies page.

We have the first sketch here, and we'll be looking for for feedback soon.

One thing that we're interested right now is how to display the values for some complex policies. In the screenshot, we have a boolean policy, which is easy to display. But some policies take arrays [1], an object with various fields [2], and even an array of objects [3].


Examples:
[1] https://searchfox.org/mozilla-central/source/browser/components/enterprisepolicies/helpers/sample.json#6
[2] https://searchfox.org/mozilla-central/source/browser/components/enterprisepolicies/helpers/sample_proxy.json
[3] https://searchfox.org/mozilla-central/source/browser/components/enterprisepolicies/helpers/sample_bookmarks.json

Comment 9

[Kanika Saini]

Comment on attachment 8989239 [details]
Bug 1472528 - Design and implementation of about:policies page

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

Comment 10

[Code Review Bot [:reviewbot]]

Comment on attachment 8989239 [details]
Bug 1472528 - Design and implementation of about:policies page

https://reviewboard.mozilla.org/r/254182/#review262804


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/enterprisepolicies/content/aboutPolicies.js:15
(Diff revision 3)
> +XPCOMUtils.defineLazyModuleGetters(this, {
> +  schema: "resource:///modules/policies/schema.jsm",
> +});
> +
> +function col(element) {
> +  let col = document.createElement("td");

Error: 'col' is already declared in the upper scope. [eslint: no-shadow]

Comment 11

[Kanika Saini]

Attachment: All Policies Listed

Comment 12

[Kanika Saini]

Comment on attachment 8989239 [details]
Bug 1472528 - Design and implementation of about:policies page

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

Comment 13

[Code Review Bot [:reviewbot]]

Comment on attachment 8989239 [details]
Bug 1472528 - Design and implementation of about:policies page

https://reviewboard.mozilla.org/r/254182/#review262828


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/enterprisepolicies/content/aboutPolicies.js:15
(Diff revision 4)
> +XPCOMUtils.defineLazyModuleGetters(this, {
> +  schema: "resource:///modules/policies/schema.jsm",
> +});
> +
> +function col(element) {
> +  let col = document.createElement("td");

Error: 'col' is already declared in the upper scope. [eslint: no-shadow]

Comment 14

[Bram Pitoyo [:bram]]

Attachment: Screen Shot 2018-07-11 at 3.25.04 PM.png

(In reply to :Felipe Gomes (needinfo me!) [offline Jul 6 - Jul 15] from comment #8)
> One thing that we're interested right now is how to display the values for
> some complex policies. In the screenshot, we have a boolean policy, which is
> easy to display. But some policies take arrays [1], an object with various
> fields [2], and even an array of objects [3].

I would do something like the example shown.

What may be a little tricky is numbering each array – even when they don’t have a number – so that they appear in a useful order (1, 2, 3, etc.)

Of course, this means that the table width will need to be variable. If each object in the array can contain its own object, we will need to add more columns.

What do you think?

Comment 15

[Kanika Saini]

Comment on attachment 8989239 [details]
Bug 1472528 - Design and implementation of about:policies page

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

Comment 16

[Code Review Bot [:reviewbot]]

Comment on attachment 8989239 [details]
Bug 1472528 - Design and implementation of about:policies page

https://reviewboard.mozilla.org/r/254182/#review263330


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/enterprisepolicies/content/aboutPolicies.js:15
(Diff revision 5)
> +XPCOMUtils.defineLazyModuleGetters(this, {
> +  schema: "resource:///modules/policies/schema.jsm",
> +});
> +
> +function col(element) {
> +  let col = document.createElement("td");

Error: 'col' is already declared in the upper scope. [eslint: no-shadow]

Comment 17

[Kanika Saini]

Attachment: Documentation

Thank you, Bram! Your idea seems good but could it work for documentation as well?

Comment 18

[Bram Pitoyo [:bram]]

Comment on attachment 8991521 [details]
Documentation

Kanika, it seems that the documentations section has a very limited formatting: 3 columns, with a short name and schema and a long description.

Because of this predictability, we can make the layout of this section fairly static. What you’ve designed looks good, and the only thing I would add is alternating row colour.

Comment 19

[:Felipe Gomes (needinfo for replies!)]

Great work so far, Kanika! This is looking really good.


I like Bram's proposed design for the objects. One thing that I'm not sure about there are the array numberings:

The "Allow" and "Block" entries are also arrays, but the items inside are not numbered.. while the array of objects in the Bookarmks policy is.  So it is a bit inconsistent. I like the idea of not having them numbered to avoid an extra column, but it would make the Bookmarks list harder to view.

Maybe we could have a separator between each item of the array, or an empty row or just extra bottom padding at the end of each item, so that it's easy to distinguish when one item ends and another doesn't.

Bram, what do you think?

-----

For the documentation part, I like where it's going, but I think each entry will have to be expandable so that we can display more information, as the tiny column at the end won't be able to show the schemas.

Other information that we'll want to show in the documentation is whether a policy is enterprise_only/machine_only.

Also if we display the schema in a nice way, we will want to show which fields are required, and have a good way to display the string enums.

Perhaps we could even create some icons to display some of this information (e.g., machine-only, required, etc.)


-----

Although I like the columns idea, I just wanted to throw an alternate idea to keep in mind, in case the columns don't work out in practice. This is from a screenshot of the about:performance page that Florian is working on: https://i.imgur.com/MGOBu95.png

So instead of creating new columns for each level in the object, we could add new rows that are indented. Basically like a better-looking JSON viewer

Comment 20

[Bram Pitoyo [:bram]]

(In reply to :Felipe Gomes (needinfo me!) from comment #19)
> Maybe we could have a separator between each item of the array, or an empty
> row or just extra bottom padding at the end of each item, so that it's easy
> to distinguish when one item ends and another doesn't.


I like this idea a lot, and have a small improvement idea to offer.

1. Each policy item is strongly separated: large bottom padding, thick lines
2. Each array that contains multiple items is weakly separated: small padding, thin lines
3. Each array that only contains a single item doesn’t need to be separated from each other


> For the documentation part, I like where it's going, but I think each entry
> will have to be expandable so that we can display more information, as the
> tiny column at the end won't be able to show the schemas.

Is it worth it to adapt the layout that other products use for their documentation, for our policies, even if our layout will end up greatly simplified?

This way, we won’t forcefully adapt the table metaphor into a purpose that doesn’t suit it well.

Examples:

* https://developer.android.com/reference/android/bluetooth/package-summary
* https://developer.apple.com/documentation/coreservices/apple_events?language=objc
* https://dropbox.github.io/SwiftyDropbox/api-docs/latest/Classes/Async.html
* https://stripe.com/docs/api <-- my favourite; it horizontally separates each topic from its usage examples


> Although I like the columns idea, I just wanted to throw an alternate idea
> to keep in mind, in case the columns don't work out in practice. This is
> from a screenshot of the about:performance page that Florian is working on:
> https://i.imgur.com/MGOBu95.png
> 
> So instead of creating new columns for each level in the object, we could
> add new rows that are indented. Basically like a better-looking JSON viewer

This is also an idea to explore. Is there a way to see the policies page displayed in this “JSON Viewer” layout?

Comment 21

[Romain Testard [:RT]]

Great to see good progress there!
Some comments in comparison to what Chrome offers:
- chrome://policy has a button to export policies with JSON format. I'm not too sure when it can be useful but maybe you see a use case for us doing it too?
- Regarding documentation, Chrome links to https://www.chromium.org/administrators/policy-list-3#AbusiveExperienceInterventionEnforce when you hit the policy name. It's nice since it allows expanding on the doc beyond what a table like https://bug1472528.bmoattachments.org/attachment.cgi?id=8991521 will allow. Any particular reason for hosting the doc under about:policy?

Comment 22

[:Felipe Gomes (needinfo for replies!)]

(In reply to Romain Testard [:RT] from comment #21)
> - chrome://policy has a button to export policies with JSON format. I'm not
> too sure when it can be useful but maybe you see a use case for us doing it
> too?

We recently added that on Nightly (not on 60 though) as a link in about:support (bug 1465952).

The original idea was to replace that link with a link to about:policies when it's ready. But once we do that, if that functionality is found useful, we could have that same functionality as a link in about:policies itself.


> - Regarding documentation, Chrome links to
> https://www.chromium.org/administrators/policy-list-
> 3#AbusiveExperienceInterventionEnforce when you hit the policy name. It's
> nice since it allows expanding on the doc beyond what a table like
> https://bug1472528.bmoattachments.org/attachment.cgi?id=8991521 will allow.
> Any particular reason for hosting the doc under about:policy?

I intend to link-ify the policies to point to more details on the documentation (either to https://github.com/mozilla/policy-templates/blob/master/README.md or somewhere else).
But the short description and the schema can be shown inline because that information is included in the build.

Comment 23

[Kanika Saini]

Comment on attachment 8989239 [details]
Bug 1472528 - Design and implementation of about:policies page

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

Comment 24

[Kanika Saini]

Attachment: Active Policies Tab

Active Policies Tab is working perfectly in terms of displaying complex policies.
Things that are left are:
1) Adding Machine-only icons
2) Removing indices for the array items and adding css to make them visually distinctive

Comment 25

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8994006 [details]
Active Policies Tab

This is looking great! Can't wait to see what it looks like with the things you mention in comment 24.

Looking into the screenshot for a little bit, I believe we really don't need the "Status" column. Removing it will give more horizontal space to the policy values, which is nice.

The "Active" tab will only contain Active policies.. And when the user chooses All Policies, we could differentiate the Inactive ones either by:
  1) Using a gray text color in the policy name
  2) Saying "Inactive" in the policy value column

(I'm more in favor of option 1)

Comment 26

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8994006 [details]
Active Policies Tab

Let's see what Bram has to say about the current screenshot

Comment 27

[Bram Pitoyo [:bram]]

Comment on attachment 8994006 [details]
Active Policies Tab

(In reply to Kanika Saini from comment #24)

This looks great, Kanika!

One design feedback. I would suggest giving every other line on the table a slight grey background (grey 10 #f9f9fa at 10%)

I will upload the rest of my design feedback in a separate attachment.


Lastly, I would echo Felipe on comment 25:

> […] we really don't need the "Status" column […]
> 
> The "Active" tab will only contain Active policies.. And when the user
> chooses All Policies, we could differentiate the Inactive ones either by:
>   1) Using a gray text color in the policy name

So let’s take out the “Status” column, and grey out the inactive policy.

According to the Photon theme (https://firefoxux.github.io/people/aalhazwani/firefox-design-system/themes/index.html), the colour of this text should be:

> Inactive Text Grey 90 #0c0c0d at 40%

Comment 28

[Bram Pitoyo [:bram]]

Attachment: Policies table design feedback.png

This feedback is an annotated version of attachment 8991199 [details].

Comment 29

[Kanika Saini]

Comment on attachment 8989239 [details]
Bug 1472528 - Design and implementation of about:policies page

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254182/diff/6-7/

Comment 30

[Kanika Saini]

Attachment: Active Policies Tab

Machine-only icon added which on hover says "machine-only".

For alternate row background color, I tried to color the even rows white (as the background is already gray) but the part of the rows that doesn't extend to the right extreme remain gray. Is there a way to extend

Comment 31

[Kanika Saini]

(In reply to Kanika Saini from comment #30)
> Created attachment 8994508 [details]
> Active Policies Tab
> 
> Machine-only icon added which on hover says "machine-only".
> 
> For alternate row background color, I tried to color the even rows white (as
> the background is already gray) but the part of the rows that doesn't extend
> to the right extreme remain gray. Is there a way to extend the background color using CSS?

Comment 32

[Kanika Saini]

Attachment: Documentation

Comment 33

[Bram Pitoyo [:bram]]

Comment on attachment 8994509 [details]
Documentation

This looks like we’re heading in the right direction. It’s quite readable, with good separation between each policy.

To further enhance this separation, I would recommend:

* Adding vertical padding between each policy. Try values in multiples of 4px, and maybe start at 16px.

* Darkening the separator line colour. Try grey-90 (#0c0c0d).

Comment 34

[Bram Pitoyo [:bram]]

Comment on attachment 8994508 [details]
Active Policies Tab

(In reply to Kanika Saini from comment #30)
> Created attachment 8994508 [details]
> Active Policies Tab


> For alternate row background color, I tried to color the even rows white (as
> the background is already gray) but the part of the rows that doesn't extend
> to the right extreme remain gray. Is there a way to extend

I’m not sure whether we can extend the row colour here, but I would recommend extending it so that the colour of each row is consistent.

Are you able to separate some (not all) strings with thin lines, as shown in attachment 8994385 [details]?

Echoing comment 33, I would also recommend separating each policy with a vertical padding, and darkening the thick lines.

Comment 35

[Kanika Saini]

Comment on attachment 8989239 [details]
Bug 1472528 - Design and implementation of about:policies page

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254182/diff/7-8/

Comment 36

[Kanika Saini]

Attachment: Active Policies Tab

Comment 37

[Kanika Saini]

Attachment: Errors Tab

Error tab is only visible when there's an error.

Comment 38

[Kanika Saini]

Attachment: Documentation with Schema

Comment 39

[Code Review Bot [:reviewbot]]

Comment on attachment 8989239 [details]
Bug 1472528 - Design and implementation of about:policies page

https://reviewboard.mozilla.org/r/254182/#review267372


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/enterprisepolicies/content/aboutPolicies.js:42
(Diff revision 8)
> +
> +function formatted_col(element, column_span = "1") {
> +  let column = document.createElement("td");
> +  column.colSpan = column_span;
> +  let content = document.createElement("pre");
> +  content.innerHTML = element;

Error: Unsafe assignment to innerHTML [eslint: no-unsanitized/property]

::: browser/components/enterprisepolicies/content/aboutPolicies.js:197
(Diff revision 8)
> +      if (seen.has(value)) {
> +        return;
> +      }
> +      seen.add(value);
> +    }
> +    return value;

Error: Arrow function expected no return value. [eslint: consistent-return]

Comment 40

[Kanika Saini]

Comment on attachment 8989239 [details]
Bug 1472528 - Design and implementation of about:policies page

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254182/diff/8-9/

Comment 41

[Code Review Bot [:reviewbot]]

Comment on attachment 8989239 [details]
Bug 1472528 - Design and implementation of about:policies page

https://reviewboard.mozilla.org/r/254182/#review267498


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/enterprisepolicies/content/aboutPolicies.js:42
(Diff revision 9)
> +
> +function formatted_col(element, column_span = "1") {
> +  let column = document.createElement("td");
> +  column.colSpan = column_span;
> +  let content = document.createElement("pre");
> +  content.innerHTML = element;

Error: Unsafe assignment to innerHTML [eslint: no-unsanitized/property]

Comment 42

[Kanika Saini]

Comment on attachment 8989239 [details]
Bug 1472528 - Design and implementation of about:policies page

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254182/diff/9-10/

Comment 43

[Code Review Bot [:reviewbot]]

Comment on attachment 8989239 [details]
Bug 1472528 - Design and implementation of about:policies page

https://reviewboard.mozilla.org/r/254182/#review268412


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python/etc)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/enterprisepolicies/content/aboutPolicies.js:42
(Diff revision 10)
> +
> +function formatted_col(element, column_span = "1") {
> +  let column = document.createElement("td");
> +  column.colSpan = column_span;
> +  let content = document.createElement("pre");
> +  content.innerHTML = element;

Error: Unsafe assignment to innerHTML [eslint: no-unsanitized/property]

Comment 44

[Francesco Lodolo [:flod]]

Is there anything blocking us from using Fluent for this page, instead of DTDs?
https://firefox-source-docs.mozilla.org/intl/l10n/l10n/fluent_tutorial.html

It's kind of sad to see brand new features use old technology, especially considering we'll need to migrate it at some point.

Comment 45

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

Not really no. We should be able to use Fluent here and use HTML instead of XHTML then.

Comment 46

[:Felipe Gomes (needinfo for replies!)]

I thought Fluent was still meant to be contained in about:preferences. I didn't know we're supposed to start using it elsewhere now.

Comment 47

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

we can select on case-by-case basis. It's also limited for now to chrome-privileged content until bug 1455649 lands.

Comment 48

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8989239 [details]
Bug 1472528 - Design and implementation of about:policies page

https://reviewboard.mozilla.org/r/254182/#review268446

Hey Kanika, below are some comments on the code, and two bugs that I noticed while testing it:

I used the sample_bookmarks.json file (and removed the data: Favicon from there), and I see more separators than there should be. (see here: https://i.imgur.com/S55jroj.png , there are some separators between Placement and Favicon that shouldn't be there).

In general, after playing with it for a while, I'm leaning torwards just having separators at the end of each object key, and not having any separators between the items of a same array.


Also, the code will need some comments for the more complicated parts :)  (like the 4n + 1 part in the CSS), and some comments explaining what each big case means in the displayPolicy functions

::: browser/components/enterprisepolicies/content/aboutPolicies.css:104
(Diff revision 10)
> +  height: 16px;
> +  vertical-align: middle;
> +  width: 16px;
> +}
> +.icon.machine-only {
> +  background-image: url("chrome://browser/skin/notification-icons/focus-tab-by-prompt.svg");

I think this icon looks better for this purpose: chrome://browser/skin/developer.svg

::: browser/components/enterprisepolicies/content/aboutPolicies.js:14
(Diff revision 10)
> +function col(element, classFlag) {
> +  let column = document.createElement("td");
> +  if (classFlag) {
> +    column.classList.add("array");
> +  }
> +  let content = document.createTextNode(element);
> +  column.appendChild(content);
> +  return column;
> +}

the parameter name `element` would be better named as `text`.

instead of the classFlag being a boolean, it could be a string containing "array". that way you wouldn't need the formatted_col function (more on that below)

::: browser/components/enterprisepolicies/content/aboutPolicies.js:30
(Diff revision 10)
> +  let icon = document.createElement("span");
> +  icon.classList.add("icon");
> +  icon.classList.add("machine-only");
> +  icon.title = "Machine-only";
> +  let column = document.createElement("td");
> +  let contentHolder = document.createElement("span");

do you really need this `<span>` to hold the text? why couldn't it go directly in the `<td>`, and then have the `<span>` only for the icon?

so, something like

`<td>DisableAppUpdate <span class="icon machine-only"></td>`

::: browser/components/enterprisepolicies/content/aboutPolicies.js:38
(Diff revision 10)
> +function formatted_col(element, column_span = "1") {
> +  let column = document.createElement("td");
> +  column.colSpan = column_span;
> +  let content = document.createElement("pre");
> +  content.innerHTML = element;
> +  column.appendChild(content);
> +  return column;
> +}

instead of using the `<pre>` tag, you could do the same thing with CSS, by choosing a monospaced font, and setting `white-space: pre`.

For the column-span, since this function returns the column, you could set the column span outside of this function.

That way, you could eliminate this function, and just use the col function from above, with a different class name.

::: browser/components/enterprisepolicies/content/aboutPolicies.js:113
(Diff revision 10)
> +  }
> +
> +  parent.replaceChild(new_cont, cont);
> +}
> +
> +function displayPolicy(data, row, depth, new_cont, islast, arr_sep = false) {

if you use a new `<tbody>` on each policy, you wouldn't need to keep track of the `islast` variable here in order to set the `"lastpolicyrow"` class.

You could just have the border-bottom in the `tbody`.

This would probably make this function easier to understand, and simplify other things too.

::: browser/components/enterprisepolicies/content/aboutPolicies.js:183
(Diff revision 10)
> +
> +function displayErrors() {
> +  const consoleStorage = Cc["@mozilla.org/consoleAPI-storage;1"];
> +  const storage = consoleStorage.getService(Ci.nsIConsoleAPIStorage);
> +  const consoleEvents = storage.getEvents();
> +  const prefixes = ["Enterprise Policies", "Policies.jsm", "GPOParser.jsm", "Enterprise Policies Child", "BookmarksPolicies.jsm", "ProxyPolicies.jsm", "WebsiteFilter Policy"];

please break this into several lines, like

const prefixes = [
  "Enterprise Policies",
  "Policies.jsm",
  ...
]

and also add JsonSchemaValidator.jsm to the list

::: browser/components/enterprisepolicies/content/aboutPolicies.js:195
(Diff revision 10)
> +  let flag = false;
> +  for (let err of consoleEvents) {
> +    if (prefixes.includes(err.prefix)) {
> +      flag = true;
> +      let row = document.createElement("tr");
> +      row.appendChild(formatted_col(JSON.stringify(err.arguments[0], null, 1)));

no need anymore to call JSON.stringify since err.arguments[0] is a string already  (this will remove the unecessary quotes displayed in the end result)

::: browser/components/enterprisepolicies/content/aboutPolicies.js:233
(Diff revision 10)
> +    main_tbody.appendChild(row);
> +    let sec_tbody = document.createElement("tbody");
> +    sec_tbody.classList.add("content");
> +    sec_tbody.classList.add("content-style");
> +    let schema_row = document.createElement("tr");
> +    schema_row.appendChild(formatted_col(schema.properties[policyName].properties ? JSON.stringify(schema.properties[policyName].properties, null, 1) : schema.properties[policyName].type, "2"));

this could be made easier to read if you break it in `if` conditions.

The SearchBar policy is not displaying the complete details because it is of type "string", but it has an `enum`.

::: browser/components/enterprisepolicies/content/aboutPolicies.js:242
(Diff revision 10)
> +function requestActivePolicies() {
> +  return Services.policies.getActivePolicies();
> +}

no need for this function, just call it directly in the other function

::: browser/components/enterprisepolicies/content/aboutPolicies.js:254
(Diff revision 10)
> +  displayActivePolicies(data);
> +  displayErrors();
> +  displayDocumentation();

Let's rename these "display" functions to "generate", because display implies that they will be called each time that the tab is selected, when in fact it's only called once.  (and it is the `show` function that is called multiple times)

::: browser/components/enterprisepolicies/content/aboutPolicies.js:293
(Diff revision 10)
> +// We use the pageshow event instead of onload. This is needed because sometimes
> +// the page is loaded via session-restore/bfcache. In such cases we need to call
> +// init() to keep the page behaviour consistent with the ticked checkboxes.
> +// Mostly the issue is with the autorefresh checkbox.
> +window.addEventListener("pageshow", function() {
> +  init();
> +});

I know that this was just copy & pasted from `about:networking`, but it isn't needed here. So you can remove this and just add onload="init()" into the `<body>` tag in the html file.

With this, you can also remove the gInited variable from the init() function because it's never gonna be called more than once

::: browser/components/enterprisepolicies/content/aboutPolicies.xhtml:25
(Diff revision 10)
> +    <body id="body">
> +        <div id="categories">
> +            <div class="category" selected="true" id="category-active">
> +                <span class="category-name">&aboutPolicies.active;</span>
> +            </div>
> +                        <div class="category" id="category-documentation">

nit: wrong indentation

Comment 49

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8989239 [details]
Bug 1472528 - Design and implementation of about:policies page

(just clearing the review request to wait for the next version)

Comment 50

[Kanika Saini]

Comment on attachment 8989239 [details]
Bug 1472528 - Design and implementation of about:policies page

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254182/diff/10-11/

Comment 51

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8989239 [details]
Bug 1472528 - Design and implementation of about:policies page

https://reviewboard.mozilla.org/r/254182/#review268868

::: browser/components/enterprisepolicies/content/aboutPolicies.css:129
(Diff revision 11)
> +.content-style {
> + background-color: #777;
> + color: white;
> +}

I changed this to

.content-style {
  background-color: white;
  color: var(--in-content-category-text-selected);
}

and I think it matches more the page style: https://i.imgur.com/pvRfUGQ.png

what do you think?

::: browser/components/enterprisepolicies/content/aboutPolicies.js:43
(Diff revision 11)
> +  let cont = document.getElementById("activeContent");
> +  let parent = cont.parentNode;
> +  let new_cont = document.createElement("tbody");
> +  new_cont.classList.add("active-policies");
> +  new_cont.setAttribute("id", "activeContent");

what's done here (and also at the generateErrors and generateDocumentation functions) is a bit weird: creating a new element with the same id and replacing it at the end of the function.

No need to do this. Just have the class="active-policies" etc in the html file, and use the existing element obtained from getElementById

::: browser/components/enterprisepolicies/content/aboutPolicies.js:51
(Diff revision 11)
> +  new_cont.classList.add("active-policies");
> +  new_cont.setAttribute("id", "activeContent");
> +
> +  for (let policyName in data) {
> +    if (schema.properties[policyName].type == "array") {
> +      for (let obj in data[policyName]) {

usually when iterating on an array, if we're using `for..in` to iterate on the indexes, we call the variable `i`, `count` or `index`.

`obj` is usually used when using `for..of`


Since you used `count` below here for the object case, you could use the same here to match

::: browser/components/enterprisepolicies/content/aboutPolicies.js:52
(Diff revision 11)
> +        if (obj == 0) {
> +          let first_row = document.createElement("tr");
> +          first_row.appendChild(col(policyName));
> +
> +          if (obj == data[policyName].length - 1) {
> +            generatePolicy(data[policyName][obj], first_row, 1, new_cont, true, false);
> +          } else {
> +            generatePolicy(data[policyName][obj], first_row, 1, new_cont, false, true);
> +          }
> +        } else if (obj == data[policyName].length - 1) {
> +          let last_row = document.createElement("tr");
> +          last_row.appendChild(col(""));
> +          generatePolicy(data[policyName][obj], last_row, 1, new_cont, true, false);
> +        } else {
> +          let row = document.createElement("tr");
> +          row.appendChild(col(""));
> +          generatePolicy(data[policyName][obj], row, 1, new_cont, false, true);
> +        }
> +      }

you could simplify this block and not have an if case for each case, by using some booleans..

something like this:

```
let isFirstRow = (i == 0);
let isLastRow = (i == length - 1);

row = createElement("tr");

row.appendChild(col(isFirstRow ? policyName : ""));

generatePolicy(..., row, 1, container, isFirstRow, !isLastRow)
```

this will also make things more explicit and make it easier to understand (i.e., it's easier to see that array_separator will be false when isLastRow is true

::: browser/components/enterprisepolicies/content/aboutPolicies.js:71
(Diff revision 11)
> +          let row = document.createElement("tr");
> +          row.appendChild(col(""));
> +          generatePolicy(data[policyName][obj], row, 1, new_cont, false, true);
> +        }
> +      }
> +    } else if (schema.properties[policyName].type == "object" && Object.keys(data[policyName]).length > 0) {

out of curiosity, what is the case that the `Object.keys(data[policyName]).length > 0` is protecting from?

::: browser/components/enterprisepolicies/content/aboutPolicies.js:74
(Diff revision 11)
> +        if (count == 0) {
> +          let first_row = document.createElement("tr");
> +          first_row.appendChild(col(policyName));
> +          first_row.appendChild(col(obj));
> +          if (count == Object.keys(data[policyName]).length - 1) {
> +            generatePolicy(data[policyName][obj], first_row, 2, new_cont, true);
> +          } else {
> +            generatePolicy(data[policyName][obj], first_row, 2, new_cont, false);
> +          }
> +        } else if (count == Object.keys(data[policyName]).length - 1) {
> +          let last_row = document.createElement("tr");
> +          last_row.appendChild(col(""));
> +          last_row.appendChild(col(obj));
> +          generatePolicy(data[policyName][obj], last_row, 2, new_cont, true);
> +        } else {
> +          let row = document.createElement("tr");
> +          row.appendChild(col(""));
> +          row.appendChild(col(obj));
> +          generatePolicy(data[policyName][obj], row, 2, new_cont, false);
> +        }

same simplification could be applied here

Comment 52

[Bram Pitoyo [:bram]]

If there’s a Mac try build that I can install, I’m happy to provide a bit more CSS tweaks.

Comment 53

[Kanika Saini]

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

 
> ::: browser/components/enterprisepolicies/content/aboutPolicies.js:71
> (Diff revision 11)
> > +          let row = document.createElement("tr");
> > +          row.appendChild(col(""));
> > +          generatePolicy(data[policyName][obj], row, 1, new_cont, false, true);
> > +        }
> > +      }
> > +    } else if (schema.properties[policyName].type == "object" && Object.keys(data[policyName]).length > 0) {
> 
> out of curiosity, what is the case that the
> `Object.keys(data[policyName]).length > 0` is protecting from?

While an active policy might would not be empty (hence, removed that), it's needed in getPolicy function so that this - https://imgur.com/a/p1w9Aij doesn't happen.

Comment 54

[Kanika Saini]

Comment on attachment 8989239 [details]
Bug 1472528 - Design and implementation of about:policies page

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254182/diff/11-12/

Comment 55

[Kanika Saini]

Comment on attachment 8989239 [details]
Bug 1472528 - Design and implementation of about:policies page

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254182/diff/12-13/

Comment 56

[Francesco Lodolo [:flod]]

(In reply to :Felipe Gomes (needinfo me!) from comment #46)
> I thought Fluent was still meant to be contained in about:preferences. I
> didn't know we're supposed to start using it elsewhere now.

Just to confirm: can we switch to Fluent and drop the DTD file? It should be relatively straightforward to do, since it's only static labels.

Comment 57

[:Felipe Gomes (needinfo for replies!)]

(In reply to Francesco Lodolo [:flod] from comment #56)
> (In reply to :Felipe Gomes (needinfo me!) from comment #46)
> > I thought Fluent was still meant to be contained in about:preferences. I
> > didn't know we're supposed to start using it elsewhere now.
> 
> Just to confirm: can we switch to Fluent and drop the DTD file? It should be
> relatively straightforward to do, since it's only static labels.

Kanika is taking a look at it :)


Kanika, here's a very quick guide. You just need to create a new .ftl instead of the .dtd file. On IRC, Gandalf suggested creating the file at this path:

browser/locales/en-US/browser/aboutPolicies.ftl

Then, in the html file, you need to include these two lines:

https://searchfox.org/mozilla-central/rev/0f4d50ff5211e8dd85e119ef683d04b63062ed90/browser/components/preferences/applicationManager.xul#19-20

(changing the path for your file)


And use data-l10n-id to specify the string to be used instead of the "&stringname;" style from .dtd

Comment 58

[Francesco Lodolo [:flod]]

(In reply to :Felipe Gomes (needinfo me!) from comment #57)
> (In reply to Francesco Lodolo [:flod] from comment #56)
> > (In reply to :Felipe Gomes (needinfo me!) from comment #46)
> > > I thought Fluent was still meant to be contained in about:preferences. I
> > > didn't know we're supposed to start using it elsewhere now.
> > 
> > Just to confirm: can we switch to Fluent and drop the DTD file? It should be
> > relatively straightforward to do, since it's only static labels.
> 
> Kanika is taking a look at it :)

Great, thanks!

P.S. You can also remove the change to browser/locales/jar.mn. Any .ftl file in that path is covered by line 10-11

Comment 59

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8989239 [details]
Bug 1472528 - Design and implementation of about:policies page

https://reviewboard.mozilla.org/r/254182/#review269270

Ok. All the code looks great. Let's just try to change the strings to the new format.

Reminder: flod will need to review the patch for the string approvals.

Other improvements and CSS tweaks will be left for follow up bugs

Comment 60

[Kanika Saini]

Comment on attachment 8989239 [details]
Bug 1472528 - Design and implementation of about:policies page

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254182/diff/13-14/

Comment 61

[:Felipe Gomes (needinfo for replies!)]

Flod for the .ftl additions


We'll leave changing the document type from .xhtml to .html on a separate bug

Comment 62

[Francesco Lodolo [:flod]]

Comment on attachment 8989239 [details]
Bug 1472528 - Design and implementation of about:policies page

https://reviewboard.mozilla.org/r/254182/#review269404

It looks good, only some nit.

I would also try to avoid spacing strings to much in the FTL, you can group them logically. For example

```
# This Source Code Form is subject to the terms of the Mozilla Public
# License, v. 2.0. If a copy of the MPL was not distributed with this
# file, You can obtain one at http://mozilla.org/MPL/2.0/.

about-policies-title = About Policies

# 'Active' is used to describe the policies currently active
active-policies-tab = Active
errors-tab = Errors
documentation-tab = Documentation

policy-name = Policy Name
policy-value = Policy Value
policy-errors = Policy Errors
```

::: browser/components/enterprisepolicies/content/aboutPolicies.css:116
(Diff revision 14)
> +  vertical-align: middle;
> +  width: 14px;
> +}
> +
> +.icon.machine-only {
> +  background-image: url("chrome://browser/skin/developer.svg");

passing by nit: looks like these 3 rules have inconsistent indentation

::: browser/components/enterprisepolicies/content/aboutPolicies.js:38
(Diff revision 14)
> +  return column;
> +}
> +
> +/*
> + * This function generates the Active Policies content to be displayed by calling
> + * a recursive function called generatePolicy() according the policy schema.

nit: according **to**

::: browser/components/enterprisepolicies/content/aboutPolicies.js:78
(Diff revision 14)
> +  }
> +}
> +
> +/*
> + * This is a helper recursive function that iterates levels of each
> + * policy and formats the content to be display accordingly.

nit: to be display**ed**

::: browser/locales/en-US/browser/aboutPolicies.ftl:5
(Diff revision 14)
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +about-policies = About Policies

Alternative ID that makes clear where the string is used: about-policies-title

::: browser/locales/en-US/browser/aboutPolicies.ftl:7
(Diff revision 14)
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +about-policies = About Policies
> +
> +active-policies-tab = Active

Let's add a comment to this string, since English it's confusing (plural or singular?), e.g.

# 'Active' is used to describe the policies currently active

Comment 63

[Kanika Saini]

Comment on attachment 8989239 [details]
Bug 1472528 - Design and implementation of about:policies page

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254182/diff/14-15/

Comment 64

[Francesco Lodolo [:flod]]

Comment on attachment 8989239 [details]
Bug 1472528 - Design and implementation of about:policies page

https://reviewboard.mozilla.org/r/254182/#review269446

It looks good, thanks!

Comment 65

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebfd031ace65
Design and implementation of about:policies page r=flod,felipe

Comment 66

[Bram Pitoyo [:bram]]

When the change has landed on trunk, will UX get a chance to review the design and submit (likely minor) tweaks?

I’d also be happy to review the design on a try build.

Comment 67

[:Felipe Gomes (needinfo for replies!)]

Yeah, definitely. It just landed on inbound so it will make it easier to experiment with it on Nightly without needing try builds.

Could you file a follow-up bug to track the design tweaks?

Comment 68

[Cosmin Sabou [:CosminS]]

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

Comment 69

[Emil Ghitta, QA [:emilghitta]]

This is verified fixed using Firefox 63.0b5 (BuildId:20180910132416) and Firefox 64.0a1 (BuildId:20180914100156) on Windows 10 64bit, Ubuntu 16.04 64bit and macOS 10.13.6.