1172454 - The about:telemetry environment section should have expandable subsections

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Georg Fritzsche [:gfritzsche]]

The environment section in about:telemetry shows a lot of data.
To make viewing easier we could make subsections (build, settings, ...) expandable.

The environment data is added as children of this node:
https://hg.mozilla.org/mozilla-central/annotate/7d4ab4a9febd/toolkit/content/aboutTelemetry.xhtml#l102

The rendering there happens dynamically here:
https://hg.mozilla.org/mozilla-central/annotate/7d4ab4a9febd/toolkit/content/aboutTelemetry.js#l490

We could just put each subsection in a different element or table and use |setHasData()| on those:
https://hg.mozilla.org/mozilla-central/annotate/7d4ab4a9febd/toolkit/content/aboutTelemetry.js#l1322

Comment 1

[Georg Fritzsche [:gfritzsche]]

simplyblue, would you be interested in working on this bug?

Comment 2

[simplyblue24]

Yes, I like to work on this.Please assign the bug to me.Thanks.!

Comment 3

[Georg Fritzsche [:gfritzsche]]

Done, thanks. Should i add additional detail on suggested steps here or would you like to try yourself on this first?

Comment 4

[simplyblue24]

I'll try myself first :)

Comment 5

[simplyblue24]

Attachment: 1172454.diff

I have made some changes, can you please review them.Thanks

Comment 6

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8689684 [details] [diff] [review]
1172454.diff

Review of attachment 8689684 [details] [diff] [review]:
-----------------------------------------------------------------

Good start!

::: toolkit/content/aboutTelemetry.js
@@ +549,5 @@
> +      var rows = table.getElementsByTagName("tr").length;
> +      if(rows > 0) {
> +        dataSection.setAttribute("class","data-section has-data");
> +      } else {
> +        dataSection.setAttribute("class","data-section");

I think we should use a new "data-subsection" class here so we can style this independently of the top-level sections.

@@ +552,5 @@
> +      } else {
> +        dataSection.setAttribute("class","data-section");
> +      }
> +
> +

Nit: Only one empty line between code sections, same below.

@@ +555,5 @@
> +
> +
> +      let checkbox = document.createElement("input");
> +      checkbox.setAttribute("type","checkbox");
> +      checkbox.setAttribute("class","state");

I don't think a checkbox here is useful, we can just leave it out.

@@ +559,5 @@
> +      checkbox.setAttribute("class","state");
> +
> +      let sectionName = document.createElement("h1");
> +      sectionName.setAttribute("class","section-name");
> +      sectionName.appendChild(document.createTextNode(section));

I think logically this should be a <h2>, we already have <h1> for the main data sections ("Environment Data" etc.).

@@ +564,5 @@
> +
> +
> +      let toggleCaption = document.createElement("span");
> +      toggleCaption.setAttribute("class","toggle-caption");
> +      toggleCaption.appendChild(document.createTextNode("caption"));

For this to have a proper caption ("Click to toggle section"), you will need to:
* add a new string to aboutTelemetry.properties
* load the string with bundle.GetStringFromName()
* use that string for the text node

You can see how we add an event listener for toggling the main data sections here:
https://dxr.mozilla.org/mozilla-central/rev/a2f83cbe53ac4009afa4cb2b0b8f549289b23eeb/toolkit/content/aboutTelemetry.js#1457

Note that we now show two captions per subsection, we should probably remove the table caption:
https://dxr.mozilla.org/mozilla-central/rev/a2f83cbe53ac4009afa4cb2b0b8f549289b23eeb/toolkit/content/aboutTelemetry.js#532

Comment 7

[simplyblue24]

Attachment: 1172454.patch

I have made the appropriate changes. 

If I'm not wrong the checkbox is necessary as it is used in |toggleSection| https://dxr.mozilla.org/mozilla-central/source/toolkit/content/aboutTelemetry.js#1362 . 

I have attached the event listeners, but still the section wont toggle on clicking,but the eventlistener callback function is getting called. Thanks.

Comment 8

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8690195 [details] [diff] [review]
1172454.patch

Thanks, i'm taking a look tomorrow.

Comment 9

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8690195 [details] [diff] [review]
1172454.patch

Review of attachment 8690195 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, i see what the checkbox is good for now, thanks.

