Per Specs: https://mozilla.invisionapp.com/share/P4ACQT1E3#/screens/217167550 Add Text: Firefox Updates Section: (Added from Firefox> About Firefox) Version info Firefox is up to date Add a copy string: Firefox Updates Section: Allow Firefox to Update Radio Buttons: Firefox Updates Section: (Merge the "Automatically Update" to "Firefox Updates" Add Automatically update search engines (check box)

Spec for Updates pane: https://mozilla.invisionapp.com/share/P4ACQT1E3#/screens/217167550

Comment on attachment 8858224 [details] Bug 1356021 - Update about:preferences#advanced to fix inconsistencies with strings, show the bottom-border beneath the header to match other categories, and update the layout of radio buttons and checkboxes to match the requested layout. https://reviewboard.mozilla.org/r/130194/#review132982 This mostly looks OK but I think the spec fails to highlight some things that are changed in the mock/spec, and we should be sure whether they don't actually need changing, too (certainly looks like some adjustments for ":" usage would fall within the purview of this bug there). ::: commit-message-1715f:1 (Diff revision 1) > +Bug 1356021 - Fix inconsistencies with strings in about:preferences#advanced. r?gijs Here too there are markup and styling changes that I think deserve mention in the (expanded) commit message. ::: browser/components/preferences/in-content/advanced.xul:57 (Diff revision 1) > <!-- Update --> > -#ifdef MOZ_UPDATER > <groupbox id="updateApp" align="start" data-category="paneAdvanced" hidden="true"> > <caption><label>&updateApplication.label;</label></caption> > + <description>&updateApplication.description;</description> > + <hbox align="start"> Can/should we remove the align=start from the groupbox? ::: browser/components/preferences/in-content/advanced.xul:86 (Diff revision 1) > - accesskey="&enableSearchUpdate.accesskey;" > + accesskey="&enableSearchUpdate2.accesskey;" > - preference="browser.search.update"/> > + preference="browser.search.update"/> > + </vbox> > + <spacer flex="1"/> > + <vbox> > +#ifdef MOZ_UPDATER The ifdefs should be around the spacer (and the containing vbox here) as well, so the stuff on the left can take up the full space. ::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd:88 (Diff revision 1) > > <!ENTITY updateTab.label "Update"> > > <!ENTITY updateApplication.label "&brandShortName; Updates"> > +<!ENTITY updateApplication.description "Allow &brandShortName; to"> > <!ENTITY updateAuto1.label "Automatically install updates (recommended: improved security)"> This isn't marked as changed in the spec, but the text in the spec doesn't match this string. ::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd:90 (Diff revision 1) > > <!ENTITY updateApplication.label "&brandShortName; Updates"> > +<!ENTITY updateApplication.description "Allow &brandShortName; to"> > <!ENTITY updateAuto1.label "Automatically install updates (recommended: improved security)"> > <!ENTITY updateAuto1.accesskey "A"> > <!ENTITY updateCheckChoose.label "Check for updates, but let you choose whether to install them"> Ditto (comma!) ::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd:92 (Diff revision 1) > +<!ENTITY updateApplication.description "Allow &brandShortName; to"> > <!ENTITY updateAuto1.label "Automatically install updates (recommended: improved security)"> > <!ENTITY updateAuto1.accesskey "A"> > <!ENTITY updateCheckChoose.label "Check for updates, but let you choose whether to install them"> > <!ENTITY updateCheckChoose.accesskey "C"> > <!ENTITY updateManual.label "Never check for updates (not recommended: security risk)"> Ditto

Driveby: don't you need to deal with the old (forked) prefs too? e.g. this is removing autoUpdateOthers.label, but isn't updating browser/components/preferences/in-content-old/advanced.xul [That said, we could also re-evaluate if we still need to maintain the fork.]

(In reply to Justin Dolske [:Dolske] from comment #4) > Driveby: don't you need to deal with the old (forked) prefs too? e.g. this > is removing autoUpdateOthers.label, but isn't updating > browser/components/preferences/in-content-old/advanced.xul > > [That said, we could also re-evaluate if we still need to maintain the fork.] The strings are also forked, see: https://dxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/preferences-old/advanced.dtd#100 vs. https://dxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/preferences/advanced.dtd#100

(In reply to :Gijs (away until Tuesday 18) from comment #5) > (In reply to Justin Dolske [:Dolske] from comment #4) > > Driveby: don't you need to deal with the old (forked) prefs too? e.g. this > > is removing autoUpdateOthers.label, but isn't updating > > browser/components/preferences/in-content-old/advanced.xul > > > > [That said, we could also re-evaluate if we still need to maintain the fork.] > > The strings are also forked, see: > > https://dxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/ > browser/preferences-old/advanced.dtd#100 > > vs. > > https://dxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/ > browser/preferences/advanced.dtd#100 Correct, and further to that point, if a string was accidentally removed but used by the in-content-old prefs, the in-content-old pref tests would fail due to the XML parsing error since the entity would be missing. The tests are at /browser/components/preferences/in-content-old/tests and run as part of normal test automation.

Comment on attachment 8858224 [details] Bug 1356021 - Update about:preferences#advanced to fix inconsistencies with strings, show the bottom-border beneath the header to match other categories, and update the layout of radio buttons and checkboxes to match the requested layout. https://reviewboard.mozilla.org/r/130194/#review133058 Same as bug 1356020, r=me to unblock this because I couldn't find any issue, but Gijs' review is better here, so he should feel free to have another look after the week-end. ::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd:101 (Diff revision 2) > -<!ENTITY updateHistory.accesskey "p"> > +<!ENTITY updateHistory2.accesskey "p"> > > <!ENTITY useService.label "Use a background service to install updates"> > <!ENTITY useService.accesskey "b"> > > -<!ENTITY autoUpdateOthers.label "Automatically Update"> > +<!ENTITY enableSearchUpdate2.label "Automatically update search engines"> I think this pref label is misleading (it's about whether we'll be updating user-installed search plugins that provide an update URL), and I'm not convinced we need to expose this in the prefs UI. This could probably be grouped with whatever pref we have to control whether we check for add-on updates. That's to be discussed in a different bug though.

Given the interdiff this looks fine to me.

Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0f82ec111261 Update about:preferences#advanced to fix inconsistencies with strings, show the bottom-border beneath the header to match other categories, and update the layout of radio buttons and checkboxes to match the requested layout. r=florian

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]