1500548 - Start presenting data according to the design mockup

Status: RESOLVED FIXED

(Toolkit :: Preferences, enhancement, P1)

Description

[Vincent Cote (:vcote)]

Add HTML and CSS to the existing aboutconfig.html and aboutconfig.css to make the page look like the approved design mock up.

Comment 1

[:Paolo Amadini]

I'm rewriting the summary to narrow the scope a bit, we can initially focus on the main data that is currently presented separated by "||" in bug 1497637.

In particular, the "modified" state is presented by making the row bold, which as far as I can tell is all that we need visually in the initial version. Later, in fact, we'll also have a way to filter for just modified preferences. We have to provide alternate hidden text so that screen readers can present the same information.

For the data type, we can either continue to print it before the value separated by "||", or maybe we could make it a tooltip.

We shouldn't handle the "locked" state in this bug, but we should file a new bug for it and start working on it later.

I think this would be a good first bug for Matthias to take. The production code can already be implemented on top of the patch in bug 1497637, although the browser-chrome test update has to wait for the test to be written first in the other bug.

Comment 2

[:Paolo Amadini]

(In reply to :Paolo Amadini from comment #1)
> We shouldn't handle the "locked" state in this bug, but we should file a new
> bug for it and start working on it later.

I filed bug 1502856 for this.

Comment 3

[Matthias :Kammueller]

Attachment: Bug 1500548 - started to implement the design from the mockup.r?paolo

Comment 4

[:Paolo Amadini]

Thanks a lot for starting your first contribution to the product code! Looking forward to see this take shape.

The first general comment I have is that there is still much more code than we need for this bug. As a general principle, every bug will add a small part to the final solution, but each one will also be fully functional when it lands in mozilla-central. Each new functionality will have a regression test that checks it works as expected.

In this case, the edit fields don't work, so this doesn't meet the criteria. We have a separate bug for editing, so the right thing to do here is just to remove that part from the patch. For everything else that remains, you'll have to add specific functional tests, but for the moment you can wait until we have a clearer idea of what the final non-test code will look like.

However, even if unintended for the final code, having a way to test a very rough version of the design in an actual product page turned out to be really useful. After applying your patch, the first thing I noticed was that when resizing the window there was a very noticeable drop in frame rate, and resizing felt choppy even on a really fast computer.

So, I started to use our built-in performance tools to track down the issue. I recorded layout times of over 100ms, which is way over what can be reasonable for an in-product page. If this doesn't seem much, we have to consider that on slower machines the time might have to be multiplied by 10 or more. Usually, we want the time on a recent computer to be in the order of a few milliseconds at most, although for this page in particular we may have some leeway.

For reference on the time scale we're talking about, you can take a look at a few performance bugs we have on file:

https://bugzilla.mozilla.org/buglist.cgi?quicksearch=uninterruptible

By trimming down the code bit by bit, I found two root causes:
 - Text fields
 - Grid layout

I'll post some results and more comments later today, but we'll most probably have to redesign things to avoid the two of these.

Comment 5

[:Paolo Amadini]

Attachment: Performance of the original about:config

For reference, this 0.2ms layout time is the performance of the current "about:config" page.

Of course, comparing the new page to this one directly would be like comparing apples to oranges, because this implementation has a virtual view that doesn't do layout for any content that is off-screen. There are also multiple JavaScript function calls during resize that are not accounted for.

The new page, in its current version, renders just about 3,500 items, so this has to be taken into account. We already know that implementing a full virtual view just for this page would require too much effort, so in the final version we can apply more creative solutions to improve performance, like always limiting the number of results or not rendering the list to start with, as well as accepting that performance may be reduced as long as it's not perceivable as a bug.

How much we can get away with is subjective, and the Firefox team will have to make an evaluation, which will happen after our strict definition of MVP is implemented, but before the new page will ride the trains to Beta or Release.

Comment 6

[:Paolo Amadini]

Attachment: Performance of the plain list

This 20ms layout time is our new page from bug 1497637 with a plain list, rendering all 3,500 items.

While this looks 100 times slower if we consider just layout, interestingly the frame rate during resize is still similar to the other page at 10fps. I don't know what this means precisely, but I guess there may be other work in the current "about:config" page, like the JavaScript calls I mentioned, that is not necessarily accounted for if we look just at layout time.

However, going forward this layout time is a baseline we can work from for the sake of this project, and we should try not to increase this time more than what is strictly necessary to implement the MVP, although we know that the page will become slower as we implement more essential features, like the toggle and reset buttons.

Comment 7

[:Paolo Amadini]

Attachment: Performance with grid and text fields

This 102ms layout time is with grid layout and text fields.

Comment 8

[:Paolo Amadini]

Attachment: Performance with grid and no fields

This 58ms layout time is with grid layout and plain text instead of text fields.

Comment 9

[:Paolo Amadini]

Attachment: Performance with table and text fields

This 64ms layout time is with table layout and text fields.

Comment 10

[:Paolo Amadini]

Attachment: Performance with table and no fields

This 38ms layout time is with table layout and plain text instead of text fields.

Comment 11

[:Paolo Amadini]

Attachment: Performance with table and no fields, half the items

When rendering just half the items, performance increases a little better than linearly, with a 16ms time in this case, and also an interesting 13fps.

Comment 12

[:Paolo Amadini]

Attachment: Performance with table and no fields, just 100 items

For reference, when rendering only 100 items, layout time is 1.2ms, and frame rate recovers earlier.

Comment 13

[:Paolo Amadini]

Attachment: Performance with grid and text fields, just 100 items

Comment 14

[:Paolo Amadini]

Comment on attachment 9021185 [details]
Performance with grid and text fields, just 100 items

Layout time is 3.5ms for only 100 items when using grid and text fields.

Comment 15

[:Paolo Amadini]

I have also just reviewed together with Mike Conley some detailed performance profiles comparing the table and text version with the grid and text fields version, where we found evidence that indeed the code involved is from layout, and the faster version is twice as fast.

Hence, our immediate recommendations are:
 1. On this bug, use a table.
 2. When working on bug 1497726, only render a text field for the row that is actively edited.

We also talked about how we can reduce the number of rows in the table, but as I mentioned that will be out of scope for the MVP.

Comment 16

[Matthias :Kammueller]

I updated my patch to a table-version.
I will also update the mock-up to a version without the textfields, just to stay consistent.

Comment 17

[Matthias :Kammueller]

Attachment: Bug 1500548 - started to implement the design from the mockup.r?paolo

Comment 18

[:Paolo Amadini]

Tim, we're looking for an in-content background color for odd rows, to use on all platforms including Windows. This is for a table whose rows don't highlight on selection, but don't have borders, see the attached patch. We have "--in-content-box-background-odd", but it's reset here for Windows:

https://searchfox.org/mozilla-central/rev/7c848ac7630df5baf1314b0c03e015683599efb9/toolkit/themes/windows/global/in-content/common.css#8

Do you think we should add a new variable, just use a hardcoded color in our page for the moment, or something else? High contrast mode is covered either way because the document colors are overridden anyways.

Comment 19

[:Paolo Amadini]

I wrote the above before looking at the latest version, which uses "--in-content-box-background-hover". Would that work for now?

Comment 20

[Tim Nguyen :ntim]

(In reply to :Paolo Amadini from comment #18)
> Tim, we're looking for an in-content background color for odd rows, to use
> on all platforms including Windows. This is for a table whose rows don't
> highlight on selection, but don't have borders, see the attached patch. We
> have "--in-content-box-background-odd", but it's reset here for Windows:
> 
> https://searchfox.org/mozilla-central/rev/
> 7c848ac7630df5baf1314b0c03e015683599efb9/toolkit/themes/windows/global/in-
> content/common.css#8
> 
> Do you think we should add a new variable, just use a hardcoded color in our
> page for the moment, or something else? High contrast mode is covered either
> way because the document colors are overridden anyways.

--in-content-box-background-odd is the correct variable. The reason I've reset it to transparent on Windows is because it matches the behaviour of the native trees (where -moz-oddtreerow == -moz-eventreerow == -moz-field) that we had since a long time. Linux & Mac are the only platforms that have native striping. 

I think using --in-content-box-background-odd would be a better choice than --in-content-box-background-hover. If UX wants striped rows on all platforms including Windows, then it'd be better to remove the Windows override and possibly adjust the variable value if needed.

Comment 21

[Matthias :Kammueller]

The reason for the stripes is that this a rather special page, because the table is very long.
Not having alternating background colors is (in my opinion) confusing.
We already made the "special decision" of having the table on 100% width, so maybe another special decision is not too bad here?

I will ask Amy for feedback.

Comment 22

[Tim Nguyen :ntim]

Just to be clear, I'm fine with having stripes on Windows. I'm simply saying that whatever we're doing, it should be consistent with the other trees in about:preferences.

Comment 23

[Matthias :Kammueller]

Who made the decision in the first place? Was that you or was that some designer?
Maybe it would be good to get their opinion

Comment 24

[Tim Nguyen :ntim]

(In reply to :Matthias Kammüller from comment #23)
> Who made the decision in the first place? Was that you or was that some
> designer?
> Maybe it would be good to get their opinion

Originally, things were just styled to match the platform, it wasn't really any designer's decision.

Comment 25

[Tim Nguyen :ntim]

FYI the current about:config uses --in-content-box-background-odd, so I would suggest using that variable for now and file a follow up to investigate which color to use on Windows.

Comment 26

[Matthias :Kammueller]

I created Bug 1505066 for this issue =)

Comment 27

[Matthias :Kammueller]

Attachment: A11Y structure of the table

I'd stick to the aria-labels for rows since it might be kind of confusing what the row does overwise (the cells have no labels, and the pref name is splitted in different leaves)

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/80ecb731e32e
Start to implement the design from the mockup. r=paolo

Comment 29

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/80ecb731e32e

Comment 30

[Tim Nguyen :ntim]

The hover state should have used --in-content-item-hover not --in-content-box-background-hover :(

Comment 31

[:Paolo Amadini]

(In reply to Tim Nguyen :ntim (please use needinfo?) from comment #30)
> The hover state should have used --in-content-item-hover not
> --in-content-box-background-hover :(

Hm, based on how it's used for the most part, it looks like this variable should be renamed to --in-content-box-background-dimmed or something along that line?

https://searchfox.org/mozilla-central/search?q=--in-content-box-background-hover

This and the change to --in-content-item-hover might be fixed as part of bug 1505066, or in a separate bug.