The Symbol "Aa" should always indicate the correct Font availabe for use with the Reader. However, for sans serif the system font is used. (see image) The css in place to set the correct font, gets overwritten by forms.css : : aboutReader.css:241; : .toolbar "Fira Sans",Helvetica,Arial,sans-serif; : forms.css:589; : button -moz-use-system-font; Missing css: .sans-serif-button{ font-family: "Fira Sans",Helvetica,Arial,sans-serif; }

Blake can you take that?

I think I could, yeah.

Hmmm. I don't think I'm seeing that in the style inspector: https://dl.dropboxusercontent.com/u/2301433/Firefox/Reading/FontPanel.png

To be a little clearer, it's asking for Fira, then Helvetica, and we're getting Helvetica, presumably because we don't have Fira available for some reason?

This problem only applies to users that have Fira installed. They will see the article in Fira, but the Symbol still in Helvetica. See comparison between Serif, which works when Charis is installed, vs. Sans-Serif which doesn't. Presumably because the .sans-serif-button class is missing which specifies the font.

Okay, here's the first cut at the patch… Screenshot at https://dl.dropboxusercontent.com/u/2301433/Firefox/Reading/FontPanelV2.png (the horizontal lines are from me using XScope to line things up.)

Comment on attachment 8586962 [details] [diff] [review] The first cut at the patch. And I think I made all the changes Android needs, but it would be good for someone to check that. :)

Comment on attachment 8586962 [details] [diff] [review] The first cut at the patch. Review of attachment 8586962 [details] [diff] [review]: ----------------------------------------------------------------- Looks good on mobile. ::: toolkit/components/reader/AboutReader.jsm @@ +795,5 @@ > let option = options[i]; > > let item = doc.createElement("button"); > > + // We make this extra div so that we can hide it if necessary. I know you just used my comment, but it sounds silly now that I'm reading it :).

:) Okay, I'll change it in the next version of the patch… Any objection to "Put the name in a div so that Android can hide it."?

I see you are aligning the height of the fonts. Do the individual fonts behave very differently? If so, this might be a problem as we do not know which font the user will have available.

Updated comment, and redirecting the review request to Jaws to give Unfocused a bit of a break from my incessant nattering. ;)

