Closed
Bug 798903
Opened 12 years ago
Closed 10 years ago
Stub installer should use Segoe UI and other newer UI fonts as appropriate when they are available
Categories
(Firefox :: Installer, enhancement)
Tracking
()
RESOLVED
FIXED
Firefox 31
People
(Reporter: jaws, Assigned: robert.strong.bugs)
References
Details
(Whiteboard: [stubv2=])
Attachments
(1 file, 2 obsolete files)
15.49 KB,
patch
|
robert.strong.bugs
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•12 years ago
|
Blocks: StubInstaller
Whiteboard: [visual-input-needed]
Assignee | ||
Comment 1•12 years ago
|
||
We use the MS Shell Dlg font. Can you provide a screenshot? Thanks.
Reporter | ||
Comment 2•12 years ago
|
||
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+.
Assignee | ||
Comment 3•12 years ago
|
||
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
Updated•12 years ago
|
Whiteboard: [visual-input-needed] → [visual-input-needed], [stub-]
Assignee | ||
Comment 4•12 years ago
|
||
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.
Assignee | ||
Comment 5•11 years ago
|
||
Additional background http://forums.winamp.com/showthread.php?t=285009
Assignee | ||
Comment 6•10 years ago
|
||
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
Assignee | ||
Updated•10 years ago
|
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
Assignee | ||
Updated•10 years ago
|
Whiteboard: [visual-input-needed], [stub-] → [visual-input-needed], [stubv2-]
Assignee | ||
Comment 7•10 years ago
|
||
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=]
Assignee | ||
Comment 8•10 years ago
|
||
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)
Assignee | ||
Comment 9•10 years ago
|
||
Note: this fixes bug 996374 as well.
Assignee | ||
Comment 10•10 years ago
|
||
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 11•10 years ago
|
||
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+
Assignee | ||
Comment 12•10 years ago
|
||
(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.
Assignee | ||
Comment 13•10 years ago
|
||
I managed to find info on Southern Ndebele
Attachment #8407956 -
Attachment is obsolete: true
Attachment #8407987 -
Flags: review+
Assignee | ||
Comment 14•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
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
Assignee | ||
Comment 15•10 years ago
|
||
Pushed to fx-team https://hg.mozilla.org/integration/fx-team/rev/3f06dd410b78
Flags: in-testsuite-
Target Milestone: --- → Firefox 31
Assignee | ||
Updated•10 years ago
|
Whiteboard: [visual-input-needed], [stubv2=] → [stubv2=]
Comment 16•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3f06dd410b78
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•