String Changes to about:preferences#advanced

VERIFIED FIXED in Firefox 55

Status

()

Firefox
Preferences
VERIFIED FIXED
4 months ago
4 months ago

People

(Reporter: JW_SoftvisionQA, Assigned: jaws)

Tracking

(Blocks: 1 bug)

55 Branch
Firefox 55
Points:
---

Firefox Tracking Flags

(firefox55 verified)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

4 months 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)
(Assignee)

Updated

4 months ago
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Spec for Updates pane: https://mozilla.invisionapp.com/share/P4ACQT1E3#/screens/217167550
(Assignee)

Updated

4 months ago
Blocks: 1324168
No longer blocks: 1335907

Comment 3

4 months 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
Attachment #8858224 - Flags: review?(gijskruitbosch+bugs)
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.]

Comment 5

4 months 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
Comment hidden (mozreview-request)
(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 8

4 months 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.
Attachment #8858224 - Flags: review?(florian) → review+

Comment 9

4 months ago
Given the interdiff this looks fine to me.

Comment 10

4 months 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
https://hg.mozilla.org/mozilla-central/rev/0f82ec111261
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Comment 12

4 months 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]
(Assignee)

Updated

4 months ago
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
You need to log in before you can comment on or make changes to this bug.