Closed Bug 1378366 Opened 6 years ago Closed 6 years ago

[Preferences] No values displayed by default under Fonts & Colors on ar build

Categories

(Firefox :: Settings UI, defect, P4)

Product:
Firefox
Component:
Settings UI
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox54 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- verified

People

(Reporter: cbadau, Assigned: jaws)

Details

(Keywords: dupeme)

Attachments

(3 files)

issue.png
137.38 KB, image/png
Details
Bug 1378366 - If the default font is not found, use 'Default' as the name for an unchanged font preference. Previously this would show up as blank in this case.
59 bytes, text/x-review-board-request
timdream
: review+
Details
Bug 1378366 - Change 'var' to 'const' and 'let' in fontBuilder.js.
59 bytes, text/x-review-board-request
timdream
: review+
Details
Attached image issue.png — 
[Affected versions]:
- latest Nightly 56.0a1
- Firefox 55 Beta 6
- Firefox 54.0a1

[Affected platforms]:
- Windows 7 x64
- Ubuntu 16.04 x64
- Mac OS X 10.12

[Steps to reproduce]:
1. Start Firefox.
2. Visit about:preferences page, 'General' section.
3. Observe the default values displayed under Fonts & Colors. 

[Expected result]:
- Correct values displayed by default under Fonts & Colors. 

[Actual result]:
- No values displayed by default under Fonts & Colors. Please see attachment "issue.png". 

[Regression range]:
- I will be back with a regression range as soon as possible. 
- It isn't a recent regression though, it reproduced in Nightly 54 as well.

[Additional notes]:
- The issue is reproducible ONLY on ar build.
- I've tested on fa, he, fr builds and the issue is NOT reproducible here.
Blocks: photon-preference
Whiteboard: [photon-preference][triage]
Can you verify this issue which still happens in latest Nightly? Thanks
Flags: needinfo?(camelia.badau)
The issue still happens on Windows 7 x64, Mac OS X 10.12 and Ubuntu 14.04 x64 using latest Nightly 56.0a1 (2017-07-31).
Flags: needinfo?(camelia.badau)
Keywords: dupeme
I am pretty sure this is a known issue. I would be suprised if it's not filed, because according to blame, the code was implemented back in bug 274712, i.e. since the existence of Firefox itself. 

It intentionally leave the label to blank if the Platform didn't return any font name for default font, in this case, a default sans-serif font for ar locale.

http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/toolkit/mozapps/preferences/fontbuilder.js#42

I would recommend fixing the logic here by labelling the blank value with a label like "(Unspecified)", "(Unknown)", or "(Default)", with parentheses.

:jaws, do you agree? I'll mark this as good first bug if this is the fix we want.
No longer blocks: photon-preference
Flags: needinfo?(jaws)
Whiteboard: [photon-preference][triage]
Yes, I agree. Michelle, what term should we use here for a missing font? We don't know the name of the default font on the system, but we need to show the user that the selected font is the "default" font. This dropdown is already labeled "Default font" so showing "Default font: (default)" might be weird.
Flags: needinfo?(jaws) → needinfo?(mheubusch)
Priority: -- → P4
Going with just "Default" the same as we already have "Default (%S)"
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Flags: needinfo?(mheubusch)
Comment on attachment 8922389 [details]
Bug 1378366 - If the default font is not found, use 'Default' as the name for an unchanged font preference. Previously this would show up as blank in this case.

https://reviewboard.mozilla.org/r/193426/#review198912

::: toolkit/mozapps/preferences/fontbuilder.js:43
(Diff revision 1)
>      var popup = document.createElement("menupopup");
>      var separator;
>      if (fonts.length > 0) {
> -      if (defaultFont) {
> -        var bundlePreferences = document.getElementById("bundlePreferences");
> +      var bundlePreferences = document.getElementById("bundlePreferences");
> -        var label = bundlePreferences.getFormattedString("labelDefaultFont", [defaultFont]);
> +      let defaultLabel = defaultFont ?

Use `var` for consistency? Maybe convert everything in this file into `const`/`let` in another commit?
Attachment #8922389 - Flags: review?(timdream) → review+
Comment on attachment 8922747 [details]
Bug 1378366 - Change 'var' to 'const' and 'let' in fontBuilder.js.

https://reviewboard.mozilla.org/r/193902/#review199056
Attachment #8922747 - Flags: review?(timdream) → review+
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1a6103d5708d
If the default font is not found, use 'Default' as the name for an unchanged font preference. Previously this would show up as blank in this case. r=timdream
https://hg.mozilla.org/integration/autoland/rev/170e3ed76a38
Change 'var' to 'const' and 'let' in fontBuilder.js. r=timdream
https://hg.mozilla.org/mozilla-central/rev/1a6103d5708d
https://hg.mozilla.org/mozilla-central/rev/170e3ed76a38
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Flags: qe-verify+
QA Contact: camelia.badau
I have reproduced this issue using Firefox  56.0a1 ar - build (build ID = 20170705095941) on Win 8.1 x64.
I can confirm this issue is fixed, I verified using Firefox 58.0b5 ar build  on Win 8.1 x64, Mac OS X 10.12.6 and Ubuntu 14.04 x64.
Status: RESOLVED → VERIFIED
status-firefox58: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.