Closed Bug 798903 Opened 7 years ago Closed 6 years ago

Stub installer should use Segoe UI and other newer UI fonts as appropriate when they are available

Categories

(Firefox :: Installer, enhancement)

x86_64
Windows 7
enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 31

People

(Reporter: jaws, Assigned: robert.strong.bugs)

References

Details

(Whiteboard: [stubv2=])

Attachments

(1 file, 2 obsolete files)

The stub installer should use Segoe UI on Win Vista/7. It looks like it is using Arial, which looks out of place and non-native.

See also bug 798866.
Whiteboard: [visual-input-needed]
We use the MS Shell Dlg font. Can you provide a screenshot? Thanks.
Here's a screenshot, http://screencast.com/t/pmyHGMTBLDy.

How complex do you think it would be to use Segoe UI if it is available? It is a much nicer looking font compared to MS Shell Dlg and is the recommended UI font for Windows Vista+.
We have to support multiple locales and have the text fit properly in controls (especially buttons) plus many (maybe most) installers use MS Shell Dlg. As for complexity, it isn't as simple as just using it due to the above.
Severity: normal → enhancement
Whiteboard: [visual-input-needed] → [visual-input-needed], [stub-]
For some background
http://social.msdn.microsoft.com/forums/en-US/windowsuidevelopment/thread/a713cb5c-1f58-4fd0-914c-d84be948ddb1/

My concerns are regarding control sizing and l10n support for all of our locales. We can revisit this after we get other stub installer bugs fixed and there is more breathing room.
Additional info
http://msdn.microsoft.com/en-us/library/windows/desktop/aa511282.aspx

We can likely get away with it for all locales that use Latin, Greek, Cyrillic, and Arabic characters.

There are other fonts for at least some of the other character sets but I would prefer to look into that in a follow up bug.

The other PITA is that we have one UI which now uses MS Shell Dlg 2 for the buttons and should be an 8 pt font while Seqoe UI should never be smaller than 9 pt.
Summary: Stub installer should use Segoe UI on Win Vista/7 → Stub installer should use Segoe UI on Win Vista/7 for locales that use Segoe UI
Summary: Stub installer should use Segoe UI on Win Vista/7 for locales that use Segoe UI → Stub installer should use Segoe UI on Win Vista/7 for locales that can use Segoe UI
Whiteboard: [visual-input-needed], [stub-] → [visual-input-needed], [stubv2-]
With how bad the font looks in bug 996374 I am going to up the priority of this.
Whiteboard: [visual-input-needed], [stubv2-] → [visual-input-needed], [stubv2=]
Attached patch patch rev1 (obsolete) — Splinter Review
Don't let perfect be the enemy of good or in this case better. This tries to use the correct font for the locale if it is available for everything that is dynamically sized which is the majority of text (does not include the buttons). To do the buttons will require a significant amount of work to size the controls based on the font used.
Assignee: nobody → robert.strong.bugs
Status: NEW → ASSIGNED
Attachment #8407956 - Flags: review?(tabraldes)
Note: this fixes bug 996374 as well.
One of the many sources I used to determine the appropriate fonts
http://msdn.microsoft.com/en-us/library/windows/apps/dn263115.aspx

I also went through all of the locales to visually verify.
Comment on attachment 8407956 [details] [diff] [review]
patch rev1

Review of attachment 8407956 [details] [diff] [review]:
-----------------------------------------------------------------

r=me as long as the comments are addressed - especially the $0/$1 issue.

::: browser/installer/windows/nsis/stub.nsi
@@ +418,5 @@
>    StrCpy $CheckboxInstallMaintSvc "0"
>  !endif
>    StrCpy $WasOptionsButtonClicked "0"
>  
> +  StrCpy $0 ""

I notice that we're not saving the previous value of $0. Elsewhere in this function we seem trash some other registers. Is that because .onInit is always called first and is only called once?

@@ +452,5 @@
> +  ${EndIf}
> +
> +  CreateFont $FontBlurb "$1" "12" "500"
> +  CreateFont $FontNormal "$1" "11" "500"
> +  CreateFont $FontItalic "$1" "11" "500" /ITALIC

Here we're using $1 but above we're placing the font name into $0. Is that intentional? It looks like we're passing the wrong value to `CreateFont`

::: toolkit/mozapps/installer/windows/nsis/locale-fonts.nsh
@@ +320,5 @@
> +!define FONT_FILE1 "meiryo.ttc"
> +!define FONT_NAME2 "MS UI Gothic"
> +!define FONT_FILE2 "msgothic.ttc"
> +!define FONT_NAME2 "MS Mincho"
> +!define FONT_FILE2 "msmincho.ttc"

