1265326 - Font indicator showing "xxx, sans-serif (not installed)" where xxx *IS* installed.

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Jorg K (CEST = GMT+2)]

What this https://www.youtube.com/watch?v=aceySKlkqo4 from 1:45.

Also read bug 1148790 comment #45 and further down.

Comment 1

[Jorg K (CEST = GMT+2)]

Correction: Watch this https://www.youtube.com/watch?v=aceySKlkqo4 from 1:45.

Comment 2

[Jorg K (CEST = GMT+2)]

I had already suggested to do something about it: attachment 8651474 [details] [diff] [review]

     // Bug 1139524: Normalise before we compare: Make it lower case
     // and replace ", " with "," so that entries like
     // "Helvetica, Arial, sans-serif" are always recognised correctly
     var stateToLower = state.toLowerCase().replace(/, /g, ",");
 
+    // Also strip generic names.
+    stateToLower = stateToLower.replace(/,serif/,"");
+    stateToLower = stateToLower.replace(/,sans-serif/,"");
+    stateToLower = stateToLower.replace(/,monospace/,"");

Comment 3

[Jorg K (CEST = GMT+2)]

(In reply to Aceman from a private message)
> Also, we may want to allow the user to use font specification
> like "Verdana, sans-serif" with a single click (from the font menu)
> as it is better than just "Verdana". Just maybe the "not installed" 
> label is misleading.
I don't have a strong opinion on that. However, the font menu so far allows the specification of a font name, never a family. To get a family one needs to use "Helvetica, Arial", "Courier" or times "Times". These are the only families we support. And not even there does the family name show in the UI.

If the HTML contains "Verdana, sans-serif" the current indicator shows "Verdana, sans-serif (not installed)". That is just plain wrong. It should show "Verdana, sans-serif" for the purists and simply "Verdana" for the simple-minded non-Linux users like myself, which have never seen "sans-serif", "serif" or "monospace" in their lives since they don't speak HTML and know nothing about font families ;-)

Comment 4

[:aceman]

I meant IF the msg already contains "Verdana, sans-serif" then it should get added to the list and allow the user to continue using it. Not that somehow he would be able to manually add such combinations to the font menu.

Comment 5

[:aceman]

Now I can ask if the solution should be restricted to the font families only.
Or if there is a font "Verdana, Arial" in the msg, and the user only has Verdana installed.
Should it get the "(not installed)" string added? I'd say not, which is what the bug report says.

However, what if the font is "Arial, Verdana" and he only has Verdana? Maybe we could write "(alternative installed)"?

So the idea is to split the font definition on the commas and analyse if any of the single fonts are found. Then we could append a more clever string.

This would also cover the reported case. If we first look for Verdana and find it, no need to check "sans-serif".

Comment 7

[KNTRO]

This bug is still present in Earlybird 47.0a2 (2016-04-18).

This happens with any font, not with the most famous only.

For instance, I have installed Source Sans Pro and in fact it's the font I choose as default for both displaying & composing. However, when I begin to compose an email, it shows "serif,Source Sans Pro (not installed)".

If I choose any other font —like, let's say Sansation— I will get the very same results.

I think this bug appeared for the first time in a few previous Earlybird releases, probably between versions 40 to 45.

Comment 8

[Jorg K (CEST = GMT+2)]

This was introduced with bug 1265198. It is present in all versions of TB from the landing of that bug. No need for further investigation. The source of the problem is known.

Comment 9

[Kent James (:rkent)]

bug 1265198 is a dup of this bug. What is the correct regression bug please?

Comment 10

[:aceman]

However, it is not known why KNTRO gets "serif" added BEFORE the font name. That's a new scenario I do not understand yet.

Comment 11

[:aceman]

