1483431 - Continue working on the design of about:policies

Status: VERIFIED FIXED

(Firefox :: Enterprise Policies, enhancement)

Description

[Bram Pitoyo [:bram]]

This is a follow-up to bug 1472528, to track further design tweaks that we’ll need to make to about:policies before it ships.

I’ll assign this bug to myself as soon as our new design has landed on Nightly.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Hi Bram, this has landed on Nightly now, so testing it is simple. Just create a string pref called "browser.policies.alternatePath" and point it to the full path of a valid policy json file. (e.g.: "/Users/felipe/Desktop/sample.json")

There are various example json files to use on this folder: https://searchfox.org/mozilla-central/source/browser/components/enterprisepolicies/helpers

Comment 2

[Sören Hentzschel]

Attachment: proposal.png

Hi,

I think a few small design tweaks would make the design much better. So here is my proposal / feedback for the documentation tab:

1. There are alternating row colors but the difference is very hard to see. The difference should be more visible.

2. The text in the rows is not vertically aligned, there is much more padding-bottom than padding-top. I propose to use 8px (current value for padding-bottom) for both.

3. There should also be padding on the left side because otherwise the text starts on the border of the row.

4. Even with the same value for padding-top and padding-bottom in 2. the icon for machine-only policies is not in the middle of the row, but it should be.

5. There should be some space between the policy name and the icon for machine-only policies.

6. There should be a bit more spacing between the caption "Policy Name" and the table with the policies.

7. (not visible in the screenshot) URls should be clickable.

Comment 3

[Sören Hentzschel]

Attachment: errors feedback

8. The description of the policies "DisableProfileImport" and "DisableProfileRefresh" are the only two without a "." at the end, so it should be added.

---

Feedback / suggestions for the errors tab:

1. The points 1, 2, 3 and 6 of the "documentation" feedback also applies here.

2. The font should not be monospaced.

3. There is a space missing between the "." after "WebsiteFilter" and "Block".

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Thanks for all the great feedback, Sören. Would you be interested in taking this bug to implement some of these suggestions? I'd be more than happy to mentor and review the patches

Comment 5

[Sören Hentzschel]

I could talke this bug but mozilla-central doesn't compile for me.

error: failed to run custom build command for `style v0.0.1 (file:///Users/cadeyrn/localhost/mozilla/mozilla-central/mozilla-central/servo/components/style)`
17:28.69 process didn't exit successfully: `/Users/cadeyrn/localhost/mozilla/mozilla-central/mozilla-central/obj-x86_64-apple-darwin18.0.0/toolkit/library/release/build/style-2b2dacf980cdf481/build-script-build` (exit code: 101)

First I would need help to solve the build issue. This bug is probably not the best place for this. What is the best place for this kind of problem?

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Usually #developers or #build on IRC is the fastest way for that. I'm not a build expert so I can't help with that specific problem.

However, I can offer an easy solution: since this bug will only be modifying HTML/CSS/JS files, you don't really need to build the binary components for Firefox. You can use artifact builds which is what most front-end developers use nowadays. More details here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Artifact_builds

But in summary, all you need to do is add `ac_add_options --enable-artifact-builds` in your mozconfig file

Comment 7

[Sören Hentzschel]

Thanks, that helped. I can work on the design improvements now.

Comment 8

[Sören Hentzschel]

Attachment: policies-active.png

I implemented all the improvements from comment 2 and comment 3 except point 7 (URLs should be clickable). I also improved the display of the active policies. I think with my improvement it looks much cleaner. The old look had a lot of problems (background color not across all columns, missing border below the "Homepage" policy in the example which you can see in my screenshot, borders not across all columns in policies with array fields also looked weird).

I will attach screenshots of all three sections for easy comparison.

Before I submitting a patch for feedback please let me know how I should proceed. For my first contribution I used MozReview. Is Phabricator the new preferred way? If yes, I need instructions how to use Phabricator.

Comment 9

[Sören Hentzschel]

Attachment: policies-documentation.png

Comment 10

[Sören Hentzschel]

Attachment: policies-errors.png

Comment 11

[:Felipe Gomes (needinfo for replies!)]

Yeah, MozReview is deprecated and doesn't accept any new submissions. Phabricator is the preferred way now. Here are the docs:
https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html

Comment 12

[Sören Hentzschel]

Attachment: Bug 1483431 - improve design of about:policies

Comment 13

[Sören Hentzschel]

