Justin [:JW_SoftvisionQA] Reporter
	Description • 8 years ago

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)

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 2 • 8 years ago

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

	:Gijs (he/him)
	Comment 3 • 8 years ago
mozreview-review

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

	Justin Dolske [:Dolske]
	Comment 4 • 8 years ago

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.]

	:Gijs (he/him)
	Comment 5 • 8 years ago

(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

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 7 • 8 years ago

(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.

	Florian Quèze [:florian]
	Comment 8 • 8 years ago
mozreview-review

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.

	:Gijs (he/him)
	Comment 9 • 8 years ago

Given the interdiff this looks fine to me.

	Pulsebot
	Comment 10 • 8 years ago

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

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 11 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/0f82ec111261

	Md. Majedul islam
	Comment 12 • 8 years ago

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]