The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Thunderbird 48.0

Status

Thunderbird
Message Compose Window
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: Jorg K (GMT+1), Assigned: aceman)

Tracking

Trunk
Thunderbird 48.0

Thunderbird Tracking Flags

(thunderbird46 wontfix, thunderbird47 fixed, thunderbird48 fixed, thunderbird_esr4546+ fixed)

Details

Attachments

(3 attachments, 17 obsolete attachments)

3.03 KB, text/plain
Details
9.49 KB, patch
Jorg K (GMT+1)
: review+
aceman
: review+
Jorg K (GMT+1)
: approval-comm-aurora+
Jorg K (GMT+1)
: approval-comm-beta-
Jorg K (GMT+1)
: approval-comm-esr45+
Details | Diff | Splinter Review
3.43 KB, patch
Jorg K (GMT+1)
: review+
Jorg K (GMT+1)
: approval-comm-aurora+
Jorg K (GMT+1)
: approval-comm-beta-
Details | Diff | Splinter Review
(Reporter)

Description

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

Also read bug 1148790 comment #45 and further down.
(Reporter)

Comment 1

11 months ago
Correction: Watch this https://www.youtube.com/watch?v=aceySKlkqo4 from 1:45.
(Reporter)

Comment 2

11 months ago
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/,"");
(Reporter)

Comment 3

11 months ago
(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 ;-)
(Assignee)

Comment 4

11 months ago
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.
(Assignee)

Comment 5

11 months ago
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".

Updated

11 months ago
Duplicate of this bug: 1265198
Comment hidden (off-topic)
Comment hidden (off-topic)

Comment 9

11 months ago
bug 1265198 is a dup of this bug. What is the correct regression bug please?
Comment hidden (off-topic)
(Assignee)

Comment 11