For the event listener you should start to look for errors:
If you open the "browser console" (Tools -> Web Developer -> Browser Console) you will see a JavaScript error and information on which line it is happening.
You might have to enable the "Chrome developer tools" first: in about:config set "devtools.chrome.enabled" to true.

::: toolkit/content/aboutTelemetry.js
@@ +551,5 @@
> +      }
> +
> +      let checkbox = document.createElement("input");
> +      checkbox.setAttribute("type","checkbox");
> +      checkbox.setAttribute("class","state");

You could just give it the class name "statebox" too - then it should be hidden as well.

@@ +556,5 @@
> +
> +      let sectionName = document.createElement("h2");
> +      sectionName.setAttribute("class","section-name");
> +      sectionName.appendChild(document.createTextNode(section));
> +      sectionName.addEventListener("click", toggleSection, false);

Just re-using the toggleSection function might not work right away.
We could either adjust how the structure of the subsection looks (so that the function works on them too) or use a new function that works specifically for the subsections.

Comment 10

[Georg Fritzsche [:gfritzsche]]

Did you find the issue? If not, i'm happy to help with more information as needed :)

Comment 11

[simplyblue24]

Hey, actually I'm quite stuck. Can you  please provide a pointer. I'm not understanding why this 
|toggleSection| is not working. https://pastebin.mozilla.org/8853164

Comment 12

[Georg Fritzsche [:gfritzsche]]

With your uploaded patch here, looking at my "browser console" i get the following error when clicking on a subsection: 
> TypeError: statebox is undefined - aboutTelemetry.js:1407:22

... that is this line of code:
> statebox.checked = parentElement.classList.contains("expanded");

Looking at your changes, i see that you use "state" instead of "statebox":
> checkbox.setAttribute("class","state");
... changing this to "statebox", the look is already as expected and no more errors show up.
Clicking the caption i don't see the subsection hiding/showing, but checking with the developer tools, i can see that "expanded" is already correctly added to the class list.
So, that part works, but the CSS is apparently not right yet.

We are creating the div that contains the data with this:
> let data = document.createElement("div");
... we should add the "data" class to it, so we can hide/show it similar to the CSS rules that hide/show the top-level sections.
We could e.g. add:
> .data-subsection:not(.expanded) .data {
>  display: none;
> }

That's not exactly the final version yet, but it should get you back on track :)

Comment 13

[simplyblue24]

Attachment: Bug1172454.patch

Hi,can you review the changes.Please let me know of any further changes.Thanks

Comment 14

[simplyblue24]

Attachment: Bug1172454.patch

Hi,  please review the patch. Let me know of any changes. :)

Comment 15

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8693611 [details] [diff] [review]
Bug1172454.patch

Review of attachment 8693611 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this is getting close!
Below are mostly style and cleanup comments.

::: toolkit/content/aboutTelemetry.css
@@ +55,5 @@
>    margin-left: 5px;
>    margin-bottom: 10px;
>  }
>  
> +.data-section,.data-subsection {

Nit: There should be a whitespace after the ",".
Things will be more readable if we put a linebreak after the "," so we have one selector per line.

The same goes for the other CSS changes below.

::: toolkit/content/aboutTelemetry.js
@@ +551,5 @@
> +      }
> +
> +      let checkbox = document.createElement("input");
> +      checkbox.setAttribute("type","checkbox");
> +      checkbox.setAttribute("class","statebox");

So, we were adding the checkbox to remember the hidden/visible state of the subsections across page reloads.
From testing this change in action, i realize that the browser doesn't remember the state of dynamically added checkboxes, only those specified in the xhtml file.
So... we could either:
a) start to add subsections to the xhtml file (more work, additional maintenance needed), or
b) just remove the checkbox for now (hidden/visible state not remembered over reloads)

Lets do b) now and we can still look into a) later if needed.

@@ +553,5 @@
> +      let checkbox = document.createElement("input");
> +      checkbox.setAttribute("type","checkbox");
> +      checkbox.setAttribute("class","statebox");
> +
> +      let sectionName = document.createElement("h2");

Lets put a comment before this to keep things more readable:
// Create section heading.

With further captions below this separate the code into distinct sections:
// First do this part.
...
// Then do this other part.
...
// Etc.

