Closed Bug 1370513 Opened 7 years ago Closed 7 years ago

Start the redesign of the Home and add a Sidebar in about:telemetry

Categories

(Toolkit :: Telemetry, enhancement, P1)

Product:
Toolkit
Component:
Telemetry
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: flyingrub, Assigned: flyingrub, Mentored)

References

Details

Attachments

(3 files)

Bug 1370513 - Add a sidebar and use Common.css style in about:telemetry
59 bytes, text/x-review-board-request
chutten
: review+
Details
Bug 1370513 - Add an explanation text on about:telemetry homepage
59 bytes, text/x-review-board-request
chutten
: review+
Details
Bug 1370513 - Fix Subsection displayed in the sidebar
59 bytes, text/x-review-board-request
chutten
: review+
Details
You can comment if you have any suggestion on the mockup available here : https://docs.google.com/document/d/1T6j_MhuwymNCNNBwfhJFu7oU2YMvPKf9aJgjApQb91Q/edit?usp=sharing
Priority: -- → P1
Assignee: nobody → flyinggrub
Status: NEW → ASSIGNED
Comment on attachment 8874830 [details] Bug 1370513 - Add a sidebar and use Common.css style in about:telemetry https://reviewboard.mozilla.org/r/146222/#review150768 ::: commit-message-4dd1d:1 (Diff revision 1) > +Bug 1370513 - Introduce a sidebar r?chutten More specifically, Introduce a sidebar to about:telemetry ::: toolkit/content/aboutTelemetry.css:275 (Diff revision 1) > +div[role='toolbar'] { > + min-width: 10vw; > + border-right: 1px solid #cedae4; > +} > + > +[role='heading'] { attribute selectors are an interesting choice. Generally they perform slower than class/id selectors and are harder to manipulate in script. I'd recommend classes over roles, unless you're following a best practice or a pattern seen elsewhere in the code.
Attachment #8874830 - Flags: review?(chutten) → review-
Comment on attachment 8874830 [details] Bug 1370513 - Add a sidebar and use Common.css style in about:telemetry https://reviewboard.mozilla.org/r/146222/#review151464 ::: toolkit/content/aboutTelemetry.js:1740 (Diff revision 2) > */ > function setupListeners() { > Settings.attachObservers(); > PingPicker.attachObservers(); > > + // Event delegation on #categories element It's not really delegation. You can skip this comment. ::: toolkit/content/aboutTelemetry.js:2132 (Diff revision 2) > pre.textContent = JSON.stringify(gPingData, null, 2); > > - try { > + > - displayRichPingData(ping, updatePayloadList); > + displayRichPingData(ping, updatePayloadList); > - } catch (err) { > - PingPicker._showRawPingData(); > + > + // try { checking-in commented-out code is frowned upon
Attachment #8874830 - Flags: review?(chutten) → review-
Comment on attachment 8874830 [details] Bug 1370513 - Add a sidebar and use Common.css style in about:telemetry https://reviewboard.mozilla.org/r/146222/#review151922 ::: toolkit/content/aboutTelemetry.css:19 (Diff revision 3) > + height: 100%; > } > > -h2 { > - font-size: medium; > +#categories { > + padding-top: 0px; > + min-width: 300px; That's an interesting magic number. Any particular reason you chose 300px?
Attachment #8874830 - Flags: review?(chutten)
Comment on attachment 8876779 [details] Bug 1370513 - Add an explanation text on about:telemetry homepage https://reviewboard.mozilla.org/r/148098/#review152566 ::: commit-message-d2230:2 (Diff revision 1) > +Bug 1370513 - Add an explanation text on about:telemetry homepage > +r?chutten The r? should be on the summary line ::: toolkit/content/aboutTelemetry.js:255 (Diff revision 1) > } > }, > > + getStatusStringForSetting(setting) { > + let enabled = Preferences.get(setting.pref, setting.defaultPrefValue); > + return enabled ? "enabled" : "disabled"; This won't say activée when the language is set to French. I think you may have to rework how you localize this text. Maybe ask l10n for tips. ::: toolkit/locales/en-US/chrome/global/aboutTelemetry.properties:10 (Diff revision 1) > # Note to translators: > # - %1$S will be replaced by brandFullName > # - %2$S will be replaced with the value of the toolkit.telemetry.server_owner preference > pageSubtitle = This page shows the information about performance, hardware, usage and customizations collected by Telemetry. This information is submitted to %1$S to help improve %2$S. > > +homeExplanation = Those data are helpful for Mozillas engineers and decision-makers to be able to measure how Firefox behaves in the real world. The Telemetry feature that provides this capability by sending performance and usage info to Mozilla is <a href="" class="change-data-choices-link">%1$S</a> and extended telemetry is <a href="" class="change-data-choices-link">%2$S</a>. As you use Firefox, Telemetry measures and collects non-personal information, such as performance, hardware, usage and customizations. It then sends this information to Mozilla on a daily basis and we use it to improve Firefox. A few things: 1) As above, you need to include notes to translators if you are making substitutions. That way the translator knows the context of the substitution. 2) Brevity is a virtue. I think `pageSubtitle` is the most concise form of what we're looking for. What will serve users most, I think, is a link to some authoritative documentation in case they want to know more other than "What data is collected" (performance, hardware, usage and customizations) "What is it called" (Telemetry) "Why is it collected?" (To help improve Firefox) "To whom is it sent?" (Mozilla). 3) All that being said, what you wrote is very clear and has very few errors. I am pleased that you understand Telemetry so well that you can express your understanding so precisely. All in all, I think you can use pageSubtitle as the introductory paragraph text. A short list of references/resources/further reading links would be an excellent addition.
Comment on attachment 8874830 [details] Bug 1370513 - Add a sidebar and use Common.css style in about:telemetry https://reviewboard.mozilla.org/r/146222/#review152974 It seems as though you may have accidentally merged the original "add a sidebar" commit with this "use common.css" one. Was that deliberate? If so, you should put in the commit message that this is now more than just using Common.css. ::: toolkit/content/aboutTelemetry.css:25 (Diff revision 4) > +} > + > +#category-raw { > + border-top: 1px solid var(--in-content-header-border-color); > + box-sizing: border-box; > + position: fixed; position: fixed; is poison on mobile, and usually not what you want on desktop (the better choices are position: absolute or using flex-align to get it where you want it) I'm not 100% sure which is the containing block here (I think it's #categories?) in which case you might prefer to make it display: flex; so that you can flex-pack the main elements to the top of the list and flex-align the last element to the end. (I might be rusty on the property names. Might be orient, not align)
Attachment #8874830 - Flags: review?(chutten) → review-
Comment on attachment 8876779 [details] Bug 1370513 - Add an explanation text on about:telemetry homepage https://reviewboard.mozilla.org/r/148098/#review152976 This doesn't appear to be the latest version you showed me in IRC
Attachment #8876779 - Flags: review?(chutten) → review-
Comment on attachment 8874830 [details] Bug 1370513 - Add a sidebar and use Common.css style in about:telemetry https://reviewboard.mozilla.org/r/146222/#review153614 Take a look at https://reviewboard.mozilla.org/r/146222/#issue-summary to see what is still outstanding before this should be reflagged for review.
Attachment #8874830 - Flags: review?(chutten) → review-
Comment on attachment 8876779 [details] Bug 1370513 - Add an explanation text on about:telemetry homepage https://reviewboard.mozilla.org/r/148098/#review153616 ::: toolkit/locales/en-US/chrome/global/aboutTelemetry.properties:10 (Diff revisions 2 - 3) > # Note to translators: > # - %1$S will be replaced by brandFullName > # - %2$S will be replaced with the value of the toolkit.telemetry.server_owner preference > pageSubtitle = This page shows the information about performance, hardware, usage and customizations collected by Telemetry. This information is submitted to %1$S to help improve %2$S. > > -homeExplanation = Those data are helpful for Mozillas engineers and decision-makers to be able to measure how Firefox behaves in the real world. The Telemetry feature that provides this capability by sending performance and usage info to Mozilla is <a href="" class="change-data-choices-link">%1$S</a> and extended telemetry is <a href="" class="change-data-choices-link">%2$S</a>. As you use Firefox, Telemetry measures and collects non-personal information, such as performance, hardware, usage and customizations. It then sends this information to Mozilla on a daily basis and we use it to improve Firefox. > +homeExplanation = Telemetry is <a href="" class="change-data-choices-link">%1$S</a> and extended telemetry is <a href="" class="change-data-choices-link">%2$S</a>. Still need notes to translators to say what %1$S and %2$S will be replaced with (enabled/disabled) ::: toolkit/locales/en-US/chrome/global/aboutTelemetry.properties:12 (Diff revisions 2 - 3) > # - %2$S will be replaced with the value of the toolkit.telemetry.server_owner preference > pageSubtitle = This page shows the information about performance, hardware, usage and customizations collected by Telemetry. This information is submitted to %1$S to help improve %2$S. > > -homeExplanation = Those data are helpful for Mozillas engineers and decision-makers to be able to measure how Firefox behaves in the real world. The Telemetry feature that provides this capability by sending performance and usage info to Mozilla is <a href="" class="change-data-choices-link">%1$S</a> and extended telemetry is <a href="" class="change-data-choices-link">%2$S</a>. As you use Firefox, Telemetry measures and collects non-personal information, such as performance, hardware, usage and customizations. It then sends this information to Mozilla on a daily basis and we use it to improve Firefox. > +homeExplanation = Telemetry is <a href="" class="change-data-choices-link">%1$S</a> and extended telemetry is <a href="" class="change-data-choices-link">%2$S</a>. > > -pingExplanation = Each data sent are bundled in a "ping". Data are stored in it as JSON object and contains common information to all pings and a payload specific to a certain ping types. This page helps you to access its data easily. You are looking at the <span class="change-ping">%1$S ping</span>. > +pingExplanation = Each data sent are bundled in a "ping". You are looking at the <span class="change-ping">%1$S</span> ping. Grammar: Data is sent bundled into a "pings" Maybe linkify the word "ping" and point it at the ping documentation here: http://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/concepts/pings.html ::: toolkit/content/aboutTelemetry.css:28 (Diff revision 3) > #category-raw { > border-top: 1px solid var(--in-content-header-border-color); > box-sizing: border-box; > - position: fixed; > + background-color: inherit; > + min-width: inherit; > + position: absolute; Ohhhh, you did the work in _this_ commit, not the previous one. That's caused a bit of confusion. Let me see if I can pry this apart properly..
Attachment #8876779 - Flags: review?(chutten) → review-
Comment on attachment 8874830 [details] Bug 1370513 - Add a sidebar and use Common.css style in about:telemetry https://reviewboard.mozilla.org/r/146222/#review151922 > That's an interesting magic number. Any particular reason you chose 300px? The particularity about this magic number is that it's him that chose me. ;) I choosed it so that no section's text was trimmed down. I don't know if there is a better way to do this ?
Comment on attachment 8874830 [details] Bug 1370513 - Add a sidebar and use Common.css style in about:telemetry https://reviewboard.mozilla.org/r/146222/#review154574
Attachment #8874830 - Flags: review?(chutten) → review+
Comment on attachment 8878452 [details] Bug 1370513 - Fix Subsection displayed in the sidebar https://reviewboard.mozilla.org/r/149802/#review154584 ::: commit-message-df7c5:2 (Diff revision 1) > +Bug 1370513 - Fix Subsection displayed in the sidebar r?chutten > + This could use an explanation sentence or two explaining what needed fixing. ::: toolkit/content/aboutTelemetry.js:236 (Diff revision 1) > }); > } else { > // Show the data choices preferences on desktop. > let mainWindow = getMainWindowWithPreferencesPane(); > - mainWindow.openPreferences("privacy-reports", {origin: "aboutTelemetry"}); > + // The advanced subpanes are only supported in the old organization, which will > + // be removed by bug 1349689. Is this properly part of this commit? Sounds like this is an actual bug that might be shipping in Beta. *checks* It is. Clicking on the links in about:telemetry in Beta55 is broken. You should file a separate bug as a follow-up to 1366978 to get this fixed and then request uplift to beta55. It will unfortunately make the rebase a little annoying, but we should fix Beta55 before it becomes Release55. ::: toolkit/content/aboutTelemetry.js:1384 (Diff revision 1) > > var GenericSubsection = { > > addSubSectionToSidebar(id, title) { > let category = document.querySelector("#categories > [value=" + id + "]"); > + if (!category.classList.contains("has-subsection")) { You don't need to guard it. classList.add can't add a second identical class, see https://developer.mozilla.org/en-US/docs/Web/API/Element/classList ::: toolkit/content/aboutTelemetry.js:1739 (Diff revision 1) > */ > function show(selected) { > + let current_button = document.querySelector(".category.selected"); > + current_button.classList.remove("selected"); > + selected.classList.add("selected"); > + document.getSelection().empty(); I don't feel comfortable leaving this hack in place. The devtools should allow you to step through the JS and find the code that creates the selection. ::: toolkit/content/aboutTelemetry.js:1780 (Diff revision 1) > Settings.attachObservers(); > PingPicker.attachObservers(); > > let menu = document.getElementById("categories"); > menu.addEventListener("click", (e) => { > + e.preventDefault(); The default action of a click isn't to select text, so I guess it makes sense that this wouldn't work. You can omit this? ::: toolkit/locales/en-US/chrome/global/aboutTelemetry.properties:17 (Diff revision 1) > # - %2$S will be replaced by either enabled or disabled > homeExplanation = Telemetry is <a href="" class="change-data-choices-link">%1$S</a> and extended telemetry is <a href="" class="change-data-choices-link">%2$S</a>. > > # Note to translators: > # - %1$S will be replaced by the ping name > -pingExplanation = Each Data is sent bundled into a <a href="http://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/concepts/pings.html">"pings"</a>. You are looking at the <span class="change-ping">%1$S</span> ping. > +pingExplanation = Each piece of information is sent bundled into a <a href="http://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/concepts/pings.html">"pings"</a>. You are looking at the <span class="change-ping">%1$S</span> ping. This is the wrong commit for this edit.
Attachment #8878452 - Flags: review?(chutten) → review-
Comment on attachment 8878452 [details] Bug 1370513 - Fix Subsection displayed in the sidebar https://reviewboard.mozilla.org/r/149802/#review154584 > I don't feel comfortable leaving this hack in place. The devtools should allow you to step through the JS and find the code that creates the selection. I already checked and it happens at the line just before (l. 1938)...
Comment on attachment 8878452 [details] Bug 1370513 - Fix Subsection displayed in the sidebar https://reviewboard.mozilla.org/r/149802/#review154584 > Is this properly part of this commit? Sounds like this is an actual bug that might be shipping in Beta. > > *checks* It is. Clicking on the links in about:telemetry in Beta55 is broken. You should file a separate bug as a follow-up to 1366978 to get this fixed and then request uplift to beta55. > > It will unfortunately make the rebase a little annoying, but we should fix Beta55 before it becomes Release55. https://bugzilla.mozilla.org/show_bug.cgi?id=1373823 I created this bug.
Comment on attachment 8876779 [details] Bug 1370513 - Add an explanation text on about:telemetry homepage https://reviewboard.mozilla.org/r/148098/#review155310 ::: toolkit/locales/en-US/chrome/global/aboutTelemetry.properties:17 (Diff revision 6) > +# - %2$S will be replaced by either enabled or disabled > +homeExplanation = Telemetry is <a href="" class="change-data-choices-link">%1$S</a> and extended telemetry is <a href="" class="change-data-choices-link">%2$S</a>. > + > +# Note to translators: > +# - %1$S will be replaced by the ping name > +pingExplanation = Each piece of information is sent bundled into a <a href="http://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/concepts/pings.html">"pings"</a>. You are looking at the <span class="change-ping">%1$S</span> ping. Alas: "Each piece of information is sent bundled into a "pings". You are looking at the <> ping" The use of a singular article "a" means that "pings" should be singular ("ping") One character to be removed and this will be good-to-go.
Attachment #8876779 - Flags: review?(chutten) → review+
Comment on attachment 8878452 [details] Bug 1370513 - Fix Subsection displayed in the sidebar https://reviewboard.mozilla.org/r/149802/#review155314 It's end-of-day for me, I'm sorry I didn't get to this sooner. We must figure out the selection thing before this code can be committed. Given the timezone difference, see if you can raise someone on #developers who'd be able to help you out with this. If nothing comes of it by the time I'm in tomorrow, I'll take this code locally and see if I can poke it into submission on my machine. We _will_ get to the bottom of this.
Attachment #8878452 - Flags: review?(chutten) → review-
Comment on attachment 8878452 [details] Bug 1370513 - Fix Subsection displayed in the sidebar https://reviewboard.mozilla.org/r/149802/#review156330
Attachment #8878452 - Flags: review?(chutten) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/7aa109946f67 Add a sidebar and use Common.css style in about:telemetry r=chutten https://hg.mozilla.org/integration/autoland/rev/811f94aea5ae Add an explanation text on about:telemetry homepage r=chutten https://hg.mozilla.org/integration/autoland/rev/cde5fccb8538 Fix Subsection displayed in the sidebar r=chutten
Keywords: checkin-needed
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=109231800&repo=autoland
Flags: needinfo?(flyinggrub)
Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/db4f180aae4b Backed out changeset cde5fccb8538 https://hg.mozilla.org/integration/autoland/rev/e0a25ab136da Backed out changeset 811f94aea5ae https://hg.mozilla.org/integration/autoland/rev/72a8245e2525 Backed out changeset 7aa109946f67 for test failures in browser_misused_characters_in_strings.js
I updated the commit on reviewBoard.
Flags: needinfo?(flyinggrub)
It looks like a lot of strings become unused in the new design, e.g. aboutTelemetry.uploadEnabled, aboutTelemetry.uploadDisabled, etc. If a string is not used anymore in the code, you should remove it, no point in leaving it there indefinitely. A few nits: to avoid other devs coming in later and thinking that is a smart idea to reuse those strings, it would be nice to use more defined string IDs. pings = pings enabled = enabled disabled = disabled to pingExplanationLink = pings telemetryEnabled = enabled telemetryDisabled = disabled
I removed unused strings and edited the strings IDs. I thought that string reuse was ok :/ ... aboutTelemetry.uploadEnabled is already removed by the second commit. :)
(In reply to flyingrub from comment #46) > I removed unused strings and edited the strings IDs. > I thought that string reuse was ok :/ ... Simple example: "enabled" works applied to telemetry and anything else in English, on the other hand most languages have genders (male, female, neuter), and adjectives need to adapt to the associated noun. This one is on the line, I assume "telemetry" and "extended telemetry" will have the same gender. If you reuse this "enabled" in a completely different sentence, it's likely going to be wrong.
Comment on attachment 8878452 [details] Bug 1370513 - Fix Subsection displayed in the sidebar https://reviewboard.mozilla.org/r/149802/#review157222 A couple of nits to fix and then this should be ready for another checkin-needed ::: toolkit/locales/en-US/chrome/global/aboutTelemetry.properties:11 (Diff revision 8) > # - %1$S will be replaced by brandFullName > # - %2$S will be replaced with the value of the toolkit.telemetry.server_owner preference > pageSubtitle = This page shows the information about performance, hardware, usage and customizations collected by Telemetry. This information is submitted to %1$S to help improve %2$S. > > # Note to translators: > # - %1$S will be replaced by either enabled or disabled You need to update these to telemetryEnabled/telemetryDisabled ::: toolkit/locales/en-US/chrome/global/aboutTelemetry.properties:16 (Diff revision 8) > # - %1$S will be replaced by either enabled or disabled > # - %2$S will be replaced by either enabled or disabled > homeExplanation = Telemetry is %1$S and extended telemetry is %2$S. > > # Note to translators: > # - %1$S will be replaced by a link with "pings" as text Ditto this with pingExplanationLink
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/cd57a442333b Add a sidebar and use Common.css style in about:telemetry r=chutten https://hg.mozilla.org/integration/autoland/rev/ffcbe7e47b3c Add an explanation text on about:telemetry homepage r=chutten https://hg.mozilla.org/integration/autoland/rev/6ca048e09dea Fix Subsection displayed in the sidebar r=chutten
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cd57a442333b https://hg.mozilla.org/mozilla-central/rev/ffcbe7e47b3c https://hg.mozilla.org/mozilla-central/rev/6ca048e09dea
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Looks like this landed without applying any of the feedback from comment 44 and comment 48?
(In reply to Francesco Lodolo [:flod] from comment #52) > Looks like this landed without applying any of the feedback from comment 44 > and comment 48? Never mind, I completely missed that the strings were modified twice.
(In reply to Carsten Book [:Tomcat] from comment #51) > https://hg.mozilla.org/mozilla-central/rev/ffcbe7e47b3c > +pingExplanation = Each piece of information is sent bundled into %1$S. You are looking at the %2$S ping. Some notes/thoughts for upcoming rework related to l10n: 1) %1$S displays as "pings", i.e. the quotation marks are hardcoded. Locales may however require quotation marks to be single, as well as curly (‘pings’). 2) The "the %2$S ping" part may cause l10n issues, as it displays as "the current ping" or "the 2017/07/08 10:40:46 - saved-session ping" which is merely fine by coincidence in English, but locales may need to use "the ping %2$S" for the latter, i.e. "the ping 2017/07/08 10:40:46 - saved-session" suits them better, and hence "the ping current" will be wrong. 3) The "current" text is not available for l10n.
> # - %1$S will be replaced by either telemetryEnabled or telemetryDisabled > # - %2$S will be replaced by either telemetryEnabled or telemetryDisabled > homeExplanation = Telemetry is %1$S and extended telemetry is %2$S. I'm sorry but such substitutions completely fail in languages like Polish - we need separate forms for %1$S and %2$S.
Flags: needinfo?(francesco.lodolo)
Flags: needinfo?(flyinggrub)
Flags: needinfo?(chutten)
(In reply to Stefan Plewako [:stef] from comment #55) > > # - %1$S will be replaced by either telemetryEnabled or telemetryDisabled > > # - %2$S will be replaced by either telemetryEnabled or telemetryDisabled > > homeExplanation = Telemetry is %1$S and extended telemetry is %2$S. > > I'm sorry but such substitutions completely fail in languages like Polish - > we need separate forms for %1$S and %2$S. Please file a new bug blocking this one. It would be also useful to give an example of how the string turns out, and how it should to be correct.
Flags: needinfo?(francesco.lodolo)
Flags: needinfo?(flyinggrub)
Flags: needinfo?(chutten)
Depends on: 1382193
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: