Closed Bug 1392532 Opened 7 years ago Closed 7 years ago

The font size on Linux is too huge, compared to Mac and Windows.

Categories

(Firefox :: Settings UI, defect, P1)

55 Branch
defect

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 + fixed

People

(Reporter: evanxd, Assigned: rickychien)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-preference])

Attachments

(5 files, 1 obsolete file)

Attached image linux-preferences.png
Since we use "rem" unit to set the font size in Photon visual refresh work, the actual font size might be too huge when the default system font size is too huge too.

The default font size on Ubuntu is 14.6667px, Windows is 12px, and Mac is 11px.

The "linux-preferences.png" attachment is the screenshot of Preferences page on Linux (Ubuntu 16.04). The Windows (Windows 10) and Mac (macOS 10.12.5) ones will be updated later. And we could see the font size might be too huge, compared to Windows and Mac.

Discussed with Photon Preferences visual designer, Helen in person, we would like to update the the font size for each OS platform to optimize our visual refresh work. We don't have the specific spec yet, we will also need to get feedback from Helen in this bug for the font size info on each OS platform.
Attached image windows-preferences.png
Attached image mac-preferences.png
Summary: The font size on Linux is too huge compare to Mac and Windows. → The font size on Linux is too huge, compared to Mac and Windows.
Blocks: 1388414
Flags: qe-verify-
Whiteboard: [photon-preference][triage] → [photon-preference]
No longer blocks: 1388414
See Also: → 1388414
Blocks: 1392601
Comment on attachment 8899731 [details]
Bug 1392532 - Set specific font size with rem unit for each OS platform to optimize Photon visual refresh work since the default system font size values are different on different OS platform.

https://reviewboard.mozilla.org/r/171060/#review176654