@@ +560,5 @@
> +      sectionName.addEventListener("click", toggleSection, false);
> +
> +      let toggleCaption = document.createElement("span");
> +      toggleCaption.setAttribute("class","toggle-caption");
> +      toggleCaption.appendChild(document.createTextNode(" "+bundle.GetStringFromName("environmentDataSubsectionToggle")));

This line is too long, suggesting:
let toggleText = bundle.Get...
toggleCaption.appendChild(toggleText);

@@ +565,5 @@
> +      toggleCaption.addEventListener("click", toggleSection, false);
> +
> +      let emptyCaption = document.createElement("span");
> +      emptyCaption.setAttribute("class","empty-caption");
> +      emptyCaption.appendChild(document.createTextNode(" "+bundle.GetStringFromName("environmentDataSubsectionEmpty")));

This line is too long, suggesting:
let emptyText = bundle.Get...
emptyCaption.appendChild(emptyText);

@@ +567,5 @@
> +      let emptyCaption = document.createElement("span");
> +      emptyCaption.setAttribute("class","empty-caption");
> +      emptyCaption.appendChild(document.createTextNode(" "+bundle.GetStringFromName("environmentDataSubsectionEmpty")));
> +
> +      let data = document.createElement("div");

For keeping things more readable, lets put another comment before this:
// Create data container.

@@ +571,5 @@
> +      let data = document.createElement("div");
> +      data.setAttribute("class","subsection-data subdata");
> +      data.appendChild(table);
> +
> +      dataSection.appendChild(checkbox);

Before this, add a comment:
// Append elements.

@@ +1393,5 @@
>   * changes caption on the toggle text
>   */
>  function toggleSection(aEvent) {
>    let parentElement = aEvent.target.parentElement;
> +  if (!parentElement.classList.contains("has-data") && !parentElement.classList.contains("has-subdata")) {

This line is too long, lets do a line-break after the "&&".

Comment 16

[Georg Fritzsche [:gfritzsche]]

Hi simplyblue24, did you run into any problems with solving this?
If you get stuck, just let me or Alessio know, we are happy to help.

Comment 17

[simplyblue24]

Attachment: 1172454.patch

Hey, can you please review the patch. If i remove off the "checkbox" the toggle is not working, so i kept it, as it is.

Comment 18

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8698361 [details] [diff] [review]
1172454.patch

Review of attachment 8698361 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks pretty close, this will be pretty helpful for us.

We can definitely remove the checkbox though. There are two things needed to remove it:
* toggleSection() needs to handle cases where there is no checkbox, i.e. where |!statebox|
* in EnvironmentData.render() the |checkbox| code needs to be removed

Any JS errors with that should show up in the "browser console" (found in Tools -> Web Developer) or in the terminal.

Note that bug 1172459 is just landing, this will have one or two smaller conflicts with the code changes here.
Ideally you re-base your patch based on the changes there :)

::: toolkit/content/aboutTelemetry.js
@@ +546,5 @@
>        }
>  
> +      let dataSection = document.createElement("section");
> +      let rows = table.getElementsByTagName("tr").length;
> +      if(rows > 0) {

We can just check sectionData.size here instead of querying the table rows.

@@ +549,5 @@
> +      let rows = table.getElementsByTagName("tr").length;
> +      if(rows > 0) {
> +        dataSection.setAttribute("class","data-subsection has-subdata");
> +      } else {
> +        dataSection.setAttribute("class","data-subsection");

We can use .classList instead:
dataSection.classList.add("data-subsection");
if (sectionData.size) {
  dataSection.classList.add("has-subdata");
}

@@ +562,5 @@
> +      sectionName.setAttribute("class","section-name");
> +      sectionName.appendChild(document.createTextNode(section));
> +      sectionName.addEventListener("click", toggleSection, false);
> +
> +      // First do this part

// Create caption for toggling the subsection visibility.

@@ +566,5 @@
> +      // First do this part
> +      let toggleCaption = document.createElement("span");
> +      toggleCaption.setAttribute("class","toggle-caption");
> +      let toggleText = bundle.GetStringFromName("environmentDataSubsectionToggle");
> +      toggleCaption.appendChild(document.createTextNode(" "+toggleText));

Nit: Add a space around the "+".

@@ +569,5 @@
> +      let toggleText = bundle.GetStringFromName("environmentDataSubsectionToggle");
> +      toggleCaption.appendChild(document.createTextNode(" "+toggleText));
> +      toggleCaption.addEventListener("click", toggleSection, false);
> +
> +      // Then do this other part

// Create caption for empty subsections.

Comment 19

[simplyblue24]

Attachment: 1172454.patch

Hi, can you review the patch,  I think there are some changes that need to done. Just a gut feeling :P. Thanks.

Comment 20

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8700321 [details] [diff] [review]
1172454.patch

Review of attachment 8700321 [details] [diff] [review]:
-----------------------------------------------------------------

If you test-run this, you'll see that the addons section can not be hidden now and looks different in style.
This is because it is after the loop where your changes are - we have to address this too.

Note that i'm on vacation for two weeks, further reviews should go to Alessio Placitelli (Dexter on IRC).

::: toolkit/content/aboutTelemetry.js
@@ +549,5 @@
>        }
>  
> +      let dataSection = document.createElement("section");
> +      dataSection.classList.add("data-subsection");
> +      if(sectionData.size > 0) {

Nit: space after the "if"

@@ +553,5 @@
> +      if(sectionData.size > 0) {
> +        dataSection.classList.add("has-subdata");
> +      }
> +
> +      // Create section heading

We should move the following code you added into a helper function (say appendSubsection()).
Then we can use that function here and for the addon data below.

@@ +1520,5 @@
>    parentElement.classList.toggle("expanded");
>  
>    // Store section opened/closed state in a hidden checkbox (which is then used on reload)
>    let statebox = parentElement.getElementsByClassName("statebox")[0];
> +  if(statebox != null) {

Nits: Add a space after the "if" & no need to explicitly check for null:
if (statebox) {
  statebox.checked = ...

Comment 21

[simplyblue24]

Happy Christmas Georg :) Have a great one :)

Comment 22

[simplyblue24]

Hi, sorry to ask, can you please provide more info,on how to incorporate the new "addon section" changes. Thank you :)

Comment 23

[Georg Fritzsche [:gfritzsche]]

No worries, happy to help out here or on IRC.
Do you mean how to adopt your changes to the changes that happened to that code recently?

Comment 24

[simplyblue24]

Hi, the current structure for subsection is :  section->h2->span-span->div (this div contains a table which contains the subsection data).

The changes are : div(id=addons-data)->table->table->table->table->table->table (there are multiple tables)

Should each of the table should be a subsection?,if so each table should be wrapped in "section->h2->span-span->div->({each new table})", just like before.

Hope it is clear :), new changes have multiple sections of data, should each be a subsection?

