1367009 - Sidebar header label is too small

Status: RESOLVED FIXED

(Firefox :: Theme, defect, P1)

Description

[Dão Gottwald [:dao]]

Seeing this on Linux, not sure if this bug manifests on Windows and Mac since I think they have smaller default font sizes.

This is due to browser/themes/shared/sidebar.inc.css hardcoding the font size. It should use the system default instead.

Comment 1

[Dão Gottwald [:dao]]

Attachment: Bug 1367009 - Make the sidebar header use the system font size on Windows and Linux and let it match the sidebar content on Mac.

Review commit: https://reviewboard.mozilla.org/r/141814/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/141814/

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: system-font-size-osx.png

This ends up too small on osx (and windows too IIRC)

Comment 3

[Brian Grinstead [:bgrins]]

Based off of the spec: https://mozilla.invisionapp.com/share/XSBHN694Y#/screens/231191185_Explainer_-_Sidebar

Comment 4

[Brian Grinstead [:bgrins]]

The system font size on osx is smaller than the tree children in the bookmarks tree (tree children are set to 12px, system font size appears to be 11px): https://dxr.mozilla.org/mozilla-central/source/browser/themes/osx/places/places.css#23

Comment 5

[Dão Gottwald [:dao]]

(In reply to Brian Grinstead [:bgrins] from comment #3)
> Based off of the spec:
> https://mozilla.invisionapp.com/share/XSBHN694Y#/screens/
> 231191185_Explainer_-_Sidebar

The Bookmarks label uses the same font size as used in tabs, for instance, i.e. the system font size.

(In reply to Brian Grinstead [:bgrins] from comment #4)
> The system font size on osx is smaller than the tree children in the
> bookmarks tree (tree children are set to 12px, system font size appears to
> be 11px):
> https://dxr.mozilla.org/mozilla-central/source/browser/themes/osx/places/
> places.css#23

Sounds like we should fix the sidebar content to use the system font size too.

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Dão Gottwald [::dao] from comment #5)
> (In reply to Brian Grinstead [:bgrins] from comment #3)
> > Based off of the spec:
> > https://mozilla.invisionapp.com/share/XSBHN694Y#/screens/
> > 231191185_Explainer_-_Sidebar
> 
> The Bookmarks label uses the same font size as used in tabs, for instance,
> i.e. the system font size.
> 
> (In reply to Brian Grinstead [:bgrins] from comment #4)
> > The system font size on osx is smaller than the tree children in the
> > bookmarks tree (tree children are set to 12px, system font size appears to
> > be 11px):
> > https://dxr.mozilla.org/mozilla-central/source/browser/themes/osx/places/
> > places.css#23
> 
> Sounds like we should fix the sidebar content to use the system font size
> too.

OK, let's do that.  Can you update your patch?

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8870367 [details]
Bug 1367009 - Make the sidebar header use the system font size on Windows and Linux and let it match the sidebar content on Mac.

https://reviewboard.mozilla.org/r/141814/#review145592

Clearing review as per Comments 4-6 since this makes the header on osx smaller than the sidebar contents.  I can test on Windows later today

Comment 8

[Dão Gottwald [:dao]]

So on Windows we don't specify a font size in places.css, i.e. we're using the system font size there. That's good. Mac's places.css seems to be deliberately different due to the inaccessibly small default system font size...

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Dão Gottwald [::dao] from comment #8)
> So on Windows we don't specify a font size in places.css, i.e. we're using
> the system font size there. That's good. Mac's places.css seems to be
> deliberately different due to the inaccessibly small default system font
> size...

Should we specify a custom sidebar header font size only on OSX then?

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 8870367 [details]
Bug 1367009 - Make the sidebar header use the system font size on Windows and Linux and let it match the sidebar content on Mac.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141814/diff/1-2/

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 8870367 [details]
Bug 1367009 - Make the sidebar header use the system font size on Windows and Linux and let it match the sidebar content on Mac.

This contains an unintentional change

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 8870367 [details]
Bug 1367009 - Make the sidebar header use the system font size on Windows and Linux and let it match the sidebar content on Mac.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141814/diff/2-3/

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8870367 [details]
Bug 1367009 - Make the sidebar header use the system font size on Windows and Linux and let it match the sidebar content on Mac.

https://reviewboard.mozilla.org/r/141814/#review145606

::: browser/themes/shared/sidebar.inc.css
(Diff revision 3)
>      list-style-image: url("chrome://global/skin/icons/[e-mail]");
>      width: 16px;
>    }
>  }
>  
> -.sidebar-title,

The .sidebar-title removal is unrelated to the fix, please revert the change to this line

Comment 14

[Dão Gottwald [:dao]]

(In reply to Brian Grinstead [:bgrins] from comment #13)
> Comment on attachment 8870367 [details]
> Bug 1367009 - Make the sidebar header use the system font size on Windows
> and Linux and let it match the sidebar content on Mac.
> 
> https://reviewboard.mozilla.org/r/141814/#review145606
> 
> ::: browser/themes/shared/sidebar.inc.css
> (Diff revision 3)
> >      list-style-image: url("chrome://global/skin/icons/[e-mail]");
> >      width: 16px;
> >    }
> >  }
> >  
> > -.sidebar-title,
> 
> The .sidebar-title removal is unrelated to the fix, please revert the change
> to this line

It's dead code. I don't feel like filing another bug and submitting another patch for this one-line drive-by-fix, but perhaps you had planned on doing this?

Comment 15

[Brian Grinstead [:bgrins]]

(In reply to Dão Gottwald [::dao] from comment #14)
> (In reply to Brian Grinstead [:bgrins] from comment #13)
> > Comment on attachment 8870367 [details]
> > Bug 1367009 - Make the sidebar header use the system font size on Windows
> > and Linux and let it match the sidebar content on Mac.
> > 
> > https://reviewboard.mozilla.org/r/141814/#review145606
> > 
> > ::: browser/themes/shared/sidebar.inc.css
> > (Diff revision 3)
> > >      list-style-image: url("chrome://global/skin/icons/[e-mail]");
> > >      width: 16px;
> > >    }
> > >  }
> > >  
> > > -.sidebar-title,
> > 
> > The .sidebar-title removal is unrelated to the fix, please revert the change
> > to this line
> 
> It's dead code. I don't feel like filing another bug and submitting another
> patch for this one-line drive-by-fix, but perhaps you had planned on doing
> this?

This was discussed in https://bugzilla.mozilla.org/show_bug.cgi?id=1355324#c44 - it was kept there since it's used by some addons.  IMO if we want to get rid of this then we should also get rid of all references to .sidebar-header and .sidebar-throbber[loading="true"].  If you don't think these styles are important for addon compat, then let's have a separate commit with the changes (I don't care if it's done in this bug or another though).

Comment 16

[Dão Gottwald [:dao]]

(In reply to Brian Grinstead [:bgrins] from comment #15)
> (In reply to Dão Gottwald [::dao] from comment #14)
> > (In reply to Brian Grinstead [:bgrins] from comment #13)
> > > Comment on attachment 8870367 [details]
> > > Bug 1367009 - Make the sidebar header use the system font size on Windows
> > > and Linux and let it match the sidebar content on Mac.
> > > 
> > > https://reviewboard.mozilla.org/r/141814/#review145606
> > > 
> > > ::: browser/themes/shared/sidebar.inc.css
> > > (Diff revision 3)
> > > >      list-style-image: url("chrome://global/skin/icons/[e-mail]");
> > > >      width: 16px;
> > > >    }
> > > >  }
> > > >  
> > > > -.sidebar-title,
> > > 
> > > The .sidebar-title removal is unrelated to the fix, please revert the change
> > > to this line
> > 
> > It's dead code. I don't feel like filing another bug and submitting another
> > patch for this one-line drive-by-fix, but perhaps you had planned on doing
> > this?
> 
> This was discussed in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1355324#c44 - it was kept there
> since it's used by some addons.  IMO if we want to get rid of this then we
> should also get rid of all references to .sidebar-header and
> .sidebar-throbber[loading="true"].  If you don't think these styles are
> important for addon compat, then let's have a separate commit with the
> changes (I don't care if it's done in this bug or another though).

Most of those add-on references to sidebar-title are in HTML. They're irrelevant here, we never matched them.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eae551d04565
Make the sidebar header use the system font size on Windows and Linux and let it match the sidebar content on Mac. r=bgrins

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eae551d04565

Comment 19

[Saritha]

I've reproduced the issue on an affected build Nightly 55(20170524100215)on Ubuntu 16.04 x64

This is verified fixed in Nightly build 55.0a1 (20170526150217)
-on Ubuntu 16.04 x64 , Windows 10 64bit, Windows 7 64bit

The sidebar header label is of expected size and matches the content font size.

Comment 20

[u554753]

Verified on Windows 10 64bit, Mac OS X 10.13, Ubuntu 16.04 64bit using Nightly 58.0a1 (64-bit) as of this date.