::: toolkit/themes/linux/global/in-content/common.css:142
(Diff revision 5)
> +
> +input,
> +.indent,
> +.help-button,
> +.indent > description {
> +  font-size: 0.97rem;

It looks weird / inconsistent that we're making the text input fields smaller than the surrounding text. Can we stop doing that?

Decreasing the text size for .indent looks pretty random too.
Comment on attachment 8899731 [details]
Bug 1392532 - Set specific font size with rem unit for each OS platform to optimize Photon visual refresh work since the default system font size values are different on different OS platform.

Hi Helen,

This is the font size update for each OS platform. Could you review this visual change?

You can download the binary from the try[1], and you also check the screenshot of each OS platform.

1. Linux: http://i.imgur.com/YesXwWg.png
2. Windows: http://i.imgur.com/y3ScHDE.png
3. Mac: http://i.imgur.com/gOC0RjX.png

Thank you.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea9f3c319d21
Attachment #8899731 - Flags: ui-review?(hhuang)
Comment on attachment 8899731 [details]
Bug 1392532 - Set specific font size with rem unit for each OS platform to optimize Photon visual refresh work since the default system font size values are different on different OS platform.

https://reviewboard.mozilla.org/r/171060/#review176672

::: toolkit/themes/linux/global/in-content/common.css:142
(Diff revision 5)
> +
> +input,
> +.indent,
> +.help-button,
> +.indent > description {
> +  font-size: 0.97rem;

Hi Dão,

This is a visual design issue. Let's get feeback from our visual designer, Helen.
Hi Helen,

What do you think of Comment 10?

Thank you.
Flags: needinfo?(hhuang)
Hi Dão,

Since we didn't refresh the components in this version, I reuse the current search field without changing it. So we could consider fixing this once the components are updated. For the text size of the description, It is intended to make it smaller than Body, which could help users to differentiate it with the setting items at a glance.
Comment on attachment 8899731 [details]
Bug 1392532 - Set specific font size with rem unit for each OS platform to optimize Photon visual refresh work since the default system font size values are different on different OS platform.

I noticed that the string "Nightly is currently..." is slightly small, it should be as big as body text, and set in semibold font. Please fix it.
Flags: needinfo?(hhuang)
Attachment #8899731 - Flags: ui-review?(hhuang) → ui-review-
Blocks: 1393409
Blocks: 1393370
Comment on attachment 8899731 [details]
Bug 1392532 - Set specific font size with rem unit for each OS platform to optimize Photon visual refresh work since the default system font size values are different on different OS platform.

Hi Helen,

I've updated the patch for the review comments.
Could you help review it again?

The below is the screenshot of
Linux: http://imgur.com/a/yESFv
Windows: http://imgur.com/a/gkJ6r
Mac: http://imgur.com/a/v3FtI

Thank you.
Attachment #8899731 - Flags: ui-review- → ui-review?(hhuang)
Attachment #8899731 - Flags: review?(jaws)
Hi Jared,

Helen already had reviewed the patch previously on Comment 15. And I've updated it for the review comments. (She haven't reviewed it again because she is on PTO today.)

Could you help review it to see if we have any technical issue there?

Thank you.
(In reply to Helen Huang [:HHuang] from comment #15)
> Comment on attachment 8899731 [details]
> Bug 1392532 - Set specific font size with rem unit for each OS platform to
> optimize Photon visual refresh work since the default system font size
> values are different on different OS platform.
> 
> I noticed that the string "Nightly is currently..." is slightly small, it
> should be as big as body text, and set in semibold font. Please fix it.

As my understand of the comment, the visual change is good to Helen except the font size and weight of the "Nightly is currently..." string.
Blocks: 1393474
Comment on attachment 8899731 [details]
Bug 1392532 - Set specific font size with rem unit for each OS platform to optimize Photon visual refresh work since the default system font size values are different on different OS platform.

https://reviewboard.mozilla.org/r/171060/#review178710

::: toolkit/themes/linux/global/in-content/common.css:142
(Diff revision 8)
> +
> +input,
> +.indent,
> +.help-button,
> +.indent > description {
> +  font-size: 0.97rem;

We should remove input, .indent and .indent > description from this rule.
Comment on attachment 8899731 [details]
Bug 1392532 - Set specific font size with rem unit for each OS platform to optimize Photon visual refresh work since the default system font size values are different on different OS platform.

They look good to me, thanks!
Attachment #8899731 - Flags: ui-review?(hhuang) → ui-review+
Comment on attachment 8899731 [details]
Bug 1392532 - Set specific font size with rem unit for each OS platform to optimize Photon visual refresh work since the default system font size values are different on different OS platform.

Redirecting to Dao since he's already taken a couple looks at the patch.
Attachment #8899731 - Flags: review?(jaws) → review?(dao+bmo)
According visual spec, the font size of (2) email address (15px) [1] should be smaller than user name (17px). Evan, can you update the font size for Firefox account page along with your patch?

Thanks


[1] https://mozilla.invisionapp.com/share/X8BGCX9PD#/screens/249684858
Flags: needinfo?(evan)
(In reply to Dão Gottwald [::dao] from comment #23)
> Comment on attachment 8899731 [details]
> Bug 1392532 - Set specific font size with rem unit for each OS platform to
> optimize Photon visual refresh work since the default system font size
> values are different on different OS platform.
> 
> https://reviewboard.mozilla.org/r/171060/#review178710
> 
> ::: toolkit/themes/linux/global/in-content/common.css:142
> (Diff revision 8)
> > +
> > +input,
> > +.indent,
> > +.help-button,
> > +.indent > description {
> > +  font-size: 0.97rem;
> 
> We should remove input, .indent and .indent > description from this rule.

Hi Dão,

What do you think of Comment 14 from Helen, our visual designer. This style rule is what we want for Photon design.
Flags: needinfo?(evan) → needinfo?(dao+bmo)
(In reply to Ricky Chien [:rickychien] from comment #26)
> According visual spec, the font size of (2) email address (15px) [1] should
> be smaller than user name (17px). Evan, can you update the font size for
> Firefox account page along with your patch?
> 
> Thanks
> 
> 
> [1] https://mozilla.invisionapp.com/share/X8BGCX9PD#/screens/249684858

Sure, I'll update it.
Follow up the previous comment, the style of the components will keep the original design. I check the search bars in Firefox, the text of search bar in sidebars and addons page is smaller than body text, for the consistency, I strongly suggest to keep it as it is for now. However, the input field inside of the content area should use the font size for body text, which also follows the original design.

For the description, currently we have the guideline for the font usage (http://design.firefox.com/photon/visual/typography.html#styles), and so it will be applied in Firefox not just in Preferences. Dão, hope you can take it into consideration and look forward to having your feedback.
Attachment #8899731 - Flags: review?(jaws)
Attachment #8899731 - Flags: review?(dao+bmo)
(In reply to Helen Huang [:HHuang] from comment #29)
> Follow up the previous comment, the style of the components will keep the
> original design. I check the search bars in Firefox, the text of search bar
> in sidebars

I believe this is a special case for the sidebar on Mac. We don't do this on Windows and Linux, nor is it the case that all search fields on Mac should have smaller text.

> and addons page is smaller than body text, for the consistency,

This is because about:addons uses the same stylesheet as preferences, so that's not a useful reference.

(In reply to Evan Tseng [:evanxd] from comment #22)
> (In reply to Helen Huang [:HHuang] from comment #15)
> > Comment on attachment 8899731 [details]
> > Bug 1392532 - Set specific font size with rem unit for each OS platform to
> > optimize Photon visual refresh work since the default system font size
> > values are different on different OS platform.
> > 
> > I noticed that the string "Nightly is currently..." is slightly small, it
> > should be as big as body text, and set in semibold font. Please fix it.
> 
> As my understand of the comment, the visual change is good to Helen except
> the font size and weight of the "Nightly is currently..." string.

This is because that text uses the indent class. As mentioned earlier, we should stop making .indent smaller than the surrounding text.
Flags: needinfo?(dao+bmo)
Hi Dão,

I update the patch for the review comments. Could you take a look?

I've removed the `input`, `.indent`, and `.indent > description` from the rule and added the new rules to match our Photon visual design.

According to Photon's visual design, we still need a specific style rule for some additional information, like the `These settings are tailored to your computer’s hardware and operating system.` string in Performance groupbox in General pane of Preferences page. See it in the attachment. Same thing for the search inputs on Preferences and about:addons pages.

What do you think of our new change?

Thank you.
> 
> According to Photon's visual design, we still need a specific style rule for
> some additional information, like the `These settings are tailored to your
> computer’s hardware and operating system.` string in Performance groupbox in
> General pane of Preferences page. See it in the attachment. Same thing for
> the search inputs on Preferences and about:addons pages.

As UX defined, we need to provide more information to describe the function for some settings, and defined it should be smaller than body text, to have some differentiation. But it doesn't mean all the indents should be small, only the sentence is defined as descriptions. I think Evan already helps to fix it.

For the search bar on the top right corner, it's the only one I made the text a bit smaller to align with about:addons; however, this change should not affect the rest of the input fields, all the text of the input field should be as big as body text, except for this one.
Flags: needinfo?(dao+bmo)
(In reply to Helen Huang [:HHuang] from comment #34)
> As UX defined, we need to provide more information to describe the function
> for some settings, and defined it should be smaller than body text, to have
> some differentiation. But it doesn't mean all the indents should be small,
> only the sentence is defined as descriptions. I think Evan already helps to
> fix it.

Okay, so instead of overloading indent, we should have a class specifically for those cases.

> For the search bar on the top right corner, it's the only one I made the
> text a bit smaller to align with about:addons; however, this change should
> not affect the rest of the input fields, all the text of the input field
> should be as big as body text, except for this one.

Modelling about:preferences after about:addons is nonsensical because it's a circular reference. As mentioned earlier, about:preferences and about:addons share a stylesheet, and the search field in about:addons is smaller only because that stylesheet currently makes all input fields smaller for no good reason that I can see.
Flags: needinfo?(dao+bmo)
Yes, I agree with you that all the input field should not be smaller. 

I only ask for decreasing the font size for the "Find in Preferences" search field. There are some reasons that we found the string "Fine in Preferences" will be truncated in some languages in current font size, so we increased the width of the search field. If the text is set as big as body text for now, it needs to get much wider. That might effect the visual hierarchy. Your thoughts?
Flags: needinfo?(dao+bmo)
(In reply to Helen Huang [:HHuang] from comment #36)
> Yes, I agree with you that all the input field should not be smaller. 
> 
> I only ask for decreasing the font size for the "Find in Preferences" search
> field. There are some reasons that we found the string "Fine in Preferences"
> will be truncated in some languages in current font size, so we increased
> the width of the search field. If the text is set as big as body text for
> now, it needs to get much wider. That might effect the visual hierarchy.
> Your thoughts?

It might be best to let locales control this like here:

http://searchfox.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.dtd#904-909

http://searchfox.org/mozilla-central/source/browser/base/content/urlbarBindings.xml#2663
Flags: needinfo?(dao+bmo)
Since I'm on the PTO, Ricky will continue to help on this issue. Thank you very much, Ricky.
Assignee: evan → rchien
Comment on attachment 8899731 [details]
Bug 1392532 - Set specific font size with rem unit for each OS platform to optimize Photon visual refresh work since the default system font size values are different on different OS platform.

https://reviewboard.mozilla.org/r/171060/#review180718
Attachment #8899731 - Flags: review?(dao+bmo)
During SV mid-nightly sign offs, this bug was listed among a few others as a P1 bug that should be fixed before pre-beta sign off of "preferences visual refresh (Photon)" feature. Tracked for 57.

FYI JGong.
Flags: needinfo?(jgong)
Attachment #8899731 - Attachment is obsolete: true
(In reply to Dão Gottwald [::dao] from comment #37)
> It might be best to let locales control this like here:
> 
> http://searchfox.org/mozilla-central/source/browser/locales/en-US/chrome/
> browser/browser.dtd#904-909
> 
> http://searchfox.org/mozilla-central/source/browser/base/content/
> urlbarBindings.xml#2663

Nice trick for addressing l10n issue! Source location has been changed recently, here is the original example with permalink:

http://searchfox.org/mozilla-central/rev/999385a5e8c2d360cc37286882508075fc2078bd/browser/locales/en-US/chrome/browser/browser.dtd#910-915

http://searchfox.org/mozilla-central/rev/999385a5e8c2d360cc37286882508075fc2078bd/browser/base/content/urlbarBindings.xml#2676


Dao,

I've submitted the updated patch based on your suggestions, please review it kindly!

* Removed specific font size for search input (all font-size of textbox in preferences are using the same size now)
* Default width (English) of search input is 230px, this width is dynamic and will be changed depending on language. (Spec will update soon)
Comment on attachment 8904100 [details]
Bug 1392532 - Polish Preferences font size depending on platforms

https://reviewboard.mozilla.org/r/175868/#review181056

::: browser/locales/en-US/chrome/browser/preferences/preferences.dtd:19
(Diff revision 2)
>  <!ENTITY  prefWinMinSize.styleGNOME     "width: 45.5em; min-height: 40.5em;">
>  
> +<!-- LOCALIZATION NOTE: (searchField.width): This is used to determine the width
> +     of the search field in about:preferences, in order to make entire placeholder
> +     string visible -->
> +<!ENTITY  searchField.width             "230px">

Please convert this to an em value

::: toolkit/themes/linux/global/in-content/common.css:140
(Diff revision 2)
> +  font-size: 1.27rem;
> +}
> +
> +.tip-caption,
> +.help-button {
> +  font-size: 0.97rem;

I don't think we should go below 1rem. Can you remove this rule (Linux only)?
Comment on attachment 8904100 [details]
Bug 1392532 - Polish Preferences font size depending on platforms

https://reviewboard.mozilla.org/r/175868/#review181056

> Please convert this to an em value

Ah, this is a new l10n property so it doesn't make any sense to revert it back.
(In reply to Ricky Chien [:rickychien] from comment #46)
> Comment on attachment 8904100 [details]
> Bug 1392532 - Polish Preferences font size depending on platforms
> 
> https://reviewboard.mozilla.org/r/175868/#review181056
> 
> > Please convert this to an em value
> 
> Ah, this is a new l10n property so it doesn't make any sense to revert it
> back.

I don't understand what this means. Dimensions that depend on the text size should always be in em.
Comment on attachment 8904100 [details]
Bug 1392532 - Polish Preferences font size depending on platforms

https://reviewboard.mozilla.org/r/175868/#review181056

> Ah, this is a new l10n property so it doesn't make any sense to revert it back.

Sorry, I misread the sentence. em value has been updated.

> I don't think we should go below 1rem. Can you remove this rule (Linux only)?

I'm going to set it to 1rem instead of going below 1rem.

According to visual design spec, font-size of .tip-caption & .help-button should be a slightly smaller than general page font size (it's currently set to 1.11 rem at line 123 within this file).
BTW, I'm curious about why `em` value is the best unit to fit our need? What I can see the difference after using em value is that the actual width of search field went to different value on different system. Ubuntu is 252px but macOS is 231px. It causes a different look and feel which isn't what we want. Or doesn't any solution we can solve this issue?
Patch has updated again and finally I figured out a way to set `em` value for all platforms with using #ifdef.

The current patch has been tested under all platforms and looks great.
(In reply to Ricky Chien [:rickychien] from comment #50)
> BTW, I'm curious about why `em` value is the best unit to fit our need? What
> I can see the difference after using em value is that the actual width of
> search field went to different value on different system. Ubuntu is 252px
> but macOS is 231px.

This should be expected since the text sizes are different. After all we want the placeholder to be visible and the user to type comfortably in the text field, both of which depends on the text size.

(In reply to Ricky Chien [:rickychien] from comment #52)
> Patch has updated again and finally I figured out a way to set `em` value
> for all platforms with using #ifdef.

Please revert this. :/ Localizers shouldn't have to worry about this; it's not even clear what platform differences they should comply with here in the first place.
Sure! Those platform specific `em` values have been reverted. IIRC, we're going to accept different search field width according to platforms since it's reasonable to expect that the width should be dynamic and adjusted corresponding to text size.
Comment on attachment 8904100 [details]
Bug 1392532 - Polish Preferences font size depending on platforms

https://reviewboard.mozilla.org/r/175868/#review181182

Thanks!
Attachment #8904100 - Flags: review?(dao+bmo) → review+
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9aa2e9106aa3
Polish Preferences font size depending on platforms r=dao
Blocks: 1397121
Depends on: 1397135
Depends on: 1398050
No longer depends on: 1398050
Depends on: 1399110
Comment on attachment 8904100 [details]
Bug 1392532 - Polish Preferences font size depending on platforms

https://reviewboard.mozilla.org/r/175868/#review184354

::: toolkit/themes/osx/global/in-content/common.css:132
(Diff revision 6)
>  xul|textbox[type="search"]:not([searchbutton]) > .textbox-input-box > .textbox-search-sign {
>    list-style-image: url(chrome://global/skin/icons/search-textbox.svg);
>    margin-inline-end: 5px;
>  }
>  
> +html|html *,

This selector has high specificity and regresses all font-size rules that use a simple class selector (for example). One such regression is the heading text in about:privatebrowsing.

I'm going to file a bug, but in the meantime: Dao, do you think we should revert this, or reduce the specificity of this selector somehow? I haven't yet investigated but I'm afraid that it'll be hard to track down all the regressions, though I'm hoping this won't be the case...
Please see comment 61.
Flags: needinfo?(dao+bmo)
(In reply to Nihanth Subramanya [:nhnt11] from comment #61)
> Comment on attachment 8904100 [details]
> Bug 1392532 - Polish Preferences font size depending on platforms
> 
> https://reviewboard.mozilla.org/r/175868/#review184354
> 
> ::: toolkit/themes/osx/global/in-content/common.css:132
> (Diff revision 6)
> >  xul|textbox[type="search"]:not([searchbutton]) > .textbox-input-box > .textbox-search-sign {
> >    list-style-image: url(chrome://global/skin/icons/search-textbox.svg);
> >    margin-inline-end: 5px;
> >  }
> >  
> > +html|html *,
> 
> This selector has high specificity and regresses all font-size rules that
> use a simple class selector (for example). One such regression is the
> heading text in about:privatebrowsing.

See bug 1397325 comment 12.
Flags: needinfo?(dao+bmo)
(In reply to Ritu Kothari (:ritu) from comment #41)
> During SV mid-nightly sign offs, this bug was listed among a few others as a
> P1 bug that should be fixed before pre-beta sign off of "preferences visual
> refresh (Photon)" feature. Tracked for 57.
> 
> FYI JGong.
The bug has been fixed.
Flags: needinfo?(jgong)
You need to log in before you can comment on or make changes to this bug.