Per Specs: https://mozilla.invisionapp.com/share/P4ACQT1E3#/screens/217167680 Remove Colon: History Section: Firefox will Location Bar Section: When using the location bar, suggest Certificates Section: When a server requests your personal certificate Update Description: Tracking Protection Section: Tracking is when companies collect information about you to build a profile and display content based on your browsing and personal data. (Learn More Link) Block known tracking companies from displaying content Update Radio Button Text: Tracking Protection Section: Only in Private Browsing mode Add Check Box: Tracking Protection Section: Always apply Do Not Track (Learn More Link) "Firefox will send a signal that you don't want to be tracked whenever Tracking Protection is on." Remove Radio Check Box: Notifications Section: Do not disturb me Not notification will be shown until you restart Nightly Remove Sections: Container Tabs Site Data Add Section: (Waiting for final copy review) Reports (previously in Advanced> Data Choices

Spec for Privacy & Security pane: https://mozilla.invisionapp.com/share/P4ACQT1E3#/screens/217167680

It looks like "Reports" for telemetry etc. is still in the "Updates" section. Is there a separate bug to change that and update copy there? Just making sure that isn't falling through the cracks...

(In reply to :Gijs (away until Tuesday 18) from comment #3) > It looks like "Reports" for telemetry etc. is still in the "Updates" > section. Is there a separate bug to change that and update copy there? Just > making sure that isn't falling through the cracks... Oh, it seems this is somewhere else in the patchset you uploaded, but it's not marked as a dep of this bug and hasn't made nightly or central yet? So it's a bit tricky for me to review the markup changes you're making there...

Comment on attachment 8858223 [details] Bug 1356020 - Update about:preferences#privacy to fix inconsistencies with strings, move the positioning of the Certificates buttons to the right-side, and update the styling of the Reports section to match the requested text-formatting. https://reviewboard.mozilla.org/r/130190/#review132976 I reviewed the l10n changes, and though there's bits missing they look more or less OK otherwise, but I'm struggling to review the markup changes given that the patch doesn't apply on central and I don't see deps where the markup gets changed to the point that this patch modifies it further. In any case, there should be enough feedback in here for now. :-) ::: commit-message-dcf01:1 (Diff revision 1) > +Bug 1356020 - Fix inconsistencies with strings in about:preferences#privacy. r?gijs This is making significant markup changes and adding JS, so either those changes should be split out or the commit message should be expanded to cover them. ::: browser/components/preferences/in-content/privacy.js:91 (Diff revision 1) > /** > * Show the Containers UI depending on the privacy.userContext.ui.enabled pref. > */ > _initBrowserContainers() { > if (!Services.prefs.getBoolPref("privacy.userContext.ui.enabled")) { > + document.getElementById("browserContainersGroup").setAttribute("data-hidden-from-search", "true"); This seems unrelated? ::: browser/components/preferences/in-content/privacy.xul:525 (Diff revision 1) > </groupbox> > > <!-- Certificates --> > -<groupbox id="certSelection" align="start" data-category="panePrivacy" hidden="true"> > +<groupbox id="certSelection" data-category="panePrivacy" hidden="true"> > <caption><label>&certificateTab.label;</label></caption> > - <description id="CertSelectionDesc" control="certSelection">&certPersonal.description;</description> > + <description id="CertSelectionDesc" control="certSelection">&certPersonal2.description;</description> I wish we just hid this if we knew NSS had no personal certificates. Unrelated to this bug, of course, but if we're simplifying prefs anyway... ::: browser/components/preferences/in-content/privacy.xul:527 (Diff revision 1) > + <hbox align="start"> > + <vbox flex="1"> I can't apply this patch, but the markup here surprises me. Specifically, the design in invision seems to suggest that the buttons are aligned only with the enableOCSP checkbox. Your markup will (I think?) top-align them to the cert selection radiogroup. Is this a change that was discussed elsewhere, or just a mistake, or...? :-) ::: browser/components/preferences/in-content/privacy.xul:705 (Diff revision 1) > - <vbox> > + <hbox align="center"> > - <caption> > <checkbox id="submitHealthReportBox" label="&enableHealthReport.label;" > accesskey="&enableHealthReport.accesskey;"/> > - </caption> > + <label id="FHRLearnMore" > - <hbox class="indent" flex="1"> > - <label flex="1">&healthReportDesc.label;</label> > - <label id="FHRLearnMore" flex="1" Can you elaborate on what these changes are achieving? ::: browser/locales/en-US/chrome/browser/preferences/privacy.dtd:10 (Diff revision 1) > <!ENTITY trackingProtectionHeader2.label "Tracking Protection"> > +<!ENTITY trackingProtection.description "Tracking is when companies collect information about you to build a profile and display content based on your browsing and personal data."> > +<!ENTITY trackingProtection.radioGroupLabel "Block known tracking companies from displaying content"> > <!ENTITY trackingProtectionAlways.label "Always"> > <!ENTITY trackingProtectionAlways.accesskey "y"> > <!ENTITY trackingProtectionPrivate.label "Only in private windows"> The spec says this needs to be updated, too.

(In reply to :Gijs (away until Tuesday 18) from comment #5) > ::: browser/components/preferences/in-content/privacy.js:91 > (Diff revision 1) > > /** > > * Show the Containers UI depending on the privacy.userContext.ui.enabled pref. > > */ > > _initBrowserContainers() { > > if (!Services.prefs.getBoolPref("privacy.userContext.ui.enabled")) { > > + document.getElementById("browserContainersGroup").setAttribute("data-hidden-from-search", "true"); > > This seems unrelated? I added a comment about this in the code. This is causing a visual gap when viewing the Privacy pane. > ::: browser/components/preferences/in-content/privacy.xul:525 > (Diff revision 1) > > </groupbox> > > > > <!-- Certificates --> > > -<groupbox id="certSelection" align="start" data-category="panePrivacy" hidden="true"> > > +<groupbox id="certSelection" data-category="panePrivacy" hidden="true"> > > <caption><label>&certificateTab.label;</label></caption> > > - <description id="CertSelectionDesc" control="certSelection">&certPersonal.description;</description> > > + <description id="CertSelectionDesc" control="certSelection">&certPersonal2.description;</description> > > I wish we just hid this if we knew NSS had no personal certificates. > Unrelated to this bug, of course, but if we're simplifying prefs anyway... I'll file a bug about doing this. > ::: browser/components/preferences/in-content/privacy.xul:527 > (Diff revision 1) > > + <hbox align="start"> > > + <vbox flex="1"> > > I can't apply this patch, but the markup here surprises me. Specifically, > the design in invision seems to suggest that the buttons are aligned only > with the enableOCSP checkbox. Your markup will (I think?) top-align them to > the cert selection radiogroup. Is this a change that was discussed > elsewhere, or just a mistake, or...? :-) You would have needed to apply the patches from bug 1355522. I moved the buttons now so that they are aligned with the enableOCSP checkbox. > ::: browser/components/preferences/in-content/privacy.xul:705 > (Diff revision 1) > > - <vbox> > > + <hbox align="center"> > > - <caption> > > <checkbox id="submitHealthReportBox" label="&enableHealthReport.label;" > > accesskey="&enableHealthReport.accesskey;"/> > > - </caption> > > + <label id="FHRLearnMore" > > - <hbox class="indent" flex="1"> > > - <label flex="1">&healthReportDesc.label;</label> > > - <label id="FHRLearnMore" flex="1" > > Can you elaborate on what these changes are achieving? These changes are to match the text-styling that is shown in the spec. Before this patch each checkbox was shown as a <caption> and thus was bolded, which doesn't match any other checkbox in the preferences. > ::: browser/locales/en-US/chrome/browser/preferences/privacy.dtd:10 > (Diff revision 1) > > <!ENTITY trackingProtectionHeader2.label "Tracking Protection"> > > +<!ENTITY trackingProtection.description "Tracking is when companies collect information about you to build a profile and display content based on your browsing and personal data."> > > +<!ENTITY trackingProtection.radioGroupLabel "Block known tracking companies from displaying content"> > > <!ENTITY trackingProtectionAlways.label "Always"> > > <!ENTITY trackingProtectionAlways.accesskey "y"> > > <!ENTITY trackingProtectionPrivate.label "Only in private windows"> > > The spec says this needs to be updated, too. I disagreed with the spec here. We only use "Private Browsing mode" in two places within our UI (Custom history settings "Always use Private Browsing mode" and in the quit dialog when in a private window. I'd like to discuss more with UX about their request here and in the meantime I'd rather not make this change.

Comment on attachment 8858223 [details] Bug 1356020 - Update about:preferences#privacy to fix inconsistencies with strings, move the positioning of the Certificates buttons to the right-side, and update the styling of the Reports section to match the requested text-formatting. https://reviewboard.mozilla.org/r/130190/#review133056 r=me to unblock this as I haven't noticed any issue, but my review is nowhere near as detailed as what Gijs did in comment 5, so feel free to re-review after the week-end Gijs.

Comment on attachment 8858223 [details] Bug 1356020 - Update about:preferences#privacy to fix inconsistencies with strings, move the positioning of the Certificates buttons to the right-side, and update the styling of the Reports section to match the requested text-formatting. https://reviewboard.mozilla.org/r/130190/#review132976 > I wish we just hid this if we knew NSS had no personal certificates. Unrelated to this bug, of course, but if we're simplifying prefs anyway... I filed bug 1356656 for this.

Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/39a46a779d51 Update about:preferences#privacy to fix inconsistencies with strings, move the positioning of the Certificates buttons to the right-side, and update the styling of the Reports section to match the requested text-formatting. r=florian

Comment on attachment 8858223 [details] Bug 1356020 - Update about:preferences#privacy to fix inconsistencies with strings, move the positioning of the Certificates buttons to the right-side, and update the styling of the Reports section to match the requested text-formatting. https://reviewboard.mozilla.org/r/130190/#review133080 Sorry, I didn't realize we were in a rush, and technically I'm supposed to be on PTO today. Anyway. ::: browser/components/preferences/in-content/privacy.xul:713 (Diff revision 2) > - <checkbox id="submitTelemetryBox" preference="toolkit.telemetry.enabled" > + <checkbox id="submitTelemetryBox" preference="toolkit.telemetry.enabled" > - label="&enableTelemetryData.label;" > + label="&enableTelemetryData.label;" > - accesskey="&enableTelemetryData.accesskey;"/> > + accesskey="&enableTelemetryData.accesskey;"/> > - </caption> > + <label id="telemetryLearnMore" > - <hbox class="indent" flex="1"> > - <label id="telemetryDataDesc" flex="1">&telemetryDesc.label;</label> > - <label id="telemetryLearnMore" flex="1" > - class="learnMore text-link">&telemetryLearnMore.label;</label> > + class="learnMore text-link">&telemetryLearnMore.label;</label> If the label string for the checkbox binding is long (in, say, German), does it ever wrap, or do we just end up with a horizontal scrollbar? And what happens with the learn more link in that case? This to say that it would probably be more appropriate to have the label as a separate element that wraps the checkbox and includes the learn more link, or something like that, but I realize that that is more work as far as styling it correctly (making wrapped lines align at an indent). If I'm right that the behaviour here is suboptimal (and I might not be, it's almost midnight here and I'm tired), then perhaps that would be fodder for a followup.

I have seen the previous strings in firefox nightly 55.0a1 (2017-04-12)on Windows 10(64Bit). I can verify that the strings has been fixed in latest nightly 55.0a1 . Build ID 20170420030346 User Agent Mozilla/5.0 (Windows NT 10.0; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0 [Bugday-20170419]