1149068 - Reading List Toolbar Sans Serif font selection always displays Helvetica (not the font that is used)

Status: VERIFIED FIXED

(Firefox Graveyard :: Reading List, defect)

Description

[Markus Jaritz [:designakt] (UX)]

Attachment: ReaderView-Toolbar-FontSelection-wrongSansSerif.png

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;
}

Comment 1

[Markus Jaritz [:designakt] (UX)]

Blake can you take that?

Comment 2

[Blake Winton (:bwinton) (:☕️)]

I think I could, yeah.

Comment 3

[Blake Winton (:bwinton) (:☕️)]

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

Comment 4

[Blake Winton (:bwinton) (:☕️)]

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?

Comment 5

[Markus Jaritz [:designakt] (UX)]

Attachment: ReaderView_FiraNotUsed.gif

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.

Comment 6

[Blake Winton (:bwinton) (:☕️)]

Attachment: The first cut at the patch.

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 7

[Blake Winton (:bwinton) (:☕️)]

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 8

[:Margaret Leibovic]

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 :).

Comment 9

[Blake Winton (:bwinton) (:☕️)]

:)  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."?

Comment 10

[Markus Jaritz [:designakt] (UX)]

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.

Comment 11

[Blake Winton (:bwinton) (:☕️)]

Attachment: The patch with an updated comment.

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

Comment 12

[Blake Winton (:bwinton) (:☕️)]

(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.

Comment 13

[Markus Jaritz [:designakt] (UX)]

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 14

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 15

[Blake Winton (:bwinton) (:☕️)]

(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?

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

(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.

Comment 17

[Blake Winton (:bwinton) (:☕️)]

Attachment: The next version of the patch.

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

Comment 18

[Marco Mucci [:MarcoM]]

Hi Blake, can you provide a point value.

Comment 19

[Blake Winton (:bwinton) (:☕️)]

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… :)

Comment 20

[Blake Winton (:bwinton) (:☕️)]

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!

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

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 22

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 23

[Blake Winton (:bwinton) (:☕️)]

Attachment: the next next version of the patch.

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 24

[:Gijs (he/him)]

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.

Comment 25

[Jared Wein [:jaws] (please needinfo? me)]

(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 26

[Markus Jaritz [:designakt] (UX)]

Comment on attachment 8592427 [details] [diff] [review]
the next next version of the patch.

Looks good! Thanks.

Comment 27

[Blake Winton (:bwinton) (:☕️)]

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

Comment 28

[Blake Winton (:bwinton) (:☕️)]

Attachment: Rebased on top of the latest m-c.

To fix conflicts with bug 1127234.

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/be151c6f68b2

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/be151c6f68b2

Comment 31

[Ryan VanderMeulen [:RyanVM]]

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

Comment 32

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 33

[Blake Winton (:bwinton) (:☕️)]

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!

Comment 34

[Sylvestre Ledru [:Sylvestre]]

Should be in 38 beta 6.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8622925c5fe7

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/44de10db57a6

Comment 37

[Andrei Vaida [:avaida]]

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.

Comment 38

[Blake Winton (:bwinton) (:☕️)]

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?

Comment 39

[Andrei Vaida [:avaida]]

(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!