Sorry, I used Phabricator the first time and accidentally added changes to build/unix/run-mozilla.sh and tools/rb/fix_macosx_stack.py which have nothing to do with this bug.

Is there an easy way to remove these file from the review request?

Comment 14

[Bram Pitoyo [:bram]]

Attachment: [e-mail]

(In reply to Sören Hentzschel from comment #10)
> Created attachment 9002279 [details]
> policies-errors.png

Hi Sören,

Thanks so much for all your design alterations. I think that all of them were on point (shading colour values are accurate, more padding is a great idea), and have gotten us 99% of the way there!

All I have are tiny tweaks:

1. For ‘simpler’ tables where most properties only have one child (e.g. documentation and error sections) – either:

  - Just use background colour, without horizontal rulers
  - Or just use horizontal rulers, without background colour

In my example, I’ve used background colour, but feel free to try out horizontal rulers. No need to do both. For more complicated tables (active policies sub-page), we can use both.


2. To increase separation and enhance readability, the inner padding of all text within the table should at least be 1 rem (we don’t want to make this too high – then there will be too much scrolling).

Comment 15

[Bram Pitoyo [:bram]]

3. Consider having a hover effect over table row, if we think that it will increase usability. But if it won’t, then don’t worry about it. Having a hover effect has a risk of falsely implying to users that the table row can be selected.

(I forgot to include this tweak in my last comment. Sorry about that.)


I’ve looked at tables in these design systems often (some are newer than others). Feel free to look there for your inspirations, too:

1. https://material.io/design/components/data-tables.html

2. http://www.carbondesignsystem.com/components/data-table

3. http://designsystem.morningstar.com/components/data-tables.html#tree-table (good example of a table with multiple child elements; each child don’t all need to occupy a new column).

4. https://vmware.github.io/clarity/documentation/v0.11/tables (good example of multi-line table cells; I suspect this will come up for us when our text get too long)

5. https://polaris.shopify.com/components/lists-and-tables/data-table (good example of a table with 5 columns; our

Comment 16

[Bram Pitoyo [:bram]]

(In reply to :Felipe Gomes (needinfo me!) from comment #1)
> Hi Bram, this has landed on Nightly now, so testing it is simple. Just
> create a string pref called "browser.policies.alternatePath" and point it to
> the full path of a valid policy json file. (e.g.:
> "/Users/felipe/Desktop/sample.json")

Hi Felipe, thanks a lot for the guidance. I was able to download all the example JSON files. However, even after creating a new string pref in about:config and inputting the full path, I still get an error that said: “Error parsing JSON file”.

Do I need to do some extra steps?

Comment 17

[Bram Pitoyo [:bram]]

Comment on attachment 9002277 [details]
policies-active.png

This is nearly good to go.

In addition to tweaks on comment 14, this page needs some additional tweaks:

1. Since we already give every other policy a darker background, there’s no need for a horizontal line separator.

2. However, we do need a line separator between each value “parent” in each policy. Not between all the value’s “children”, though. And note that separators don’t extend all the way to the Policy Name column.

  * On a light background, the ruler colour should be grey-20
  * On a darker background, the ruler colour should be grey-10

What do you think?

Comment 18

[Bram Pitoyo [:bram]]

Attachment: [e-mail]

Comment 19

[Sören Hentzschel]

Thanks for your feedback, Bram! Before I start implementing the changes later today I have one question.

> Consider having a hover effect over table row, if we think that it will increase usability. But if it won’t, then don’t worry about it. Having a hover effect has a risk of falsely implying to users that the table row can be selected.

Who will decide if it will increase the usability / if it should be implemented? ;-) Or should I just implement it and the decision will be made later, depending on how it feels in a real build?

Comment 20

[:Felipe Gomes (needinfo for replies!)]

(In reply to Sören Hentzschel from comment #19)
> Who will decide if it will increase the usability / if it should be
> implemented? ;-) Or should I just implement it and the decision will be made
> later, depending on how it feels in a real build?

Yeah. If it's not too much work, that's usually what we do

Comment 21

[:Felipe Gomes (needinfo for replies!)]

In the meantime, while Sören works on the latest tweaks suggested by Bram, I sent the current patch to tryserver so that Bram can take a look at the modifications already made:

https://tools.taskcluster.net/index/gecko.v2.try.revision.15c14add9e4dd0456fe3eec3bcb23ae066679ca6.firefox


(In reply to Bram Pitoyo [:bram] from comment #16)
> Hi Felipe, thanks a lot for the guidance. I was able to download all the
> example JSON files. However, even after creating a new string pref in
> about:config and inputting the full path, I still get an error that said:
> “Error parsing JSON file”.

If you've made any modifications to the file, can you put it in an online JSON validator tool? JSON is very finicky in where it accepts commas etc..

Otherwise, it probably is not finding the file in the right path.. There's a pref called browser.policies.loglevel that you can change to "debug" to get more info.

If it doesn't work, ping me on Slack and I can help figure it out

Comment 22

[Bram Pitoyo [:bram]]

(In reply to Sören Hentzschel from comment #19)
> Who will decide if it will increase the usability / if it should be
> implemented? ;-) Or should I just implement it and the decision will be made
> later, depending on how it feels in a real build?

I think you should implement it – if it’s not too hard, and then we’ll see how it feels in the build.

Here’s a version of the Active Policies page that has the hover effect. Also notable: I’ve chosen to separate each policy only by line, rather than use alternating row colour.

What do you think?

https://dzwonsemrish7.cloudfront.net/items/0c341U050s223r3O1H3b/active-policies-row-hover-effect.mov

(Thanks a lot, Felipe, for helping me get the JSON file working!)

Comment 23

[Sören Hentzschel]

Attachment: Bug 1483431 - improve design of about:policies

Comment 25

[Sören Hentzschel]

Sorry for the delay. I implemented the requested changes.

> In my example, I’ve used background colour, but feel free to try out horizontal rulers. No need to do both.

I tried both and decided for alternating background colors because in my opinion it looked much better. But feel free to request a change if you disagree.

@Felipe: Thank you for your comments in Phabricator. I already marked the old request as abandoned a few days ago because it was a big mess with unwanted changes and I restarted with a new request. Now I don't know if it still makes sense to answer in the old Phabricator request so I will reply to one of the questions here:

> Why is the color_class only added in the isFirstRow case?

It's only added in the isFirstRow case because I didn't want to change the background color within a policy. I use the "even"/"odd" class only on the "top level" of each policy.

All your other feedback has been addressed.

Comment 26

[Bram Pitoyo [:bram]]

(In reply to Sören Hentzschel from comment #25)

Thanks for implementing these changes so quickly, Sören! As soon as they land in Nightly, I’d be able to check them out and provide feedback.

Comment 27

[:Felipe Gomes (needinfo for replies!)]

I compiled the patch locally to play with it and I noticed two small problems:

- if you use the Bookmarks policy (there's a sample_bookmarks.json file in the tree), the rows aren't colored correctly. I think there are other cases that are missing the .classList.add(color_class)

- The array separators have some issues too. I don't know what's the final desired behavior, but I was using the FlashPlugin policy to test, and when there's only one item in each Allow and Block arrays, a separator shows up, but if I add more items on the these arrays, the separator between Allow/Block no longer shows up.


Oh and I'd just like to mention that I really liked the first style with both the colored rows and the separated lines. I'll note that Bram said in comment 14 that this is fine for the "Active Policies" table  (he only suggested not having both for the simpler Documentation and Errors tables).  But this is just my personal taste, no need to go back if you prefer.


One other suggestion: The HTML spec allows multiple <tbody>s inside a table. I think the even/odd rules can be greatly simplified if we do one <tbody> per policy, so then it's only a matter of doing the same :nth-child(odd) in CSS.

I tried a quick hack here and it looks like it wouldn't be hard to make changes for this. However, it is more work, so if you prefer to leave this for a separate bug, that's fine.

Comment 28

[Bram Pitoyo [:bram]]

Comment on attachment 9002277 [details]
policies-active.png

We’re nearly there. See comment 14, comment 17 and comment 27 for the latest feedback. Thanks, Sören!

Comment 29

[Sören Hentzschel]

Hi Felipe,

I tried to fix the issues but it's not that easy and I need your help. The generation of the rows of active policies is already broken before my changes. I'll attach a screenshot which shows the problems without my patch. As you can see I marked some rows with "missing border". It's not about the style (the missing border) itself, the screenshot shows that there are underlying issues with the current implementation which has to been fixed before I can proceed to improve the style. Unfortunately I was not able to fix these issues. Can you help?

Comment 30

[Sören Hentzschel]

Attachment: issues.png

Comment 31

[:Felipe Gomes (needinfo for replies!)]

Attachment: aboutpoliciesfixup.patch

Hi Sören, I figured out what the issues that you pointed out were, and here's a patch that fixes them. Feel free to incorporate this patch directly into yours.

Comment 32

[Sören Hentzschel]

Thank you, this helps a lot! A will have an updated patch soon.

Comment 33

[Sören Hentzschel]

> A will have an updated patch soon.

I meant: *I* will have an updated patch soon. ;-)

I updated the Phabricator request. Is it expected that there is no update in Bugzilla or did I something wrong?

Comment 34

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 9003228 [details]
Bug 1483431 - improve design of about:policies

:Felipe Gomes (needinfo me!) has approved the revision.

Comment 35

[:Felipe Gomes (needinfo for replies!)]

(In reply to Sören Hentzschel from comment #33)
> > A will have an updated patch soon.
> 
> I meant: *I* will have an updated patch soon. ;-)
> 
> I updated the Phabricator request. Is it expected that there is no update in
> Bugzilla or did I something wrong?

Yeah, it's expected. Phabricator sends a separate e-mail request, which I got. I just compiled it locally and it looks awesome!

Can you post the final screenshots here for future reference?

I'll land the patch as it is now and if there are more visual tweaks to do we can do in a new bug, in order to not make this a never-ending bug.


I noticed one edge case that already exists but these changes don't fully fix them.. I'll file a separate bug with more details and CC you to see if you can work on that too

Comment 36

[Sören Hentzschel]

Attachment: active policies

(In reply to :Felipe Gomes (needinfo me!) from comment #35)
> (In reply to Sören Hentzschel from comment #33)
> > I updated the Phabricator request. Is it expected that there is no update in
> > Bugzilla or did I something wrong?
> 
> Yeah, it's expected. Phabricator sends a separate e-mail request, which I
> got. I just compiled it locally and it looks awesome!

Good to know, thank you!

> Can you post the final screenshots here for future reference?

Yes, I will post the final screenshots.

Comment 37

[Sören Hentzschel]

Attachment: documentation

Comment 38

[Sören Hentzschel]

Attachment: errors

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd82664c5a14
improve design of about:policies. r=felipe

Comment 40

[Raul Gurzau (:RaulG)]

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

Comment 41

[Bram Pitoyo [:bram]]

Attachment: active-policies-feedback.png

(In reply to Sören Hentzschel from comment #36)
> Created attachment 9005504 [details]
> active policies

Hi Sören, the design of the new pages looks really good!

The only thing we need to erase is the horizontal line divider between each policy, since the background shading already takes care of that.

Then we’re good to go. Thanks!

Comment 42

[Sören Hentzschel]

I can remove the horizontal line dividers again, it's a really small change. But Felipe said in comment #27:

> Oh and I'd just like to mention that I really liked the first style with both the colored rows and the separated lines. I'll note that Bram said in comment 14 that this is fine for the "Active Policies" table  (he only suggested not having both for the simpler Documentation and Errors tables). But this is just my personal taste, no need to go back if you prefer.

There are two different opinions. Could you clarify who has the last word? :D

Comment 43

[:Felipe Gomes (needinfo for replies!)]

(In reply to Sören Hentzschel from comment #42)
> I can remove the horizontal line dividers again, it's a really small change.
> But Felipe said in comment #27:
> 
> > Oh and I'd just like to mention that I really liked the first style with both the colored rows and the separated lines. I'll note that Bram said in comment 14 that this is fine for the "Active Policies" table  (he only suggested not having both for the simpler Documentation and Errors tables). But this is just my personal taste, no need to go back if you prefer.
> 
> There are two different opinions. Could you clarify who has the last word? :D


I really liked the visual of the line divider, so if Bram doesn't mind, I vote for keeping it!

Comment 44

[Bram Pitoyo [:bram]]

(In reply to Sören Hentzschel from comment #42)
> There are two different opinions. Could you clarify who has the last word? :D

(In reply to :Felipe Gomes (needinfo me!) from comment #43)
> I really liked the visual of the line divider, so if Bram doesn't mind, I
> vote for keeping it!

I don’t mind keeping the divider around! I guess this means that no further change is needed.

I had a chance to test the design on the latest Nightly build. It looked and felt great to use. Thank you, Sören and Felipe.

Comment 45

[Emil Ghitta, QA [:emilghitta]]

This bug was covered by the overall testing efforts invested in the About:Policies feature.

This is verified fixed using Firefox 63.0b14 (BuildId:20181011200118) on Windows 10 64bit and Ubuntu 16.04 64bit.