Closed Bug 1367009 Opened 6 years ago Closed 6 years ago

Sidebar header label is too small

Categories

(Firefox :: Theme, defect, P1)

All
Linux
defect

Tracking

()

RESOLVED FIXED
Firefox 55
Iteration:
55.6 - May 29
Tracking Status
firefox55 --- fixed
firefox58 --- verified

People

(Reporter: dao, Assigned: dao)

References

Details

(Whiteboard: [reserve-photon-structure])

Attachments

(2 files)

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.
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Attachment #8870367 - Flags: review?(bgrinstead)
This ends up too small on osx (and windows too IIRC)
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
(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.
(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 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
Attachment #8870367 - Flags: review?(bgrinstead)
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...
(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 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
Attachment #8870367 - Flags: review?(bgrinstead)
Whiteboard: [photon-structure]
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/loading@2x.png");
>      width: 16px;
>    }
>  }
>  
> -.sidebar-title,

The .sidebar-title removal is unrelated to the fix, please revert the change to this line
Attachment #8870367 - Flags: review?(bgrinstead) → review+
(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/loading@2x.png");
> >      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?
Flags: needinfo?(bgrinstead)
(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/loading@2x.png");
> > >      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).
Flags: needinfo?(bgrinstead)
Whiteboard: [photon-structure] → [photon-structure] [triage]
(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/loading@2x.png");
> > > >      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.
Pushed by dgottwald@mozilla.com:
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
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [photon-structure] [triage] → [reserve-photon-structure]
Flags: qe-verify? → qe-verify+
QA Contact: gwimberly
https://hg.mozilla.org/mozilla-central/rev/eae551d04565
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Iteration: --- → 55.6 - May 29
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.
Verified on Windows 10 64bit, Mac OS X 10.13, Ubuntu 16.04 64bit using Nightly 58.0a1 (64-bit) as of this date.
You need to log in before you can comment on or make changes to this bug.