11 months ago
(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.
Blocks: 1148790
OS: Unspecified → All
Hardware: Unspecified → All
Comment hidden (off-topic)
Comment hidden (off-topic)
Comment hidden (off-topic)
Comment hidden (off-topic)
Comment hidden (off-topic)
Comment hidden (off-topic)
Comment hidden (off-topic)
Comment hidden (off-topic)
Comment hidden (off-topic)
Comment hidden (off-topic)
Comment hidden (off-topic)
(Reporter)

Updated

11 months ago
Attachment #8742761 - Attachment mime type: message/rfc822 → text/plain
Comment hidden (off-topic)
Comment hidden (off-topic)
(Assignee)

Comment 25

11 months ago
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.
Assignee: nobody → acelists
Comment hidden (off-topic)
(Reporter)

Comment 27

11 months ago
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.
(Assignee)

Comment 28

11 months ago
(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.
(Assignee)

Comment 29

11 months ago
Created attachment 8743010 [details] [diff] [review]
patch

So this is something to play with.
Attachment #8743010 - Flags: feedback?(mozilla)
(Reporter)

Comment 30

11 months ago
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.
Attachment #8743010 - Flags: feedback?(mozilla) → feedback-
(Assignee)

Comment 31

11 months ago
(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.
(Reporter)

Comment 32

11 months ago
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.
(Reporter)

Comment 33

11 months ago
(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.
(Reporter)

Comment 34

11 months ago
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.
Flags: needinfo?(axelg)
(Assignee)

Comment 35

11 months ago
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.
(Reporter)

Comment 36

11 months ago
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?
status-thunderbird46: --- → affected
status-thunderbird47: --- → affected
status-thunderbird48: --- → affected
status-thunderbird_esr45: --- → affected
tracking-thunderbird_esr45: --- → +
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.
(Reporter)

Comment 38

11 months ago
(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.
(Reporter)

Comment 39

11 months ago
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 hidden (off-topic)
Comment hidden (off-topic)
(Reporter)

Comment 42

11 months ago
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).
Flags: needinfo?(axelg) → needinfo?(acelists)
(Assignee)

Comment 43

11 months ago
(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?
Flags: needinfo?(acelists)
(Reporter)

Comment 44

11 months ago
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 ;-)
(Assignee)

Comment 45

11 months ago
(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.
Flags: needinfo?(axelg)
(Reporter)

Comment 46

11 months ago
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.
(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.
Flags: needinfo?(axelg)
(Reporter)

Comment 48

11 months ago
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.
(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. :)
(Reporter)

Comment 50

11 months ago
(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?
(Reporter)

Comment 51

11 months ago
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.
(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)
(Reporter)

Comment 53

11 months ago
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.
(Reporter)

Comment 54

11 months ago
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?
(Reporter)

Comment 55

11 months ago
Created attachment 8744199 [details]
Test cases for font indicator
(Reporter)

Comment 56

11 months ago
Created attachment 8744204 [details]
Test cases for font indicator (v2)
Attachment #8744199 - Attachment is obsolete: true
(Reporter)

Updated

11 months ago
Attachment #8744204 - Attachment mime type: message/rfc822 → text/plain
(Reporter)

Comment 57

11 months ago
Created attachment 8744206 [details] [diff] [review]
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.
Attachment #8744206 - Flags: review?(acelists)
(Assignee)

Comment 58

11 months ago
Yes, I'll look at this today.
(Reporter)

Comment 59

11 months ago
Created attachment 8744290 [details] [diff] [review]
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.
Attachment #8744206 - Attachment is obsolete: true
Attachment #8744206 - Flags: review?(acelists)
Attachment #8744290 - Flags: review?(acelists)
(Assignee)

Comment 60

11 months ago
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.
(Reporter)

Comment 61

11 months ago
(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.
(Reporter)

Comment 62

11 months ago
Created attachment 8744481 [details]
Test cases for font indicator (v3)
Attachment #8744204 - Attachment is obsolete: true
(Assignee)

Comment 63

11 months ago
(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.
(Reporter)

Comment 64

11 months ago
(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.
(Reporter)

Comment 65

11 months ago
Created attachment 8744489 [details] [diff] [review]
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.
Attachment #8744290 - Attachment is obsolete: true
Attachment #8744290 - Flags: review?(acelists)
Attachment #8744489 - Flags: review?(acelists)
(Assignee)

Comment 66

11 months ago
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.
(Assignee)

Comment 67

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

Or maybe 0 :)
(Reporter)

Comment 68

11 months ago
Created attachment 8744515 [details] [diff] [review]
Patch to fix font menu according to comment #54 (v4).

Incorporated Aceman's suggestions. Also fixed another logic error in clearing the check mark.
Attachment #8744489 - Attachment is obsolete: true
Attachment #8744515 - Flags: review?(acelists)
Attachment #8744489 - Flags: review?(acelists)
(Assignee)

Comment 69

11 months ago
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.
Attachment #8744515 - Flags: review?(acelists) → review+
(Reporter)

Comment 70

11 months ago
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.
(Reporter)

Comment 71

11 months ago
Created attachment 8744560 [details]
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".
Attachment #8744481 - Attachment is obsolete: true
(Assignee)

Comment 72

11 months ago
Created attachment 8744622 [details] [diff] [review]
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.
Attachment #8743010 - Attachment is obsolete: true
Attachment #8744622 - Flags: feedback?(richard.marti)
Attachment #8744622 - Flags: feedback?(mozilla)
(Reporter)

Comment 73

11 months ago
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.
(Reporter)

Comment 74

11 months ago
Created attachment 8744631 [details]
Test cases for font indicator (v5)

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

Comment 75

11 months ago
(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.
Status: NEW → ASSIGNED
(Assignee)

Comment 76

11 months ago
Created attachment 8744636 [details] [diff] [review]
patch for font selector, v2.1

Sorry, this one should be better.
Attachment #8744622 - Attachment is obsolete: true
Attachment #8744622 - Flags: feedback?(richard.marti)
Attachment #8744622 - Flags: feedback?(mozilla)
Attachment #8744636 - Flags: ui-review?(richard.marti)
Attachment #8744636 - Flags: feedback?(mozilla)
(Assignee)

Comment 77

11 months ago
Created attachment 8744643 [details] [diff] [review]
patch for font selector, v2.2
Attachment #8744636 - Attachment is obsolete: true
Attachment #8744636 - Flags: ui-review?(richard.marti)
Attachment #8744636 - Flags: feedback?(mozilla)
Attachment #8744643 - Flags: feedback?(mozilla)
(Reporter)

Comment 78

11 months ago
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.
(Reporter)

Comment 79

11 months ago
Created attachment 8744650 [details] [diff] [review]
patch for font selector, v2.3 (commented by JK)

Excellent work!! Thanks a lot.
Attachment #8744643 - Attachment is obsolete: true
Attachment #8744643 - Flags: feedback?(mozilla)
Attachment #8744650 - Flags: review+
(Reporter)

Comment 80

11 months ago
Created attachment 8744651 [details] [diff] [review]
patch for font selector, v2.3 (commented by JK)

(grr, trailing space).
Attachment #8744650 - Attachment is obsolete: true
Attachment #8744651 - Flags: review+
(Reporter)

Comment 81

11 months ago
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.
(Reporter)

Comment 82

11 months ago
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?
(Assignee)

Comment 83

11 months ago
(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.
(Assignee)

Comment 84

11 months ago
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.
Attachment #8744651 - Flags: review+
(Reporter)

Comment 85

11 months ago
Created attachment 8744653 [details] [diff] [review]
Patch to fix font menu according to comment #54 (v5).

Minor tweaks to make it more consistent with Aceman's patch.
Attachment #8744515 - Attachment is obsolete: true
Attachment #8744653 - Flags: review?(acelists)
(Assignee)

Updated

11 months ago
Attachment #8744560 - Attachment is obsolete: true
(Assignee)

Comment 86

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

Moved to other bug.
Attachment #8742762 - Attachment is obsolete: true
(Assignee)

Comment 87

11 months ago
Comment on attachment 8742761 [details]
Test message.eml

Moved to other bug.
Attachment #8742761 - Attachment is obsolete: true
(Assignee)

Comment 88

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

Moved to other bug.
Attachment #8742720 - Attachment is obsolete: true
(Assignee)

Comment 89

11 months ago
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.
Attachment #8744653 - Flags: review?(acelists) → review+
(Reporter)

Comment 90

11 months ago
Created attachment 8744654 [details] [diff] [review]
Patch to fix font menu according to comment #54 (v5a).

Removed left-overs from v5.
Attachment #8744653 - Attachment is obsolete: true
Attachment #8744654 - Flags: review+
(Assignee)

Comment 91

11 months ago
Landed with the comments fixed:
https://hg.mozilla.org/comm-central/rev/bfecdd72d317ee293b455c88aad11f0a0a8cf6dd
https://hg.mozilla.org/comm-central/rev/ce11ffed34de38d6b0f06de5298033000d5a558e
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 48.0
(Reporter)

Comment 92

11 months ago
Minor comment tweak:
https://hg.mozilla.org/comm-central/rev/d6cc99efbe15
status-thunderbird48: affected → fixed
(Reporter)

Comment 93

11 months ago
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.
Attachment #8744654 - Flags: approval-comm-esr45?
Attachment #8744654 - Flags: approval-comm-beta?
Attachment #8744654 - Flags: approval-comm-aurora+
(Reporter)

Comment 94

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

Approval comment: See preceding comment.
Attachment #8744651 - Flags: approval-comm-esr45?
Attachment #8744651 - Flags: approval-comm-beta?
Attachment #8744651 - Flags: approval-comm-aurora+
(Reporter)

Comment 95

11 months ago
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.
status-thunderbird47: affected → fixed

Comment 96

11 months ago
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
Attachment #8744654 - Flags: approval-comm-esr45? → approval-comm-esr45+

Updated

11 months ago
status-thunderbird_esr45: affected → fixed
tracking-thunderbird_esr45: + → 46+
(Reporter)

Updated

11 months ago
Attachment #8744651 - Flags: approval-comm-beta? → approval-comm-beta-
(Reporter)

Updated

11 months ago
Attachment #8744654 - Flags: approval-comm-beta? → approval-comm-beta-
(Reporter)

Updated

11 months ago
Attachment #8744651 - Flags: approval-comm-esr45? → approval-comm-esr45+
(Reporter)

Updated

11 months ago
status-thunderbird46: affected → wontfix
You need to log in before you can comment on or make changes to this bug.