Comment 25

[Georg Fritzsche [:gfritzsche]]

Ah, thanks for the details :)
Lets just treat the addons section as one expandable subsection, that should be fine here.

Comment 26

[simplyblue24]

Attachment: 1172454.patch

Hi, can you please review the patch.Thank you :)

Comment 27

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8707828 [details] [diff] [review]
1172454.patch

Review of attachment 8707828 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this is close.

From a high-level look, let's not duplicate the captions: http://imgur.com/nRZRxEO
There is a few more comments for fixes below, but nothing major at this point.

::: toolkit/content/aboutTelemetry.js
@@ +569,5 @@
>        }
>  
> +       let dataSection = document.createElement("section");
> +       dataSection.classList.add("data-subsection");
> +       this.createSubsection(section,sectionData.size > 0,table,dataSection);

Nit: Add spaces between arguments.
For readability, let's make this:
let hasData = sectionData.size > 0;
this.createSubsection(...);

@@ +575,5 @@
>      }
>  
>      // We use specialized rendering here to make the addon and plugin listings
>      // more readable.
> +    let dataSection = document.createElement("section");

Lets just move all the addon rendering code from here on into createAddonSection(), that should make it cleaner and more readable.

@@ +587,5 @@
>      this.renderKeyValueObject(addons.activeExperiment, addonSection, "activeExperiment");
>      this.renderAddonsObject(addons.activeGMPlugins, addonSection, "activeGMPlugins");
>      this.renderPersona(addons, addonSection, "persona");
> +
> +    this.createSubsection("addons-data",Object.keys(ping.environment.addons).length > 0,addonSection,dataSection);

Nits: Space between arguments & let's have a separate "let hasData = ..." for readability.