(In reply to Markus Jaritz [:maritz] from comment #10) > I see you are aligning the height of the fonts. Do the individual fonts > behave very differently? > If so, this might be a problem as we do not know which font the user will > have available. Yeah, the two fonts there are really quite different in terms of whitespace above and below… Ideally, we'll ship Fira and Charis, so it will all just work at some point in the near future.

So that means that optimizing for both ( Fira and Helvetica/Arial) won't work now? If that is the case we best leave out Fira and Charis as long as we can not ship them, and optimize for Helvetica/Arial and Georgia instead.

Comment on attachment 8587399 [details] [diff] [review] The patch with an updated comment. Review of attachment 8587399 [details] [diff] [review]: ----------------------------------------------------------------- I agree with comment #13, until we are shipping Fira and Charis, we should be tweaking the offsets to make Georgia & Helvetica/Arial look better.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #14) > I agree with comment #13, until we are shipping Fira and Charis, we should > be tweaking the offsets to make Georgia & Helvetica/Arial look better. Yes, I totally agree. While I update those numbers, was there anything else in the CSS or JS I should change to get an r+ next time?

(In reply to Blake Winton (:bwinton) from comment #15) > (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #14) > > I agree with comment #13, until we are shipping Fira and Charis, we should > > be tweaking the offsets to make Georgia & Helvetica/Arial look better. > > Yes, I totally agree. While I update those numbers, was there anything else > in the CSS or JS I should change to get an r+ next time? Maybe remove the references to Fira Sans and Charis until we're shipping those fonts.

Okay, I've removed Fira and Charis, because we're not shipping them, and got Helvetica and Georgia to line up correctly. Screenshot from Mac at https://dl.dropboxusercontent.com/u/2301433/Firefox/Reading/FontPanelV3.png

Hi Blake, can you provide a point value.

Yeah, this ended up being both fiddly and involved, so I'm going to say five points. (If I had to guess beforehand, I probably would have said "one point", because it sounded so simple… :)

So, there's a slight problem. Here's Windows, _if_ I remove the "margin-top: 10px" from "#font-type-buttons > .sans-serif-button > .name". https://dl.dropboxusercontent.com/u/2301433/Firefox/Reading/FontPanelV3.1Win.png If the 10px is there, it gets pushed way down. Jaws: Any thoughts on per-platform CSS? (Is it a good idea? How would we do it?) Thanks!

There are two ways we could do per-platform CSS for this. One way would be to rename these files to be aboutReader.inc.css and then have per-platform aboutReader.css files that %include the *.inc.css file. The other way would be to do %ifndef XP_WIN around the rule that adds the margin-top:10px. %ifndef XP_WIN /* Some comment here about it looking bad with the margin-top on Windows. */ #font-type-buttons > .sans-serif-button > .name { margin-top: 10px; } %endif In this case, I think the %ifndef approach is the least complex and I would prefer it.

Comment on attachment 8589651 [details] [diff] [review] The next version of the patch. Review of attachment 8589651 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the addition of the %ifndef XP_WIN mentioned in the previous comment.

Screenshot of various configurations up at https://dl.dropboxusercontent.com/u/2301433/Firefox/Reading/FontPanelV4.png It was a little more complicated than it seemed, so I'm asking for a re-review, to make sure it's not too complicated to continue down this direction. (Also, how does the file get into the Linux builds when it's not listed in the jar.mn? Like, it seems to work, but I don't know why…)

Comment on attachment 8592427 [details] [diff] [review] the next next version of the patch. Review of attachment 8592427 [details] [diff] [review]: ----------------------------------------------------------------- Stealing. LGTM based on the screenshots. I'm a little nervous about the different fallback fonts continuing to make this iffy but I have no ideas on how to fix that, and at least Helvetica, Arial and Georgia should be reasonably reliably available.

(In reply to Blake Winton (:bwinton) from comment #23) > (Also, how does the file get into the Linux builds when it's not listed in > the jar.mn? Like, it seems to work, but I don't know why…) The linux toolkit theme builds off of the windows toolkit theme, as can be seen here: http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/moz.build

Comment on attachment 8592427 [details] [diff] [review] the next next version of the patch. Looks good! Thanks.

Comment on attachment 8592427 [details] [diff] [review] the next next version of the patch. Approval Request Comment [Feature/regressing bug #]: reading-list [User impact if declined]: The font buttons will be mis-aligned for many users. [Describe test coverage new/current, TreeHerder]: manual [Risks and why]: Reasonably low risk, due to mostly css changes, and simple javascript changes. [String/UUID change made/needed]: None

To fix conflicts with bug 1127234.

Blake, do we still want to uplift this if we're not planning to ship Reading List in 38 or 39? Or does it make sense to uplift anyway to keep this clear ? It may be better to keep all the pending Reading List change on 40. Let me know what you think.

Yes, because this is actually a Reading Mode fix, which I believe we're shipping in 38. (I've stopped working on Reading List stuff for now, so I won't be asking for any uplifts… ;) Thanks!

Should be in 38 beta 6.

The current state of the font panel on Beta 38.0b6-build1 (20150420134330) is as follows: * Ubuntu 14.04 (x64) - http://i.imgur.com/1OHdTBy.png (200%), * Windows 7 (x64) - http://i.imgur.com/11WOT0F.png (200%), * Mac OS X 10.9.5 - http://i.imgur.com/xD3tJFw.png (200%). I think this is reasonable, but then again - I'm not sure exactly how much tolerance we have in terms of mis-alignments. Blake, what do you think? Also, please note that Bug 1142298 might have regressed here, take a look at the bottom-border: http://i.imgur.com/cKusdea.png (there's a pixel escaping at the left side). Reproducible across the specified operating systems.

The alignment seems to me to be as close as we're probably going to get it. I can't see any how the patch posted could be responsible for regressing that bug. Could there be a different patch responsible for its re-appearance?

(In reply to Blake Winton (:bwinton) from comment #38) > The alignment seems to me to be as close as we're probably going to get it. > I can't see any how the patch posted could be responsible for regressing > that bug. Could there be a different patch responsible for its > re-appearance? I'm gonna go ahead and mark this fix as verified fixed then. As for the regression I mentioned, I'll look for a range and file a separate bug for it. Thanks for the quick reply!