Typo: 2 is repeated instead of having a 2 and a 3

@@ +416,5 @@
> +!if "${AB_CD}" == "mn"
> +!define FONT_NAME1 "Segoe UI"
> +!define FONT_FILE1 "segoeui.ttf"
> +!define FONT_NAME1 "Mongolian Baiti"
> +!define FONT_FILE1 "monbaiti.ttf"

Typo: 1 is repeated instead of having a 1 and a 2

@@ +461,5 @@
> +!define FONT_NAME1 "Segoe UI"
> +!define FONT_FILE1 "segoeui.ttf"
> +!endif
> +
> +; Southern Ndebele nr

It looks like we don't know of a font for this language. Maybe make the comment mention that?
Attachment #8407956 - Flags: review?(tabraldes) → review+
(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #11)
> Comment on attachment 8407956 [details] [diff] [review]
> patch rev1
> 
> Review of attachment 8407956 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me as long as the comments are addressed - especially the $0/$1 issue.
> 
> ::: browser/installer/windows/nsis/stub.nsi
> @@ +418,5 @@
> >    StrCpy $CheckboxInstallMaintSvc "0"
> >  !endif
> >    StrCpy $WasOptionsButtonClicked "0"
> >  
> > +  StrCpy $0 ""
> 
> I notice that we're not saving the previous value of $0. Elsewhere in this
> function we seem trash some other registers. Is that because .onInit is
> always called first and is only called once?
Yes and the installer files can do what they want with the registers. It is in the common code where the registers should be preserved after the call to the common code.

> 
> @@ +452,5 @@
> > +  ${EndIf}
> > +
> > +  CreateFont $FontBlurb "$1" "12" "500"
> > +  CreateFont $FontNormal "$1" "11" "500"
> > +  CreateFont $FontItalic "$1" "11" "500" /ITALIC
> 
> Here we're using $1 but above we're placing the font name into $0. Is that
> intentional? It looks like we're passing the wrong value to `CreateFont`
Meh, researching all of the languages and fonts kicked my butt today.
Fixed

> ::: toolkit/mozapps/installer/windows/nsis/locale-fonts.nsh
> @@ +320,5 @@
> > +!define FONT_FILE1 "meiryo.ttc"
> > +!define FONT_NAME2 "MS UI Gothic"
> > +!define FONT_FILE2 "msgothic.ttc"
> > +!define FONT_NAME2 "MS Mincho"
> > +!define FONT_FILE2 "msmincho.ttc"
> 
> Typo: 2 is repeated instead of having a 2 and a 3
Fixed

> @@ +416,5 @@
> > +!if "${AB_CD}" == "mn"
> > +!define FONT_NAME1 "Segoe UI"
> > +!define FONT_FILE1 "segoeui.ttf"
> > +!define FONT_NAME1 "Mongolian Baiti"
> > +!define FONT_FILE1 "monbaiti.ttf"
> 
> Typo: 1 is repeated instead of having a 1 and a 2
Fixed

> @@ +461,5 @@
> > +!define FONT_NAME1 "Segoe UI"
> > +!define FONT_FILE1 "segoeui.ttf"
> > +!endif
> > +
> > +; Southern Ndebele nr
> 
> It looks like we don't know of a font for this language. Maybe make the
> comment mention that?
Will do.
Attached patch patch - updated to comments (obsolete) — Splinter Review
I managed to find info on Southern Ndebele
Attachment #8407956 - Attachment is obsolete: true
Attachment #8407987 - Flags: review+
I removed the additional fallback fonts since they typically look worse than the current font. If we decide there is value in additional fallback fonts they can be added in another bug.
Attachment #8407987 - Attachment is obsolete: true
Attachment #8408500 - Flags: review+
Summary: Stub installer should use Segoe UI on Win Vista/7 for locales that can use Segoe UI → Stub installer should use Segoe UI and other newer UI fonts as appropriate when they are available
Pushed to fx-team
https://hg.mozilla.org/integration/fx-team/rev/3f06dd410b78
Flags: in-testsuite-
Target Milestone: --- → Firefox 31
Whiteboard: [visual-input-needed], [stubv2=] → [stubv2=]
https://hg.mozilla.org/mozilla-central/rev/3f06dd410b78
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Duplicate of this bug: 996374
You need to log in before you can comment on or make changes to this bug.