Closed Bug 1149068 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox Graveyard :: Reading List, defect)

38 Branch
x86
macOS
defect
Not set

Tracking

(firefox38 verified, firefox39 fixed, firefox40 fixed)

VERIFIED FIXED
Firefox 40
Tracking Status
firefox38 --- verified
firefox39 --- fixed
firefox40 --- fixed

People

(Reporter: designakt, Assigned: bwinton)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 4 obsolete files)

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?
Flags: needinfo?(bwinton)
I think I could, yeah.
Assignee: nobody → bwinton
Flags: needinfo?(bwinton)
Status: NEW → ASSIGNED
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.
Attached patch The first cut at the patch. (obsolete) — Splinter Review
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.)
Attachment #8586962 - Flags: ui-review?(mjaritz)
Attachment #8586962 - Flags: review?(bmcbride)
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.  :)
Attachment #8586962 - Flags: review?(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 :).
Attachment #8586962 - Flags: review?(margaret.leibovic) → review+
:)  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.  ;)
Attachment #8586962 - Attachment is obsolete: true
Attachment #8586962 - Flags: ui-review?(mjaritz)
Attachment #8586962 - Flags: review?(bmcbride)
Attachment #8587399 - Flags: ui-review?(mjaritz)
Attachment #8587399 - Flags: review?(jaws)
(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.
Attachment #8587399 - Flags: review?(jaws) → review-
(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?
Flags: needinfo?(jaws)
(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.
Flags: needinfo?(jaws)
Attached patch The next version of the patch. (obsolete) — Splinter Review
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
Attachment #8587399 - Attachment is obsolete: true
Attachment #8587399 - Flags: ui-review?(mjaritz)
Attachment #8589651 - Flags: ui-review?(mjaritz)
Attachment #8589651 - Flags: review?(jaws)
Hi Blake, can you provide a point value.
Blocks: 1132074
Iteration: --- → 40.1 - 13 Apr
Flags: qe-verify?
Flags: needinfo?(bwinton)
Flags: firefox-backlog+
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… :)
Points: --- → 5
Flags: needinfo?(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!
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.
Attachment #8589651 - Flags: review?(jaws) → review+
Iteration: 40.1 - 13 Apr → 40.2 - 27 Apr
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…)
Attachment #8589651 - Attachment is obsolete: true
Attachment #8589651 - Flags: ui-review?(mjaritz)
Attachment #8592427 - Flags: ui-review?(mjaritz)
Attachment #8592427 - Flags: review?(florian)
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.
Attachment #8592427 - Flags: review?(florian) → review+
(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.
Attachment #8592427 - Flags: ui-review?(mjaritz) → ui-review+
Keywords: checkin-needed
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
Attachment #8592427 - Flags: approval-mozilla-beta?
Attachment #8592427 - Flags: approval-mozilla-aurora?
To fix conflicts with bug 1127234.
Attachment #8592427 - Attachment is obsolete: true
Attachment #8592427 - Flags: approval-mozilla-beta?
Attachment #8592427 - Flags: approval-mozilla-aurora?
Attachment #8593168 - Flags: ui-review+
Attachment #8593168 - Flags: review+
Attachment #8593168 - Flags: approval-mozilla-beta?
Attachment #8593168 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/be151c6f68b2
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
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.
Flags: needinfo?(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!
Flags: needinfo?(bwinton)
Attachment #8593168 - Flags: approval-mozilla-beta?
Attachment #8593168 - Flags: approval-mozilla-beta+
Attachment #8593168 - Flags: approval-mozilla-aurora?
Attachment #8593168 - Flags: approval-mozilla-aurora+
Should be in 38 beta 6.
QA Contact: andrei.vaida
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.
Flags: needinfo?(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?
Flags: needinfo?(bwinton)
(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!
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.