(In reply to Kent James (:rkent) from comment #9)
> bug 1265198 is a dup of this bug. What is the correct regression bug please?

This feature was introduced in bug 1148790, TB43. So it is not a real regression if you compare with previous release of 38.

Comment 12

[KNTRO]

(In reply to :aceman from comment #10)

I also forget to say that, despite the font I choose, it won't change the font of the message's body, as you can see in this GIF:
http://www104.zippyshare.com/v/HjAFsjwz/file.html

Comment 13

[:aceman]

Can you attach the gif here? Firefox marks the zippyshare.com site as suspicious.

Comment 14

[KNTRO]

Attachment: Earlybird serif,[font name] (not installed).gif

Comment 15

[KNTRO]

(In reply to :aceman from comment #13)
Zippyshare.com, suspicious?! Wow, it's one of the top most secure filesharing websites out there!

Anyway, I attached the GIF here. I should have done this before. Shame on me! :P

Comment 16

[:aceman]

Are you on linux?

Comment 17

[KNTRO]

(In reply to :aceman from comment #16)
Windows 10 Home 64 bits, version 1511, build 10586.218.

Comment 18

[:aceman]

Jorg, can you reproduce the case shown in the gif? I couldn't in Win XP.

Comment 19

[Jorg K (CEST = GMT+2)]

It's an animated GIF with a whole video!
So what I see is that the user selects "FontA" as font, and after a little while, the display changes to "serif,FontA (not installed)". That's the weirdest thing I've seen in a while and I can't reproduce this neither in TB 45.0 (official release) nor the TB 48 Daily I'm using normally (since I'm a good guy and decided to be test rabbit myself) on my Win 7 (64bit) system.

Since the reporter said in comment #7 that he uses a specific font by default for composing and displaying, I did the same and I still can't see it.

This is really weird. What we display in the font indicator is what the underlying M-C editor returns and I'm surprised that it should prepend the "serif". On the other hand, I've never seen the fonts used in the video. Is there something special about them?

Reporter:
Does this happen in safe mode? Also, can you send such a message to yourself as HTML and attach the received message or at least paste the relevant HTML here, so we can see whether it's really has <font face="serif,Whatever">.
And does it happen for every font, even the stock-standard ones like "Arial"?

Comment 20

[KNTRO]

(In reply to Jorg K (GMT+2) from comment #19)
> I've never seen the fonts used in the video. Is there something special about them?

Nah, just a bunch of fonts downloaded from Google or other free font downloading sites. They all work perfect on Firefox, LibreOffice, Notepad++, etc.

> Does this happen in safe mode?

Yeah, I'm afraid so. :(

> Also, can you send such a message to yourself as HTML and attach the received message or at least paste the relevant HTML here, so we can see whether it's really has <font face="serif,Whatever">.

Sure! I attached it here. I only edited my email address in that file. I replaced my real Gmail address by "[e-mail]".

> And does it happen for every font, even the stock-standard ones like "Arial"?

That's correct. I'm attaching a screenshot as well.

Comment 21

[KNTRO]

Attachment: Test message.eml

Comment 22

[KNTRO]

Attachment: serif,Arial (not installed).png

Comment 23

[Jorg K (CEST = GMT+2)]

Thanks for that. Did you look at the HTML, I'll paste it here:

<p><font face="Source Sans Pro"><font face="Source Sans Pro"><font
            face="Arial"></font></font></font>Hello world! :P</p>

Something has gone wrong with this. I'll see whether I can reproduce it. If I can, I'll raise a new bug for it.

BTW, you're using Earlybird TB 47. Have you tried with TB45 or the latest daily?

Comment 24

[KNTRO]

(In reply to Jorg K (GMT+2) from comment #23)
> Something has gone wrong with this. I'll see whether I can reproduce it. If I can, I'll raise a new bug for it.

Oh, I see now.

> BTW, you're using Earlybird TB 47. Have you tried with TB45 or the latest daily?

No, sir. However, I will overwrite Earlybird 47.0a2's installation with Thunderbird 45.0's tonight. I'll let you know if this crazy behavior happens with Tb45 as well, or if it doesn't.

Comment 25

[:aceman]

I can try to implement my proposal in comment 5.

I am not sure now if KNTRO's problem belongs here (it will not be fixed by the patch I propose). We do not know why it happens so maybe it could be moved to a new bug.

Comment 26

[Jorg K (CEST = GMT+2)]

I got Abacus from here (http://www.fontpalace.com/font-download/Abacus/) and I don't see the problem, neither with Arial. BTW, that Abacus is shocking, doesn't work, neither in TB nor OpenOffice.

Comment 27

[Jorg K (CEST = GMT+2)]

OK, I promised a reply, so here it is:

(In reply to :aceman from comment #5)
> So the idea is to split the font definition on the commas and analyse if any
> of the single fonts are found. Then we could append a more clever string.
There is a problem, the fixed items:
"Helvetica, Arial, sans-serif"
"Courier New, Courier, monospace"
"Times New Roman, Times, serif".
These we do not want to split.
Otherwise yes.

> This would also cover the reported case. If we first look for Verdana and
> find it, no need to check "sans-serif".
Agreed.

We also know that "sans-serif", "serif" and "monospace" are not valid font names, so they will never be found (we disabled them on Linux, remember?), so that part can be stripped already.

We could also consider stripping the remaining two family names: "cursive" and "fantasy":
https://www.w3.org/Style/Examples/007/fonts.en.html#font-family

My approach was to just strip them and be done. I don't think the case "Verdana, Arial" is realistic, but anyway. So my approach:

1) recognise "Helvetica, Arial, sans-serif" plus the other two mentioned above and do nothing with them.
2) strip "sans-serif", "serif" and "monospace" (with their comma, of course).
3) If then there is more than one in the list, show the first one found.
4) If all fails, show the original with "(not installed)".
Remember, before TB 38 the font indicator did not update at all for a non-recognised font. That was terrible.

Comment 28

[:aceman]

(In reply to Jorg K (GMT+2) from comment #27)
> (In reply to :aceman from comment #5)
> > So the idea is to split the font definition on the commas and analyse if any
> > of the single fonts are found. Then we could append a more clever string.
> There is a problem, the fixed items:
> "Helvetica, Arial, sans-serif"
> "Courier New, Courier, monospace"
> "Times New Roman, Times, serif".
> These we do not want to split.
> Otherwise yes.

If the message contains "Helvetica, Arial, sans-serif" and we search the menulist top to bottom, then we will find and match "Helvetica, Arial, sans-serif" in the list first. Only if a complete match is not found, 

> My approach was to just strip them and be done. I don't think the case
> "Verdana, Arial" is realistic, but anyway.
Surely somebody will use that in their email art works :)

> So my approach:
> 
> 1) recognise "Helvetica, Arial, sans-serif" plus the other two mentioned
> above and do nothing with them.
OK.

> 2) strip "sans-serif", "serif" and "monospace" (with their comma, of course).
I'm not yet sure if this is needed. I make a patch without this and see if something is not working.

> 3) If then there is more than one in the list, show the first one found.
If the msgs contains font xxx,yyy and we do have xxx in the list, I still add "xxx,yyy" into the "used fonts" block, but will not mark it "not installed".

> 4) If all fails, show the original with "(not installed)".
Yes.

Comment 29

[:aceman]

Attachment: patch

So this is something to play with.

Comment 30

[Jorg K (CEST = GMT+2)]

Comment on attachment 8743010 [details] [diff] [review]
patch

Sorry, I don't like it.

My real life test case comes from Outlook:
<font face="Comic Sans MS,sans-serif">
YOur indicator shows exactly that. Please, can we remove the family names as I suggested. They are internal stuff for HTML and CSS connoisseurs, nothing the user wants to see in the interface. The font used is the first one, not the generic one, so let's use that.

Equally, your "Arial,Verdana" shows exactly that. No, the font is "Arial". There aren't two fonts in use.
And if I use "XArial,Verdana" (where XArial" is not installed), I get "XArial,Verdana (alternative installed)". Also, the user will be confused. The font used is Verdana, and that needs to show.

Overall, we don't need to be able to compose in the exact same font "conglomeration". We can reply in the font that we use out of the ones which are possible.

Also, we need this in TB 45, so no string changes, please.

Comment 31

[:aceman]

(In reply to Jorg K (GMT+2) from comment #30)
> Comment on attachment 8743010 [details] [diff] [review]
> patch
> 
> Sorry, I don't like it.
> 
> My real life test case comes from Outlook:
> <font face="Comic Sans MS,sans-serif">
> YOur indicator shows exactly that.

Yes, that was my proposal.

> Equally, your "Arial,Verdana" shows exactly that. No, the font is "Arial".
> There aren't two fonts in use.

But the recipient may use Verdana for this same <font> tag. Why not loose this information (two alternative fonts) when we can keep it?

> And if I use "XArial,Verdana" (where XArial" is not installed), I get
> "XArial,Verdana (alternative installed)". Also, the user will be confused.
> The font used is Verdana, and that needs to show.
> 
> Overall, we don't need to be able to compose in the exact same font
> "conglomeration".

Why not? Isn't the point of the used fonts block in the picker to show which fonts (and conglomerations) are used in the document? So that the user can quickly use them again in other parts of the text? Keeping the conglomerates should be a great feature.

I could accept showing just the one used font as the label on the menuitem, but keep the whole string as the value so that all the alternatives can still be put in the html source (silently).

But that may become strange, e.g. both "Arial, sans-serif" and "Arial, Verdana" would show as 2 entries of Arial in the list.

Comment 32

[Jorg K (CEST = GMT+2)]

People want to use the composition window like a word processor. I tried my OpenOffice. You can type "A;B" into the font window and it gets pasted into HTML as "A,B". Well. How about that ;-)

But if you type "Arial,sans-serif", OpenOffice removes the "sans-serif" bit, so it does strip those generic names.

So perhaps I can live with the "A,B", since I don't think the will happen that often. But I'd like to see the family names stripped.

BTW, what about the Format > Font menu? That doesn't select "Comic Sans MS" in my test case. Shouldn't they both work and both show the same information? Only where we we have none of the fonts, should the Font menu show nothing.

Also, if you have "XArial,sans-serif" you should not show the "(not installed)" since "sans-serif" can always be used. Not installed if is none of the alternatives is installed.

Comment 33

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #31)
> Why not? Isn't the point of the used fonts block in the picker to show which
> fonts (and conglomerations) are used in the document? So that the user can
> quickly use them again in other parts of the text? Keeping the conglomerates
> should be a great feature.

IMHO, the used fonts block is there to give easy access to the fonts used in the composition, not the fonts used in the message being replied to. If we can match "Donald,Duck" and use "Duck" in the composition, then we should show "Duck" in all the menus. If we return "Donald,Duck" the recipient may view the message with "Donald", but the sender wrote with "Duck".

I repeat: Other than the three "conglomerates" "Helvetica/etc.", "Courier/etc." and "Times/etc." we don't support conglomerates. We should transmit exactly the font used in the composition.

You saw Alex video: He likes the fact that he has easy access to previously used fonts. Showing and retransmitting a conglomerate since we couldn't find any matches should be the very last resort. And, as I said, don't show "(not installed)" when defaulting to a generic family.

Comment 34

[Jorg K (CEST = GMT+2)]

Maybe Alex can comment here. What would you like to see in the font indicator? The font used in the display in the composition, or something font specification (conglomerate) "quoted" from an e-mail that's being replied to?

BTW, why should be replies any different to new messages? In new messages you can only select a single font and the font menu shows the ones you've used so far.

Comment 35

[:aceman]

It doesn't need to be replies. The user may have a specially crafted template with his favourite "conglomerates" and then just use them via our font picker.

Comment 36

[Jorg K (CEST = GMT+2)]

Yes, that is true. But such a template can't be constructed in TB. Also, such a template wouldn't have worked until now, since before TB 45 ESR (or TB 43 when bug 1148790 landed) the font indicator didn't change when such a conglomerate wasn't detected. Why do you want to confuse 99.5% of the users with a hypothetical case that doesn't exist?

Let me restate what I think is useful and not confusing. I still believe we should transmit exactly what is shown in the font indicator.

Tag encountered   Font indicator                  Font Menu
===============   ==============                  =========
Arial             Arial                           Arial
Arial,sans-serif  Arial                           Arial
Arial,Verdana     Arial                           Arial
XArial,Verdana    Verdana                         Verdana
XArial,sans-serif XArial,sans-serif               (no selection) (*)
XArial            XArial (not installed)          (no selection)
XArial,YCourier   XArial,YCourier (not installed) (no selection)

*) Note: "XArial,sans-serif" will hopefully use the configured "Sans Serif" font. Alternatively you could show that, so the user gets displayed what they *see*.

Richard, do you have an opinion?

Comment 37

[Richard Marti (:Paenglab)]

I'm for showing it as simple as possible and showing only the used font, also for the (*) case. If somebody is using a template with alternative fonts, he normally don't need to change the font as all he needs is already defined in his template.

Comment 38

[Jorg K (CEST = GMT+2)]

(In reply to Richard Marti (:Paenglab) from comment #37)
> showing only the used font, also for the (*) case.
That would be ideal, but is more work. The premise is that the default to the configured "sans-serif" font actually works, which I'd haven't tried. But I will try it right now.

Comment 39

[Jorg K (CEST = GMT+2)]

Yes, it works. In a windows-1252 encoded e-mail I set the "Sans serif" of the Latin writing system to a special font, and it is in fact used for a text in "XArial,sans-serif". If the e-mail were UTF-8, I'd have to configure it under "Other Writing Systems".

Comment 40

[KNTRO]

(In reply to Jorg K (GMT+2) from comment #23)
> BTW, you're using Earlybird TB 47. Have you tried with TB45 or the latest daily?

Jorg, gentlemen,

I have just tried this on Thunderbird 45.0 and I have bad news: yep, this bug happens on this version as well. :(

I'm not techie at all, so I can't even guess where is this bug exactly. I wish I could help with this bug further, but I'm afraid I don't have the technical knowledge to do so. I'm sorry if I'm not useful debugging this issue.

Comment 41

[Jorg K (CEST = GMT+2)]

Let's move this issue to a new bug: Bug 1266024.

Comment 42

[Jorg K (CEST = GMT+2)]

Can we settle for a solution here? This needs to land before the end of Sunday since branch day is on Monday. Kent wants to land everything on Tuesday to go into TB 45 beta.x and then land everything on TB 45.1 one week later.

If you don't have time, I can propose a patch, but it would be done my way ;-), see comment #36 and Richard's comment #37 (although I don't know that this is easily possible).

Comment 43

[:aceman]

(In reply to Jorg K (GMT+2) from comment #36)
> Tag encountered   Font indicator                  Font Menu
> ===============   ==============                  =========
> Arial             Arial                           Arial
> Arial,sans-serif  Arial                           Arial
> Arial,Verdana     Arial                           Arial
> XArial,Verdana    Verdana                         Verdana
> XArial,sans-serif XArial,sans-serif               (no selection) (*)
> XArial            XArial (not installed)          (no selection)
> XArial,YCourier   XArial,YCourier (not installed) (no selection)

Sorry I did not completely understand this.
Tell me which column is the name displayed in the font menu (menulist) and what is the 'value' attribute behind the menuitem. The font that will be applied when user chooses the item.

Also,what should happen if there are ALL these combinations in a single message? Will will become 3 'Arial' items in the font menu?

Comment 44

[Jorg K (CEST = GMT+2)]

Let's see again:

Tag encountered   Font indicator                  Font Menu
===============   ==============                  =========
Arial             Arial                           Arial
Arial,sans-serif  Arial                           Arial
Arial,Verdana     Arial                           Arial
XArial,Verdana    Verdana                         Verdana
XArial,sans-serif XArial,sans-serif               (no selection) (option 1)
XArial,sans-serif Arial                           Arial (option 2 where the configured
                                                         font for sans-serif is displayed)
XArial            XArial (not installed)          (no selection)
XArial,YCourier   XArial,YCourier (not installed) (no selection)

OK, tag encountered is the value reported by the M-C editor, so "state" in the program.

What is displayed in the font indicator (and the font menu, although I didn't look at that in great detail) is what you see in the 2nd and 3rd column. In the JS code it's called "fontLabel" for the indicator.

Now, what should the value be? "fontMenuValue" in the JS code. Well, I suggest to make it the same as the label with the exception that "(not installed)" doesn't get appended to the value, of course.

So with a mix of "Arial", "Arial,sans-serif", "Arial,Verdana", and even "XArial,sans-serif" (assuming that the default sans-serif font is "Arial"), the resulting menu will show Arial once only. If the user picks that, the message will be composed in Arial.

OK, I already hear you shouting: But what if the user replies to a section in "Arial,sans-serif" and wants to send the out as well. Answer: We (or at least I) don't support that. We only support three *fixed* conglomerates, all other fonts are single fonts only, no families appended.

The original idea of Bug 1148790 - "Font indicator doesn't update when cursor is placed into text with unsupported font in message composer" was to make the font indicator react to any font we place the cursor in. As you've seen in Alex' video, users are also excited that they now get a "used" fonts menu, and I personally find that very useful too. The idea was *not* to enable sending in the same font as was received, although I have the impression that you saw it that way.

I repeat: The original bug was about giving the user some useful feedback, but it has become much to geeky and we need to cut it down to something everyone will understand. IMHO, users want the font indicator to respond and show them the name of the font they are *seeing*. Nothing more and nothing less ;-)

Comment 45

[:aceman]

(In reply to Jorg K (GMT+2) from comment #44)
> Now, what should the value be? "fontMenuValue" in the JS code. Well, I
> suggest to make it the same as the label with the exception that "(not
> installed)" doesn't get appended to the value, of course.

I understand.

> So with a mix of "Arial", "Arial,sans-serif", "Arial,Verdana", and even
> "XArial,sans-serif" (assuming that the default sans-serif font is "Arial"),
> the resulting menu will show Arial once only. If the user picks that, the
> message will be composed in Arial.

I understand.

> OK, I already hear you shouting: But what if the user replies to a section
> in "Arial,sans-serif" and wants to send the out as well. Answer: We (or at
> least I) don't support that.

But we easily could, unless the price in UI clutter is too high. You think it is.

> The original idea of Bug 1148790 - "Font indicator doesn't update when
> cursor is placed into text with unsupported font in message composer" was to
> make the font indicator react to any font we place the cursor in. As you've
> seen in Alex' video, users are also excited that they now get a "used" fonts
> menu, and I personally find that very useful too. The idea was *not* to
> enable sending in the same font as was received, although I have the
> impression that you saw it that way.

Yes, I see that as a very useful feature.

But I do not actually compose any HTML msgs :) So I want to hear Axel first as he is a heavy HTML user.

Comment 46

[Jorg K (CEST = GMT+2)]

I NIed Axel before an he didn't react. Anyway, perhaps you have more luck.

> But we easily could, unless the price in UI clutter is too high. You think it is.
It's not so much "clutter" as in many items to choose from, it's about confusion. Most users don't speak HTML and they wouldn't have the faintest idea what these family names are. I don't see why a reply should be any different to a new message, were you can send a defined set of fonts, nothing you picked up from somewhere else (yes, unless you hand-craft a template).

Our UI man has already spoken and he said:
I'm for showing it as simple as possible and showing only the used font, also for the (*) case.

Comment 47

[Axel Grude [:realRaven]]

(In reply to Jorg K (GMT+2) from comment #46)
> I NIed Axel before an he didn't react. Anyway, perhaps you have more luck.
> 
> > But we easily could, unless the price in UI clutter is too high. You think it is.
> It's not so much "clutter" as in many items to choose from, it's about
> confusion. Most users don't speak HTML and they wouldn't have the faintest
> idea what these family names are. I don't see why a reply should be any
> different to a new message, were you can send a defined set of fonts,
> nothing you picked up from somewhere else (yes, unless you hand-craft a
> template).
> 
> Our UI man has already spoken and he said:
> I'm for showing it as simple as possible and showing only the used font,
> also for the (*) case.

I agree, as simple as possible in the UI - show the first item in the tag (Arial or Arial Unicode). If we can extrapolate the kind of font, can the ",sans serif" be automatically added in the markup anyway?

> We only support three *fixed* conglomerates, all other fonts are single fonts only, no families appended.

If this means the answer is "no", I do not think it is a big deal. Just the simple font name will do.

Finally I don't think users would care whether a font is installed or not, they will get instant feedback from the font name and what it does in their HTML composition anyway, so I would drop the "not installed" as well. Within the "quick list" simple and short is the sweet spot. If people want to do more elaborate stuff they can scroll down in the list or edit the HTML.

Comment 48

[Jorg K (CEST = GMT+2)]

Thanks for the reply.

(In reply to Axel Grude [:realRaven] from comment #47)
> I agree, as simple as possible in the UI - show the first item in the tag
> (Arial or Arial Unicode). If we can extrapolate the kind of font, can the
> ",sans serif" be automatically added in the markup anyway?
This is a nice idea. If we were able to derive the font family, we could add it. I suggest to propose this in another bug, if technically possible, and hide it behind an option.

> Finally I don't think users would care whether a font is installed or not,
> they will get instant feedback from the font name and what it does in their
> HTML composition anyway, so I would drop the "not installed" as well.
In my proposal, the "not installed" is only shown if the font is really not installed and if there is no replacement via an alternative or font family. In this case it would be good to alert the user to the fact. Then they can decide whether they want to use the uninstalled font and not see a proper representation while they write, or use a different font.

Comment 49

[Axel Grude [:realRaven]]

(In reply to Jorg K (GMT+2) from comment #48)
> Thanks for the reply.
> 
> (In reply to Axel Grude [:realRaven] from comment #47)
> > I agree, as simple as possible in the UI - show the first item in the tag
> > (Arial or Arial Unicode). If we can extrapolate the kind of font, can the
> > ",sans serif" be automatically added in the markup anyway?
> This is a nice idea. If we were able to derive the font family, we could add
> it. I suggest to propose this in another bug, if technically possible, and
> hide it behind an option.
> 
> > Finally I don't think users would care whether a font is installed or not,
> > they will get instant feedback from the font name and what it does in their
> > HTML composition anyway, so I would drop the "not installed" as well.
> In my proposal, the "not installed" is only shown if the font is really not
> installed and if there is no replacement via an alternative or font family.
> In this case it would be good to alert the user to the fact. Then they can
> decide whether they want to use the uninstalled font and not see a proper
> representation while they write, or use a different font.

I guess it is okay if it works. When I made my video (ad hoc) I was little taken aback by the fact that it would report that the fonts in an email I had sent to myself on the same system were shown as actually "not installed":

https://youtu.be/aceySKlkqo4?t=1m58s

but I quickly glossed over that. :)

Comment 50

[Jorg K (CEST = GMT+2)]

(In reply to Axel Grude [:realRaven] from comment #49)
> When I made my video (ad hoc) I was little
> taken aback by the fact that it would report that the fonts in an email I
> had sent to myself on the same system were shown as actually "not installed":
That video made me raise this bug here (after I had already detected bug 1265181).

How did you generate the "Verdana, sans-serif"? Where does the "sans-serif" come from? From a template?

Comment 51

[Jorg K (CEST = GMT+2)]

If your "Verdana, sans-serif" really came from a template, then my approach is too simplistic. Then we should do this:

Simplistic:
Tag encountered   Font indicator                  Font Menu
===============   ==============                  =========
Arial             Arial                           Arial
Arial,sans-serif  Arial                           Arial
Arial,Verdana     Arial                           Arial
XArial,Verdana    Verdana                         Verdana
XArial,sans-serif Arial                           Arial (where the configured
                                                         font for sans-serif is displayed)
XArial            XArial (not installed)          (no selection)
XArial,YCourier   XArial,YCourier (not installed) (no selection)

More geeky:
Tag encountered   Font indicator                  Font Menu
===============   ==============                  =========
Arial             Arial                           Arial
Arial,sans-serif  Arial,sans-serif                Arial
Arial,Verdana     Arial                           Arial
XArial,Verdana    Verdana                         Verdana
XArial,sans-serif Arial,sans-serif                Arial (where the configured
                                                         font for sans-serif is displayed)
XArial            XArial (not installed)          (no selection)
XArial,YCourier   XArial,YCourier (not installed) (no selection)

So we preserve family names in the indicator but not the font menu. I'm sure Aceman can settle for that ;-)
Preserving family names makes sense, preserving conglomerates doesn't make sense IMHO.

Comment 52

[Axel Grude [:realRaven]]

(In reply to Jorg K (GMT+2) from comment #50)
> (In reply to Axel Grude [:realRaven] from comment #49)
> > When I made my video (ad hoc) I was little
> > taken aback by the fact that it would report that the fonts in an email I
> > had sent to myself on the same system were shown as actually "not installed":
> That video made me raise this bug here (after I had already detected bug
> 1265181).
> 
> How did you generate the "Verdana, sans-serif"? Where does the "sans-serif"
> come from? From a template?

Actually, the email I REPLY to (and which has Verdana and ) it was on the same system but in a different Thunderbird profile on the current Thunderbird beta. Funny thing is, I cannot reproduce how I added the "sans-serif" all of a sudden. As far as I remember I simply selected it directly from the dropdown when I edited the original Email.

I may have looked at the HTML source in order to check the markup for the different font sizes, btu I honestly do not remember whether I edited and may have added the ", sans-serif" manually. Otherwise I can't really explain how they got there.

    <p style="font-family: Verdana, sans-serif; font-size:16px;">
      This is a Test in Verdana font.</p>
    <p style="font-family: Aharoni, sans-serif; font-size:18px;">
      And here some more text in Aharoni font. </p>

(the sans serif is definitely _not_ added automatically when I simply choose Verdana or Aharoni from the drop down)

Comment 53

[Jorg K (CEST = GMT+2)]

I copied the snippet of HTML into a message and it's interesting to see that the font indicator also responds to CSS. Good to know.

Well, the question is whether we should go for the "simplistic" solution or the more "geeky" one.
Aceman prefers the geeky one as it gives more power to the user, and I did prefer the simplistic one, but I'm not 100% sure any more since the words "sans-serif" and "serif" already show in the user interface.

So repeating: Since font families do make sense I'd be happy to support them. I'm not a fan of conglomerates. The user should see which font they are using, appending the font family shows them that they will send the e-mail with additional information, which might be beneficial to the recipient.

Aceman, when are we going to get it? I'm happy to review it. Or I'm happy to write it if you don't have time.

Comment 54

[Jorg K (CEST = GMT+2)]

I've been thinking about this more. We said that we wanted to retrieve the font the system is using for display in case of "XArial,sans-serif". Most likely this is not easily achievable since the font defaulting happens somewhere in the M-C display engine and is based on the document encoding and language to derive the writing system and from it the configured fonts. So for a quick fix I now propose the following:

More geeky simplified:
Tag encountered   Font indicator                    Font Menu
===============   ==============                    =========
Arial             Arial                             Arial
Arial,sans-serif  Arial,sans-serif                  Arial
Arial,Verdana     Arial                             Arial
XArial,Verdana    Verdana                           Verdana
XArial,sans-serif XArial,sans-serif (not installed) (no selection)
XArial            XArial (not installed)            (no selection)
XArial,YCourier   XArial,YCourier (not installed)   (no selection)

Aceman, can we deliver that in the next two days?
Values are always as the label without the "(not installed)".

"Not installed" signifies automatically that something else is used, since of course we do display the text with some default setting.

If you don't mind, I will add a patch for the Font Menu today, so we can split the effort, OK?

Comment 55

[Jorg K (CEST = GMT+2)]

Attachment: Test cases for font indicator

Comment 56

[Jorg K (CEST = GMT+2)]

Attachment: Test cases for font indicator (v2)

Comment 57

[Jorg K (CEST = GMT+2)]

Attachment: Simple patch to fix font menu according to comment #54 (v1).

This fixes the font menu. The font indicator is more complex and Aceman's territory.

Aceman, please review and if you like it, please include in your patch.

Comment 58

[:aceman]

Yes, I'll look at this today.

Comment 59

[Jorg K (CEST = GMT+2)]

Attachment: Simple patch to fix font menu according to comment #54 (v2).

Renamed badly-named variables, optimisation so string is only split once, added comments.

Aceman, please note: There are only ever *two* alternatives, since "alter" is from Latin: the other one. So your variable name fontAlternatives should be changed.

Comment 60

[:aceman]

Comment on attachment 8744290 [details] [diff] [review]
Simple patch to fix font menu according to comment #54 (v2).

Review of attachment 8744290 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/ui/composer/content/editor.js
@@ +860,4 @@
>        }
>      }
>  
> +    var editorFontOptions = null;

Why is this defined here and not in the inner loop?

@@ +884,5 @@
> +              editorFontOptions = editorFont.split(',');
> +            var foundMatch = false;
> +            for (var j = 0; j < editorFontOptions.length; j++)
> +            {
> +              if (menuFont == editorFontOptions[j])

You could probably use editorFontOptions.includes(menuFont) here.

Anyway, do I understand this correctly that if you get a font string live "Verdana, Arial", you have both in the list, but Arial is the first item matched (due to alphabetic order of the menu), so you select Arial in the menu? Even though Verdana is actually used to render because it is installed too.

Comment 61

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #60)
> Why is this defined here and not in the inner loop?
Because the inner loop runs over 100+ menu items and I want to split it only once when needed.

> You could probably use editorFontOptions.includes(menuFont) here.
Not really, since I need to find the first match.

> Anyway, do I understand this correctly that if you get a font string like
> "Verdana, Arial", you have both in the list, but Arial is the first item
> matched (due to alphabetic order of the menu), so you select Arial in the
> menu? Even though Verdana is actually used to render because it is installed
> too.
Damn, looks like I got it wrong and I also need another test case.
Good we have reviews. New patch coming.

Comment 62

[Jorg K (CEST = GMT+2)]

Attachment: Test cases for font indicator (v3)

Comment 63

[:aceman]

(In reply to Jorg K (GMT+2) from comment #54)
> More geeky simplified:
> Tag encountered   Font indicator                    Font Menu
> ===============   ==============                    =========
> Arial             Arial                             Arial
> Arial,sans-serif  Arial,sans-serif                  Arial
> Arial,Verdana     Arial                             Arial
> XArial,Verdana    Verdana                           Verdana
> XArial,sans-serif XArial,sans-serif (not installed) (no selection)
> XArial            XArial (not installed)            (no selection)
> XArial,YCourier   XArial,YCourier (not installed)   (no selection)

What about the case of "Verdana,Arial,sans-serif" ?

(In reply to Jorg K (GMT+2) from comment #61)
> > You could probably use editorFontOptions.includes(menuFont) here.
> Not really, since I need to find the first match.
What is first match? You do not use 'j' (the match position) in any way. But you probably should, as I commented. I actually need to do the same thing in my patch.

Comment 64

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #63)
> (In reply to Jorg K (GMT+2) from comment #54)
> > More geeky simplified:
> > Tag encountered   Font indicator                    Font Menu
> > ===============   ==============                    =========
> > Arial             Arial                             Arial
> > Arial,sans-serif  Arial,sans-serif                  Arial
> > Arial,Verdana     Arial                             Arial
> > XArial,Verdana    Verdana                           Verdana
> > XArial,sans-serif XArial,sans-serif (not installed) (no selection)
> > XArial            XArial (not installed)            (no selection)
> > XArial,YCourier   XArial,YCourier (not installed)   (no selection)
> 
> What about the case of "Verdana,Arial,sans-serif" ?

Tag encountered            Font indicator                    Font Menu
===============            ==============                    =========
Verdana,Arial,sans-serif   Verdana,sans-serif                Verdana

We show the font used but carry the family forward.

Comment 65

[Jorg K (CEST = GMT+2)]

Attachment: Patch to fix font menu according to comment #54 (v3).

This fixes the logic error Aceman detected.

We select the item in the menu if it's in the list of font options, but if we find another menu entry later which matches an earlier option, we select it instead.

Comment 66

[:aceman]

Comment on attachment 8744489 [details] [diff] [review]
Patch to fix font menu according to comment #54 (v3).

Review of attachment 8744489 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/ui/composer/content/editor.js
@@ +884,5 @@
> +            if (!editorFontOptions)
> +              editorFontOptions = editorFont.split(',');
> +            for (var j = 0; j < editorFontOptions.length; j++)
> +            {
> +              if (menuFont == editorFontOptions[j])

What about j=editorFontOptions.indexOf(menuFont) ?

@@ +891,5 @@
> +                // prefer it.
> +                if (j < matchedOption) {
> +                  menuItem.setAttribute("checked", "true");
> +                  matchedOption = j;
> +                  break;

As an optimization, if matchedOption == 1 you could exit from the i loop.

Comment 67

[:aceman]

(In reply to :aceman from comment #66)
> As an optimization, if matchedOption == 1 you could exit from the i loop.

Or maybe 0 :)

Comment 68

[Jorg K (CEST = GMT+2)]

Attachment: Patch to fix font menu according to comment #54 (v4).

Incorporated Aceman's suggestions. Also fixed another logic error in clearing the check mark.

Comment 69

[:aceman]

Comment on attachment 8744515 [details] [diff] [review]
Patch to fix font menu according to comment #54 (v4).

Review of attachment 8744515 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/ui/composer/content/editor.js
@@ +902,5 @@
>          }
>  
> +        // In case this item doesn't match, make sure we've cleared the checkmark.
> +        if (!matchFound)
> +          menuItem.removeAttribute("checked");

Not your fault, but I wonder if this unchecking is at the right level in the 'i' loop. Do we need to uncheck all menuitems that are NOT matched? The items can be radios or not. If radios, checking one item should automatically uncheck all others.
But we do not uncheck further items once a match is found. So I do not understand why this magically works. But the original code was the same.

Comment 70

[Jorg K (CEST = GMT+2)]

I think it always uses radio-buttons
https://dxr.mozilla.org/comm-central/source/editor/ui/composer/content/editor.js#903
and we rely on that for unchecking.

Now the old code was:
get editor font
for each menu item:
  if menu font == editor font:
    set check mark, automatically clears all others
    break: // we achieved the desired state, one set, others cleared.
  clear this check mark // since we know this doesn't match
                        // and we don't know yet, whether it will
                        // be cleared by setting another one later.
end for.

So far, so good, right?

Now the new code is:
get editor font
for each menu item:
  if menu font == editor font:
    set check mark, automatically clears all others
    break: // we achieved the desired state, one set, others cleared.
  else if we have a better match on a font option:
    set check mark, automatically clears all others
    remember that we checked the item // matchFound = true
    break if we found the best match at the first position. // desired state achieved.
    // we need to keep looping to find a better match.
  if we didn't set the check mark: // since there wasn't an option match
    clear this check mark // reason as above in old code.
end for.

There is no magic but agreed, it's tricky.

Comment 71

[Jorg K (CEST = GMT+2)]

Attachment: Test cases for font indicator (v4)

I hope I have all the test cases we need now. I'm using "Batang" since I have that installed and it's towards the beginning of the list. Saves me scrolling down to "Verdana".

Comment 72

[:aceman]

Attachment: patch for font selector, v2

So I think I understood most of the specification :) This is what I whipped up. 
I have tested it on Jorg's attached test message.
Please check if I got what you wanted.
I've left some debugging output in so you can see what it does.

Comment 73

[Jorg K (CEST = GMT+2)]

Comment on attachment 8744622 [details] [diff] [review]
patch for font selector, v2

Review of attachment 8744622 [details] [diff] [review]:
-----------------------------------------------------------------

As discussed on IRC, doesn't work for some test cases.

::: editor/ui/composer/content/editor.js
@@ +689,5 @@
> +
> +  let menuPopup = fontFaceMenuList.menupopup;
> +  let menuItems = menuPopup.childNodes;
> +
> +  const genericFamilies = [ "cursive", "fantasy", "monospace", "sans-serif", "serif" ];

I'd remove cursive and fantasy as we don't support them.

@@ +696,5 @@
> +  // "Helvetica, Arial, sans-serif" are always recognised correctly
> +  let fontToLower = editorFont.toLowerCase().replace(/, /g, ",");
> +  let foundFont = null;
> +  let exactMatch = false;
> +  let notUsedFont = false;

Please explain this variable. Does that mean it's not in the list of used fonts?

@@ +706,5 @@
> +  for (let i = 0; i < menuItems.length; i++)
> +  {
> +    let menuItem = menuItems.item(i);
> +    if (menuItem.hasAttribute("label") && menuItem.hasAttribute("value_parsed"))
> +    { // The element seems to represent a font <menuitem>.

Nit. Shouldn't the // go on the next line?

@@ +717,5 @@
> +        exactMatch = true;
> +Components.utils.reportError("exact!"+fontMenuValue);
> +        break;
> +      }
> +      else if (optionsCount > 1 && notUsedFont)

why notUsedFont here?

@@ +735,4 @@
>        }
>      }
> +    else
> +    { // Some other element type.

Same here.

Comment 74

[Jorg K (CEST = GMT+2)]

Attachment: Test cases for font indicator (v5)

Now Arial,Calibi,Consolas instead of
Arial,Batang,Calibri. Maintains the alphabetical order of the test cases.

Comment 75

[:aceman]

(In reply to Jorg K (GMT+2) from comment #73)
> > +  const genericFamilies = [ "cursive", "fantasy", "monospace", "sans-serif", "serif" ];
> 
> I'd remove cursive and fantasy as we don't support them.
We can't create them, but why drop them if we can preserve them using the proposed general algorithm. Why should we preserve only some generics and not others.

> @@ +696,5 @@
> > +  let notUsedFont = false;
> Please explain this variable. Does that mean it's not in the list of used
> fonts?
Yes. Added comment.

> @@ +717,5 @@
> > +        exactMatch = true;
> > +Components.utils.reportError("exact!"+fontMenuValue);
> > +        break;
> > +      }
> > +      else if (optionsCount > 1 && notUsedFont)
> 
> why notUsedFont here?
When we are matching the individual fonts in the conglomerate, I'd rather compare them to the individual fonts in the system, not to the font strings (some of them conglomerates) in the used-fonts list.

Comment 76

[:aceman]

Attachment: patch for font selector, v2.1

Sorry, this one should be better.

Comment 77

[:aceman]

Attachment: patch for font selector, v2.2

Comment 78

[Jorg K (CEST = GMT+2)]

Comment on attachment 8744643 [details] [diff] [review]
patch for font selector, v2.2

Review of attachment 8744643 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/ui/composer/content/editor.js
@@ +696,5 @@
> +  // "Helvetica, Arial, sans-serif" are always recognised correctly
> +  let fontToLower = editorFont.toLowerCase().replace(/, /g, ",");
> +  let foundFont = null;
> +  let exactMatch = false;
> +  let notUsedFont = false;

Please explain the variable here, comment below comes to late.
Maybe call it: fontInUsedSection.

@@ +702,5 @@
> +  let editorFontOptions = fontToLower.split(",");
> +  let optionsCount = editorFontOptions.length;
> +  let matchedFontIndex = optionsCount;
> +
> +  for (let i = 0; i < menuItems.length; i++)

i = 0, 1, 2, ... 7 are also not in the "in used" section, since the standard fonts precede that section.

@@ +711,5 @@
> +      // The element seems to represent a font <menuitem>.
> +      let fontMenuValue = menuItem.getAttribute("value_parsed");
> +      if (fontMenuValue == fontToLower ||
> +          (menuItem.hasAttribute("value_cache") &&
> +           menuItem.getAttribute("value_cache").split("|").includes(fontToLower)))

Please explain the structure of the cache.

@@ +741,5 @@
> +    {
> +      // Some other element type.
> +      if (menuItem == usedFontsSep)
> +      {
> +        // We have now passed the block of used fonts and are now in the list of all.

Comment nice, but too late.

Comment 79

[Jorg K (CEST = GMT+2)]

Attachment: patch for font selector, v2.3 (commented by JK)

Excellent work!! Thanks a lot.

Comment 80

[Jorg K (CEST = GMT+2)]

Attachment: patch for font selector, v2.3 (commented by JK)

(grr, trailing space).

Comment 81

[Jorg K (CEST = GMT+2)]

Comment on attachment 8744651 [details] [diff] [review]
patch for font selector, v2.3 (commented by JK)

Review of attachment 8744651 [details] [diff] [review]:
-----------------------------------------------------------------

I think I understand it all. Since you're not answering on IRC, let me ask a minor thing here.

Also, of course, please revise my comments.

After that, can we please land both parts and be done here?

::: editor/ui/composer/content/editor.js
@@ +793,5 @@
> +
> +      // Check if such an item is already in the used font section.
> +      if (afterUsedFontSection)
> +        foundFont = menuPopup.querySelector('menuitem[used="true"][value_parsed="'+
> +                    editorFont.toLowerCase()+'"]');

Why do you use editorFont.toLowerCase() and not editorFontToLower? There should be almost the same.

Comment 82

[Jorg K (CEST = GMT+2)]

Comment on attachment 8744651 [details] [diff] [review]
patch for font selector, v2.3 (commented by JK)

Review of attachment 8744651 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/ui/composer/content/editor.js
@@ +788,5 @@
> +    {
> +      // Keep only the found font and generic families in the font string.
> +      editorFont = editorFont.replace(/, /g, ",").split(",").filter(
> +        font => ((font.toLowerCase() == foundFont.getAttribute("value_parsed")) ||
> +                 genericFamilies.includes(font))).join(",");

And can you explain what this does?

Comment 83

[:aceman]

(In reply to Jorg K (GMT+2) from comment #81)
> ::: editor/ui/composer/content/editor.js
> @@ +793,5 @@
> > +
> > +      // Check if such an item is already in the used font section.
> > +      if (afterUsedFontSection)
> > +        foundFont = menuPopup.querySelector('menuitem[used="true"][value_parsed="'+
> > +                    editorFont.toLowerCase()+'"]');
> 
> Why do you use editorFont.toLowerCase() and not editorFontToLower? There
> should be almost the same.

No, editorFontToLower also contains the font options that we removed. editorFont does no contain them at this spot.

(In reply to Jorg K (GMT+2) from comment #82)
> ::: editor/ui/composer/content/editor.js
> @@ +788,5 @@
> > +    {
> > +      // Keep only the found font and generic families in the font string.
> > +      editorFont = editorFont.replace(/, /g, ",").split(",").filter(
> > +        font => ((font.toLowerCase() == foundFont.getAttribute("value_parsed")) ||
> > +                 genericFamilies.includes(font))).join(",");
> 
> And can you explain what this does?

As the comment says, removes all font options that are not the one we matches (the best one) and the generic family. In the JS code it splits it on the commas into an array, filters away members that do not match and then joins back into a string concatenated by commas.

Comment 84

[:aceman]

Comment on attachment 8744651 [details] [diff] [review]
patch for font selector, v2.3 (commented by JK)

Review of attachment 8744651 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for adding the comments.

::: editor/ui/composer/content/editor.js
@@ +773,5 @@
> +    if (exactMatch)
> +    {
> +      if (afterUsedFontSection)
> +      {
> +        // Copy it into the section of used fonts if it isn't there already.

It surely isn't there (in the used fonts), because we found an exact match after the used fonts (per the variable).

@@ +775,5 @@
> +      if (afterUsedFontSection)
> +      {
> +        // Copy it into the section of used fonts if it isn't there already.
> +        // We insert after the separator following the default fonts,
> +        // so right at the begigging of the used fonts section.

beginning.

Comment 85

[Jorg K (CEST = GMT+2)]

Attachment: Patch to fix font menu according to comment #54 (v5).

Minor tweaks to make it more consistent with Aceman's patch.

Comment 86

[:aceman]

Comment on attachment 8742762 [details]
serif,Arial (not installed).png

Moved to other bug.

Comment 87

[:aceman]

Comment on attachment 8742761 [details]
Test message.eml

Moved to other bug.

Comment 88

[:aceman]

Comment on attachment 8742720 [details]
Earlybird serif,[font name] (not installed).gif

Moved to other bug.

Comment 89

[:aceman]

Comment on attachment 8744653 [details] [diff] [review]
Patch to fix font menu according to comment #54 (v5).

Review of attachment 8744653 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/ui/composer/content/editor.js
@@ +976,5 @@
> +          // Next compare the individual options.
> +          else if (editorFontOptions.length > 1)
> +          {
> +            if (!editorFontOptions)
> +              editorFontOptions = editorFont.split(',');

You can now remove these 2 lines.

Comment 90

[Jorg K (CEST = GMT+2)]

Attachment: Patch to fix font menu according to comment #54 (v5a).

Removed left-overs from v5.

Comment 91

[:aceman]

Landed with the comments fixed:
https://hg.mozilla.org/comm-central/rev/bfecdd72d317ee293b455c88aad11f0a0a8cf6dd
https://hg.mozilla.org/comm-central/rev/ce11ffed34de38d6b0f06de5298033000d5a558e

Comment 92

[Jorg K (CEST = GMT+2)]

Minor comment tweak:
https://hg.mozilla.org/comm-central/rev/d6cc99efbe15

Comment 93

[Jorg K (CEST = GMT+2)]

Comment on attachment 8744654 [details] [diff] [review]
Patch to fix font menu according to comment #54 (v5a).

[Approval Request Comment]
Regression caused by (bug #): Bug 1148790
User impact if declined: Very ugly and confusing font selector.
Testing completed (on c-c, etc.): Manual testing, test cases attached to bug.
Risk to taking this patch (and alternatives if risky):
There's always risk, but we thoroughly tested this.

Comment 94

[Jorg K (CEST = GMT+2)]

Comment on attachment 8744651 [details] [diff] [review]
patch for font selector, v2.3 (commented by JK)

Approval comment: See preceding comment.

Comment 95

[Jorg K (CEST = GMT+2)]

Aurora (TB 47):
https://hg.mozilla.org/releases/comm-aurora/rev/0d5dcdccea0b
https://hg.mozilla.org/releases/comm-aurora/rev/8d340a618eaf (also contains comment fix from 3rd changeset)

Dear beta/ESR lander: I merged the comment fix from comment #92 into the second changeset so you have less work. Please uplift the *two* changesets listed above.

Comment 96

[Kent James (:rkent)]

Comment on attachment 8744654 [details] [diff] [review]
Patch to fix font menu according to comment #54 (v5a).

http://hg.mozilla.org/releases/comm-esr45/rev/a17db1a5d114
http://hg.mozilla.org/releases/comm-esr45/rev/c7e1e46b105f