@@ +592,5 @@
> +    dataDiv.appendChild(dataSection);
> +  },
> +
> +  createSubsection: function(title,hasSubdata,subSectionData,sectionToAppend) {
> +

Nit: Remove empty line, add spaces between parameters.

@@ +626,5 @@
> +    sectionToAppend.appendChild(sectionName);
> +    sectionToAppend.appendChild(toggleCaption);
> +    sectionToAppend.appendChild(emptyCaption);
> +    sectionToAppend.appendChild(data);
> +

Nit: Remove empty line.

@@ +1546,5 @@
>    parentElement.classList.toggle("expanded");
>  
>    // Store section opened/closed state in a hidden checkbox (which is then used on reload)
>    let statebox = parentElement.getElementsByClassName("statebox")[0];
> +  if(statebox) {

Nit: Spacing, "if (..."

Comment 28

[simplyblue24]

Attachment: 1172454.patch

Hi, Hope this will look good now, please review :). Thank you.

Comment 29

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8708037 [details] [diff] [review]
1172454.patch

Review of attachment 8708037 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks much cleaner.
Let's fix the minor issues below, than this is good to go :)

::: toolkit/content/aboutTelemetry.js
@@ +570,5 @@
>      }
>  
>      // We use specialized rendering here to make the addon and plugin listings
>      // more readable.
> +    this.createAddonSection(dataDiv,ping);

Nit: space between the arguments.

@@ +576,2 @@
>  
> +  createSubsection: function(title, hasSubdata, subSectionData, divToAppend) {

Nit: |divToAppend| is unfortunately named (it sounds like "append this div to some element" while the functions actually appends *to that div*).
I suggest to just name this |dataDiv| here too and explain the parameters in a doc comment (i.e. with @param comments).

@@ +698,5 @@
>      caption.appendChild(document.createTextNode(section));
>      table.appendChild(caption);
>    },
>  
> +  createAddonSection: function(dataDiv,ping) {

Add space between the arguments.

@@ +709,5 @@
> +    this.renderAddonsObject(addons.activeGMPlugins, addonSection, "activeGMPlugins");
> +    this.renderPersona(addons, addonSection, "persona");
> +
> +    let hasAddonData = Object.keys(ping.environment.addons).length > 0;
> +    this.createSubsection("addons-data", hasAddonData, addonSection, dataDiv);

The title should just be "addons".

Comment 30

[simplyblue24]

Attachment: 1172454.patch

Hope this looking good now :) Thanks.

Comment 31

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8708759 [details] [diff] [review]
1172454.patch

Review of attachment 8708759 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, thanks for the persistence :)
Please fix the minor nits below, then we can get this landed (you can mark the updated patch as r+, as it was already reviewed successfully).

::: toolkit/content/aboutTelemetry.js
@@ +583,5 @@
> +    }
> +
> +    // Create section heading
> +    let sectionName = document.createElement("h2");
> +    sectionName.setAttribute("class","section-name");

Nit: Add a space between the arguments.

@@ +589,5 @@
> +    sectionName.addEventListener("click", toggleSection, false);
> +
> +    // Create caption for toggling the subsection visibility.
> +    let toggleCaption = document.createElement("span");
> +    toggleCaption.setAttribute("class","toggle-caption");

Ditto.

@@ +596,5 @@
> +    toggleCaption.addEventListener("click", toggleSection, false);
> +
> +    // Create caption for empty subsections.
> +    let emptyCaption = document.createElement("span");
> +    emptyCaption.setAttribute("class","empty-caption");

Ditto.

@@ +602,5 @@
> +    emptyCaption.appendChild(document.createTextNode(" " + emptyText));
> +
> +    // Create data container
> +    let data = document.createElement("div");
> +    data.setAttribute("class","subsection-data subdata");

Ditto.

Comment 32

[simplyblue24]

Attachment: 1172454.patch

This should be ready now :D

Comment 33

[Georg Fritzsche [:gfritzsche]]

This looks good :)
You can add "checkin-needed" to the keywords field so others can check it in for you.

Side note:
This doesn't need a try run, this only changes about:telemetry, which doesn't have test coverage.
I tested the patch locally though.

Comment 34

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8709984 [details] [diff] [review]
1172454.patch

checkin+ would be used to show that a patch was checked in (this is rarely used).
We use the "checkin-needed" keyword to mark that something needs to be landed.

Comment 35

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/7bc3b5df86f2

Comment 36

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7bc3b5df86f2