Font indicator doesn't update when cursor is placed into text with unsupported font in message composer

RESOLVED FIXED in Thunderbird 43.0

Status

Thunderbird
Message Compose Window
RESOLVED FIXED
2 years ago
2 months ago

People

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

Tracking

Trunk
Thunderbird 43.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

2 years ago
This is a follow-up bug to bug 1139524 and bug 1148330.

When while composing a message the user clicks on text with a font not recognised by the current system, the font indicator doesn't update. 

This can happen when a message is replied to which came from a system which supports other fonts.
(Assignee)

Updated

2 years ago
OS: Windows 7 → All
Hardware: x86_64 → All
Summary: Font indicator doesn't update when cursor is place into text with unsupported font → Font indicator doesn't update when cursor is placed into text with unsupported font in message composer
(Assignee)

Comment 1

2 years ago
So if the user clicks into a range with an unknown font name, the editor core returns this font name and the UI tries to find it in the menulist of available fonts so it can be selected.
My proposal would be to just add such a font to the menulist somewhere at the bottom, with a remark that it is e.g. "not installed". We can still allow the user to compose messages with such a font name. It will not render in any visible way, but the message should be valid. The user would be able to keep the font fidelity in the message.
If we dumped all unknown fonts into a single "unknown" entry in the menulist, this would prevent the user from knowing what fonts there are specified and to use them.
Assignee: nobody → acelists
(Reporter)

Comment 2

2 years ago
(In reply to :aceman from comment #1)
> My proposal would be to just add such a font to the menulist somewhere at
> the bottom, with a remark that it is e.g. "not installed". We can still
> allow the user to compose messages with such a font name. 

I agree in principle. However, putting the font at the bottom will make it hard to select since most people have very long font menus. I'd put it in 3rd position, so it's obvious:

Variable Width
Fixed Width
----
Helvetica, Arial
Times
Courier
----
xyz (not installed)
1st-installed font
(Assignee)

Comment 3

2 years ago
I'd think a position at the end would discourage the use of the font, which may be the right thing to do :)
Flags: needinfo?(bwinton)
Yeah, I think at the end would be better, for the reason Aceman gave.
Flags: needinfo?(bwinton)
(Assignee)

Comment 5

2 years ago
On the other hand, if the font is already used in the message, it may be the user wants to actually use it again so why prioritize a ton of unneeded fonts before it (just because they are on the system).
(Assignee)

Comment 6

2 years ago
Created attachment 8588493 [details] [diff] [review]
WIP patch

So this patch does these 2 features:
1. It creates a "block" in the font face menulist of compose window. It puts there any fonts it encounters in the composed message. "Encounter" means that the user clicks in a range with that font. I call this block "used fonts". So it basically creates a cache a quick list of fonts that the user used in the message and may want to also use in the future. He doesn't need to hunt them in the long list of fonts.
2. If there are any unknown fonts encountered they are also added into this "used fonts" list, but at the bottom of it. They are labeled as "not installed", but the user can still use them for composing (he will just not see their rendering). This may happen if the user is replying to a message with fonts that are not on his system. You may have to edit the message source in Drafts folder to create such a special message.

How would you like this?
Attachment #8588493 - Flags: feedback?(mozilla)
Attachment #8588493 - Flags: feedback?(bwinton)
Attachment #8588493 - Flags: feedback?(axelg)
(Reporter)

Comment 7

2 years ago
I will try it today and let you know.
I'm currently more worried about TB 38. Is this intended to be landed for TB 38? And more importantly, how does this relate to bug 1148330 and attachment 8585223 [details] [diff] [review]. There we reinterpret the suppress the sans-serif, serif and monospace.
My suggestion would be to land bug 1148330 now and land this bug later. Also, from Gecko 40, the core will return sans-serif, serif and in theory monospace (although I've never seen it) since bug 1141017 got landed on mozilla40.
(In reply to Jorg K (at the beach until 22nd April) from comment #2)
> (In reply to :aceman from comment #1)
> > My proposal would be to just add such a font to the menulist somewhere at
> > the bottom, with a remark that it is e.g. "not installed". We can still
> > allow the user to compose messages with such a font name. 
> 
> I agree in principle. However, putting the font at the bottom will make it
> hard to select since most people have very long font menus. I'd put it in
> 3rd position, so it's obvious:
> 
> Variable Width
> Fixed Width
> ----
> Helvetica, Arial
> Times
> Courier
> ----
> xyz (not installed)
> 1st-installed font

I think this makes the most sense. 

General feedback on the patch: I think it is a good idea but you have to guard against pulling fonts from the blockquote if I click there. Same goes for "paste as quotation" which I use quite a lot. I would like to use my own fonts when I write, so when Tb oftentimes continues my replies in the font of the addressee I find this a big distraction. Same goes for font sizes. 

If I had my "standard-paragraph" font and a new <p> inserted whenever I hit Return, I would be a happy camper. <font> is such a distraction.
(Reporter)

Comment 9

2 years ago
Created attachment 8588530 [details]
Great! Showing the "used fonts" section.

Great! It's not hard to get an "unknown font", since "sans-serif" is returned from the editor (if not reinterpreted - bug 1148330).

For testing, I have "Batang" as a default composition font.
"Batang" and "sans-serif" show up the in the "used fonts" section.
(Reporter)

Updated

2 years ago
Attachment #8588493 - Flags: feedback?(mozilla) → feedback+
(Assignee)

Comment 10

2 years ago
This is not intended for TB38. Cleaning up the sans-serif handling should be done in the other bugs independently. And this bug will work better after them (on linux it now pulls up the default "sans-serif" into the "used fonts" list).
This should only pull fonts into the "used fonts" group if they are in the list of all fonts. It does not act if you use (encounter) fonts in the first 5 predefined items (variable width -> courier).
(Assignee)

Comment 11

2 years ago
(In reply to Axel Grude [:realRaven] from comment #8)
> General feedback on the patch: I think it is a good idea but you have to
> guard against pulling fonts from the blockquote if I click there. Same goes
> for "paste as quotation" which I use quite a lot. I would like to use my own
> fonts when I write, so when Tb oftentimes continues my replies in the font
> of the addressee I find this a big distraction. Same goes for font sizes. 
Actually the point of this bug is to pull fonts used by the sender in the message, that you are now replying to. Yes, if you click in his text you get the font added into the list (it may be the "not installed" case). So it enables you to use his font IF you want. But you do not need to. Yes, it may clutter the font picker. But that is why there is the "used fonts" block now, where you also get YOUR fonts listed and YOUR fonts get before the "not installed" ones. But yeah, if the sender has fonts that are also installed on your machine, then they mix with yours. We could put sender's fonts (from the reply or quote) again at the bottom of the "used fonts" group. However I am not sure how to detect the text is a quote (onFontFaceChange function does not have such info). And it is a feature specific for message composition, while this patch is done in the generic editor code.

> If I had my "standard-paragraph" font and a new <p> inserted whenever I hit
> Return, I would be a happy camper. <font> is such a distraction.
Yeah, predefined styles (font faces) for paragraphs or headings would be the next level of this feature, for new bug :)
(Reporter)

Comment 12

2 years ago
(In reply to :aceman from comment #10)
> Cleaning up the sans-serif handling should be
> done in the other bugs independently. And this bug will work better after
> them (on linux it now pulls up the default "sans-serif" into the "used
> fonts" list).

On Windows I get sans-serif as well, see attachment 8588530 [details], remember, it's returned from the core editor. How would we distinguish between "sans-serif", the CSS font family as returned by the core editor on all platforms, and "sans-serif", the Linux font?

If bug 1148330 goes ahead, "sans-serif", etc. will be reinterpreted/suppressed. I thought your aim was to allow a message with generic "sans-serif" to be sent. Or did you change your mind given Neil's comment in bug 1139524, comment #84? I think we should land bug 1148330 now, perhaps without the suppression (bug 1148330 comment #22), and you can present a more complete solution in this bug.

> This should only pull fonts into the "used fonts" group if they are in the
> list of all fonts.

I don't understand this. I thought the aim is to identify "not installed" fonts as "used fonts" which are clearly *not* in the list of all installed fonts.
(Assignee)

Comment 13

2 years ago
(In reply to Jorg K (at the beach until 22nd April) from comment #12)
> On Windows I get sans-serif as well, see attachment 8588530 [details],
> remember, it's returned from the core editor. How would we distinguish
> between "sans-serif", the CSS font family as returned by the core editor on
> all platforms, and "sans-serif", the Linux font?
> 
> If bug 1148330 goes ahead, "sans-serif", etc. will be
> reinterpreted/suppressed. I thought your aim was to allow a message with
> generic "sans-serif" to be sent. Or did you change your mind given Neil's
> comment in bug 1139524, comment #84?
Yes, Neil said it is hard to distinguish the generic "serif" family and a system font by the "serif" name. So that problem would need some more thought and design in a separate bug.
I only had the idea of being able to use "serif" (and the rest) family as a font on all platforms (put it in the message as <font face="serif">).
This is currently not implemented precisely. There are only the "Helvetica, Arial, sans-serif" items which partly want to implement this (but you get Helvetica forced if it is on your system, regardless if you overrode sans-serif in TB to some other font). So I am fine if this stays as it is.
So I am also fine with whatever you do in bug 1148330. Hiding them for Linux to get platform parity is fine for me.

> I don't understand this. I thought the aim is to identify "not installed"
> fonts as "used fonts" which are clearly *not* in the list of all installed
> fonts.
The "used fonts" will also contain installed fonts. It will collect all fonts used in the message. The idea is to group the several fonts you may want to use in the message at hand and quickly switching between them. Instead of hunting them in the all fonts list each time.
(Reporter)

Comment 14

2 years ago
(In reply to :aceman from comment #13)

> So I am also fine with whatever you do in bug 1148330. Hiding them for Linux
> to get platform parity is fine for me.
My aim is to squash the regression we're currently seeing *before* the final version. After that I'm open to a good comprehensive solution.

> The "used fonts" will also contain installed fonts. It will collect all
> fonts used in the message. The idea is to group the several fonts you may
> want to use in the message at hand and quickly switching between them.
> Instead of hunting them in the all fonts list each time.
Yes, indeed. What confused me before is what you said in comment #10:
> This should only pull fonts into the "used fonts" group *if they are in the list of all fonts*.
I repeat: "not installed" fonts (which are not in the list) will also go into "used fonts" shortlist, right? I tried it, I manually edited a draft message to contain an unknown font an this - as expected - went into the shortlist as "not installed".
(Assignee)

Comment 15

2 years ago
(In reply to Jorg K (at the beach until 22nd April) from comment #14)
> > The "used fonts" will also contain installed fonts. It will collect all
> > fonts used in the message. The idea is to group the several fonts you may
> > want to use in the message at hand and quickly switching between them.
> > Instead of hunting them in the all fonts list each time.
> Yes, indeed. What confused me before is what you said in comment #10:
> > This should only pull fonts into the "used fonts" group *if they are in the list of all fonts*.
> I repeat: "not installed" fonts (which are not in the list) will also go
> into "used fonts" shortlist, right? I tried it, I manually edited a draft
> message to contain an unknown font an this - as expected - went into the
> shortlist as "not installed".
Yeah, sorry, I forgot not installed fonts in that sentence:) I just wanted to distinguish fonts that are above the "used fonts" group (the 5 fixed entries) and the "all system fonts" group that are below. Not installed fonts are in no group but if encountered they go into the end of the used fonts group as that is the only one I change dynamically in the patch.
Comment on attachment 8588493 [details] [diff] [review]
WIP patch

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

Obviously, remove all the "dump" statements, but other than that, and the stuff below, it looks pretty good to me.  ;)

::: editor/ui/composer/content/editor.js
@@ +487,2 @@
>  {
>    try {  

Nit: trailing spaces here…  ;)

@@ +713,5 @@
>      {
> +      let menuItem = menuItems.item(i);
> +      if (menuItem.hasAttribute("label") && ("value" in menuItem)) {
> +        // The element seems to represent a font <menuitem>.
> +        if (menuItem.value.toLowerCase().replace(/, /g, ",") == stateToLower) {

So, this seems kind of expensive to do for every menu element, every time the font-face changes.  Could we store the results of the substitution on the menuItems, so that we don't have to re-calculate it, perhaps in createFontFaceMenuitem?

@@ +894,5 @@
>      }
>    }
>  }
>  
> +const kFixedFontFaceMenuItems = 8; // number of fixed font face menuitems

Why did this change to 8?
Attachment #8588493 - Flags: feedback?(bwinton) → feedback+
(In reply to :aceman from comment #11)
> (In reply to Axel Grude [:realRaven] from comment #8)
> > General feedback on the patch: I think it is a good idea but you have to
> > guard against pulling fonts from the blockquote if I click there. Same goes
> > for "paste as quotation" which I use quite a lot. I would like to use my own
> > fonts when I write, so when Tb oftentimes continues my replies in the font
> > of the addressee I find this a big distraction. Same goes for font sizes. 
> Actually the point of this bug is to pull fonts used by the sender in the
> message, that you are now replying to. Yes, if you click in his text you get
> the font added into the list (it may be the "not installed" case). So it
> enables you to use his font IF you want. But you do not need to. Yes, it may
> clutter the font picker.
that does NOT make sense. Current behavior is when I PICK a font MYSELF (let's say Cambria) to highlight my text that I have written then I would expect THAT font to be in the list. not Some other persons font (unless I used Paste to insert it into the BODY level). Whatever is blockquoted should remain blockquoted and in its own font. 

Currently if I highlight some text and say it should be in Consolas then I do not get that option again unless I pick it from the long picker again. And I think that this is the main use case. So why not only use "other peoples font" when you do a straight paste of the text but not when it is quoted? I think the dismbiguation between reply and quote is one of the strong features of Thunderbird (and always confuses me when I am using OUtlook as everything seems to "muddle up")

> But that is why there is the "used fonts" block
> now, where you also get YOUR fonts listed and YOUR fonts get before the "not
> installed" ones. But yeah, if the sender has fonts that are also installed
> on your machine, then they mix with yours. We could put sender's fonts (from
> the reply or quote) again at the bottom of the "used fonts" group. 

LIke I said, only add if you insert (lets say cut and paste) but not when you "Paste as quotation". And OF COURSE add the font when we pick it once from the long list ourselves. That's my personal view on it. 

> However I
> am not sure how to detect the text is a quote (onFontFaceChange function
> does not have such info). And it is a feature specific for message
> composition, while this patch is done in the generic editor code.

Oh I see how that could lead to complications... hmm well is that event even triggered when you click into the text or only when you start typing / changing?


> 
> > If I had my "standard-paragraph" font and a new <p> inserted whenever I hit
> > Return, I would be a happy camper. <font> is such a distraction.
> Yeah, predefined styles (font faces) for paragraphs or headings would be the
> next level of this feature, for new bug :)

Definitely. If you plan anything like this get in touch I want to help if there is UI to be made for it.
(Assignee)

Comment 18

2 years ago
(In reply to Axel Grude [:realRaven] from comment #17)
> that does NOT make sense. Current behavior is when I PICK a font MYSELF
> (let's say Cambria) to highlight my text that I have written then I would
> expect THAT font to be in the list. not Some other persons font (unless I
> used Paste to insert it into the BODY level). Whatever is blockquoted should
> remain blockquoted and in its own font. 
Yes, also your fonts are put in the "used fonts" list. Simply all fonts used in the message, whether yours or put the by the sender (but only when you click on a range containing the font, or use the font from the picker). Have you actually tried the patch?

> > But that is why there is the "used fonts" block
> > now, where you also get YOUR fonts listed and YOUR fonts get before the "not
> > installed" ones. But yeah, if the sender has fonts that are also installed
> > on your machine, then they mix with yours. We could put sender's fonts (from
> > the reply or quote) again at the bottom of the "used fonts" group. 
> 
> LIke I said, only add if you insert (lets say cut and paste) but not when
> you "Paste as quotation". And OF COURSE add the font when we pick it once
> from the long list ourselves. That's my personal view on it. 
The fonts are not put into the "used fonts" block just by replying to a message.
Only when you select the text being replied to and it contains some font.

> Oh I see how that could lead to complications... hmm well is that event even
> triggered when you click into the text or only when you start typing /
> changing?
Just clicking into a span of text contained in a <font face> triggers the event and shows the used font in the font picker menulist. That is when this patch kicks in and adds even unknown fonts to the list so that they can be displayed in the picker.
(Assignee)

Comment 19

2 years ago
(In reply to Blake Winton (:bwinton) from comment #16)
> ::: editor/ui/composer/content/editor.js
> @@ +487,2 @@
> >  {
> >    try {  
> 
> Nit: trailing spaces here…  ;)
Yeah, but it is far from code I touch :)

> @@ +713,5 @@
> >      {
> > +      let menuItem = menuItems.item(i);
> > +      if (menuItem.hasAttribute("label") && ("value" in menuItem)) {
> > +        // The element seems to represent a font <menuitem>.
> > +        if (menuItem.value.toLowerCase().replace(/, /g, ",") == stateToLower) {
> 
> So, this seems kind of expensive to do for every menu element, every time
> the font-face changes.  Could we store the results of the substitution on
> the menuItems, so that we don't have to re-calculate it, perhaps in
> createFontFaceMenuitem?
This would belong in bug 1139524, but yes, that one was for TB38 so needed to be easy. I can fix this here in a more invasive change.

> > +const kFixedFontFaceMenuItems = 8; // number of fixed font face menuitems
> Why did this change to 8?
I added one new menuseparator, hidden by default.
Comment on attachment 8588493 [details] [diff] [review]
WIP patch

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

::: editor/ui/composer/content/editor.js
@@ +695,5 @@
>      fontFaceMenuList.selectedItem = null;
>      fontFaceMenuList.setAttribute("label",GetString('Mixed'));
>    }
>    else
>    {

I think (careful) var ==> let refactoring is positive (especially considering the deadzone bug) could you possibly do it consistently in the functions you touched?
Attachment #8588493 - Flags: feedback?(axelg) → feedback+
(Assignee)

Comment 21

2 years ago
(In reply to Axel Grude [:realRaven] from comment #20)
> I think (careful) var ==> let refactoring is positive (especially
> considering the deadzone bug) could you possibly do it consistently in the
> functions you touched?

Sure. Which occurrence do you feel to be inconsistent?
(In reply to :aceman from comment #21)
> (In reply to Axel Grude [:realRaven] from comment #20)
> > I think (careful) var ==> let refactoring is positive (especially
> > considering the deadzone bug) could you possibly do it consistently in the
> > functions you touched?
> 
> Sure. Which occurrence do you feel to be inconsistent?

IIRC, you replaced "var menuitem" with "let menuitem", which is fine. Why not replace all vars in the function, while you are there? Using var can lead to inconsistencies (especially if you forget you used it and create a let of same name in a contained block), so it would be best to (carefully) replace all vars in the function with "let". I have done this with my addons and can now be sure I am not affected by the deadzone bug (mixing var + let for same name will lead to a runtime error).
(Assignee)

Comment 23

2 years ago
I am not sure wholesale replacement would pass review :) I am not even sure the lets I have done will pass, in this ancient code. Sometimes we need to keep the preferred style of files/modules.
Status: NEW → ASSIGNED
(In reply to :aceman from comment #23)
> I am not sure wholesale replacement would pass review :) I am not even sure
> the lets I have done will pass, in this ancient code. Sometimes we need to
> keep the preferred style of files/modules.

I think it is okay if you do it within the functions you touch if there is are already significant insertions of code. You just have to do it "carefully". I wouldn't do it for the whole module.
(Reporter)

Comment 25

2 years ago
What's happening with this bug? I thought it was a neat idea that we should land some time soon ;-)
(Assignee)

Comment 26

2 years ago
Created attachment 8606682 [details] [diff] [review]
patch v1.1

Somebody unexpectedly bitroted me in that editor file ;)
Attachment #8588493 - Attachment is obsolete: true
Attachment #8606682 - Flags: ui-review?(bwinton)
Attachment #8606682 - Flags: review?(neil)
Comment on attachment 8606682 [details] [diff] [review]
patch v1.1

Why not add the extra fonts when we open the message (instead of waiting until we click in the text)?
Attachment #8606682 - Flags: ui-review?(bwinton) → ui-review+
(Assignee)

Comment 28

2 years ago
(In reply to Blake Winton (:bwinton) from comment #27)
> Why not add the extra fonts when we open the message (instead of waiting
> until we click in the text)?

Mainly that I do not know right now how to do that. Surely that can be added later if somebody implements it. But it may also have perf implications on complex documents so it may not be such a simple decision/implementation.
(Reporter)

Comment 29

2 years ago
Are we going to land this or are we leaving this to rot? If Neil can't review it, we need to find another reviewer.
Flags: needinfo?(acelists)
(Assignee)

Comment 30

2 years ago
Comment on attachment 8606682 [details] [diff] [review]
patch v1.1

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

I am not sure who a better reviewer would be for this /editor/ui code. And need somebody from the Seamonkey side so check whether this does not break them.
Attachment #8606682 - Flags: review?(mkmelin+mozilla)
Attachment #8606682 - Flags: review?(iann_bugzilla)
(Reporter)

Comment 31

2 years ago
According to https://wiki.mozilla.org/Modules/All#Composer, Ian is a good choice (as you already knew).

Comment 32

2 years ago
Comment on attachment 8606682 [details] [diff] [review]
patch v1.1

I've noticed during testing that, on a compose window with a new profile, the drop down list of fonts has a separator at the very bottom of the list:
Variable Width
Fixed Width
----------------
Helvetica, Arial
Times
Courier
----------------

This might be related to the message in the error console:
Timestamp: 21/07/15 11:15:03
Error: TypeError: usedFontsSep is null
Source File: chrome://editor/content/editor.js
Line: 751

Comment 33

2 years ago
(In reply to Ian Neal from comment #32)
> Comment on attachment 8606682 [details] [diff] [review]
> patch v1.1
> 
> I've noticed during testing that, on a compose window with a new profile,
> the drop down list of fonts has a separator at the very bottom of the list:
> Variable Width
> Fixed Width
> ----------------
> Helvetica, Arial
> Times
> Courier
> ----------------
> 
> This might be related to the message in the error console:
> Timestamp: 21/07/15 11:15:03
> Error: TypeError: usedFontsSep is null
> Source File: chrome://editor/content/editor.js
> Line: 751

This error message occurs when selecting text within a quoted reply.
STR
1/ Reply to an email
2/ Select some text within the quoted email (font-family happens to be "Helvetica, Arial, sans-serif")
3/ Look at error console.

Comment 34

2 years ago
Comment on attachment 8606682 [details] [diff] [review]
patch v1.1

For the moment r- due to the issues found.
Attachment #8606682 - Flags: review?(iann_bugzilla) → review-

Updated

2 years ago
Attachment #8606682 - Flags: review?(neil)
Attachment #8606682 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 35

2 years ago
Created attachment 8648134 [details] [diff] [review]
patch v1.2

OK, I can't reproduce on TB, but it looks like I missed a place where the new classes need to be changed in editorOverlay.xul, which may only be used by SM.

Please try with this version.
Attachment #8606682 - Attachment is obsolete: true
Flags: needinfo?(acelists)
Attachment #8648134 - Flags: review?(iann_bugzilla)

Comment 36

2 years ago
Comment on attachment 8648134 [details] [diff] [review]
patch v1.2

looks good to me
Attachment #8648134 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Updated

2 years ago
Attachment #8648134 - Flags: review?(mkmelin+mozilla)
(Reporter)

Comment 37

2 years ago
Created attachment 8648468 [details]
Picture showing a case for a worthwhile improvement

The other day I received and e-mail whose author uses fancy fonts.
In the e-mail we find:
<font face="Comic Sans MS,sans-serif" size="3" color="#7030A0">

I noticed that in the "regular" TB the font indicator doesn't update, it shows "Variable Width".

With this patch, we get what is shown in the attachment. Much better, but there is still room for improvement.

I propose to look at the fonts that are listed, and then display the first one found, in this case "Comic Sans MS".

It would be so cool if the font indicator could really show us the font that is used, and in this case even installed.

Sorry to be picky, but let's discuss some more ;-)
(Assignee)

Comment 38

2 years ago
So how do we know the Gecko core actually used Comic Sans to render the text? Can you determine with the patch what exact font string we are given to look up in the list of fonts?
(Reporter)

Comment 39

2 years ago
Take a look at bug 1140105 and M-C/editor/libeditor/tests/test_bug1140105.html.
I think that
  editor.getInlineProperty(propAtom, "face", "font to query", firstHas, anyHas, allHas);
is your friend here.
That works in all cases now, even with collapsed selections (ie. user clicks).

Comment 40

2 years ago
Comment on attachment 8648134 [details] [diff] [review]
patch v1.2

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

::: editor/ui/composer/content/editor.js
@@ +745,5 @@
> +
> +    if (!foundFont) {
> +      // The text editor encountered a font that is not installed on this system,
> +      // so add it to the font menu now.
> +      let fontLabel = GetString("NotInstalled").replace("%font%", state);

this should use GetFormattedString
Attachment #8648134 - Flags: review?(mkmelin+mozilla) → review+
(Reporter)

Comment 41

2 years ago
I hope my problem from comment #37 gets fixed before this lands.
<font face="Comic Sans MS,sans-serif" size="3" color="#7030A0"> results in
"Comic Sans MS,sans-serif (not installed)".

If the font wasn't found and the 'stateToLower' has a comma, you need to look a little harder.
(Reporter)

Comment 42

2 years ago
One other thing: Have you looked at the font menu (Format > Font)?

This already doesn't update when you select "Variable Width", most likely due to this query:
if (!anyHas.value)
  EditorGetTextProperty("font", "face", "", firstHas, anyHas, allHas);

I think "" has been phased out, you'd need to query for "sans-serif" or "serif". Do you want me to put this into another bug?

Also, when I select my "Comic Sans MS,sans-serif", this menu doesn't check the "Comic Sans MS" entry.
The call
EditorGetTextProperty("font", "face", "Comic Sans MS", firstHas, anyHas, allHas);
returns (I tested) false/false/false when the selection has "Comic Sans MS,sans-serif". Sigh.

I think we need to give "EditorGetTextProperty" the flick here (sorry, and getInlineProperty as mentioned in comment #39 also doesn't serve the purpose) and use GetCurrentEditor().getFontFaceState(mixed); as per Neil's proposal in attachment 8591403 [details] [diff] [review]. That returns (I tested) "Comic Sans MS,sans-serif". That can be pulled apart and compared to the items in the menu list just like what needs to be done for the problem from comment #41.

Neil, do you have an opinion?
Flags: needinfo?(neil)
(Reporter)

Comment 43

2 years ago
Created attachment 8651466 [details] [diff] [review]
Additional processing: Strip generic font family names and rework font menu.

Sorry for jumping in here. I suggest to merge this into Aceman's patch, so we can settle the "font indicator doesn't update" problem once and for all. (The patch is not based on Aceman's patch, it's just based on trunk, but the merging effort is minimal). You can also make it more elegant and have a "normalise font name function".

The first thing this does is stripping generic font family names, so that
<font face="Comic Sans MS,sans-serif" size="3" color="#7030A0">
is correctly recognised in the font indicator.

The second part is a complete rework of "initLocalFontFaceMenu()" according to Neil's idea from attachment 8591403 [details] [diff] [review]. This is also far more efficient. Instead of asking Gecko one hundred times "Is it this font?" (EditorGetTextProperty), we ask once "Give me the font" (getFontFaceState) and then do our own processing which is 100% inline with the processing done for the font indicator.
Attachment #8651466 - Flags: feedback?(neil)
(Reporter)

Comment 44

2 years ago
Created attachment 8651474 [details] [diff] [review]
Additional processing: Strip generic font family names and rework font menu (v2).

Oops, if mixed is returned, don't set state to "" since this matches "Variable Width".
Attachment #8651466 - Attachment is obsolete: true
Attachment #8651466 - Flags: feedback?(neil)
Attachment #8651474 - Flags: feedback?(neil)
(In reply to Jorg K (GMT+2) from comment #42)
> One other thing: Have you looked at the font menu (Format > Font)?
> 
> This already doesn't update when you select "Variable Width", most likely
> due to this query:
> if (!anyHas.value)
>   EditorGetTextProperty("font", "face", "", firstHas, anyHas, allHas);
> 
> I think "" has been phased out, you'd need to query for "sans-serif" or
> "serif". Do you want me to put this into another bug?
Yes, this belongs in a separate bug (I thought there was one already but there are so many editor font regressions lately it's getting hard to keep track... sigh).

> Also, when I select my "Comic Sans MS,sans-serif", this menu doesn't check
> the "Comic Sans MS" entry.
...
> Neil, do you have an opinion?
Well, the two are not strictly the same. The first says "use Comic Sans MS if you can, or else the user's default sans serif font" while the second says "or else the user's default font of the user's default font type". (But you probably meant to use "Comic Sans MS, cursive" anyway.)
Flags: needinfo?(neil)
(Reporter)

Comment 46

2 years ago
(In reply to neil@parkwaycc.co.uk from comment #45)
> Yes, this belongs in a separate bug (I thought there was one already but
> there are so many editor font regressions lately it's getting hard to keep
> track... sigh).
Bug 1197687.

> (But you probably meant to use "Comic Sans MS, cursive" anyway.)
<font face="Comic Sans MS,sans-serif" size="3" color="#7030A0">
was sent to me from someone using the MSN/Outlook/Hotmail web interface.
(Reporter)

Comment 47

2 years ago
Comment on attachment 8651474 [details] [diff] [review]
Additional processing: Strip generic font family names and rework font menu (v2).

Sorry, Aceman, I'll leave your bug alone and carry on in bug 1197687.
Perhaps we land your stuff now and worry about the "double/multiple font"
<font face="Comic Sans MS,sans-serif" size="3" color="#7030A0">
in yet another bug. What do you think?
Attachment #8651474 - Attachment is obsolete: true
Attachment #8651474 - Flags: feedback?(neil)
(Assignee)

Comment 48

2 years ago
Yes, I would land it now, if you can work on your proposal in a new bug so you can work on top of this patch. I just need to address comment 40.
(Assignee)

Comment 49

2 years ago
Created attachment 8651810 [details] [diff] [review]
patch v1.3

Changed to GetFormattedString.
Attachment #8648134 - Attachment is obsolete: true
Attachment #8651810 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 50

2 years ago
https://hg.mozilla.org/comm-central/rev/0b5b80bde5b9edcc12f98c6146cedd28a074ce6d
Bug 1148790 - Add unknown fonts from message compose to the fonts menu. ui-r=bwinton, r=mkmelin, r=IanN

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 43.0
(Reporter)

Updated

a year ago
Depends on: 1265181

Updated

a year ago
See Also: → bug 1265198
(Assignee)

Updated

a year ago
Depends on: 1265326

Updated

2 months ago
Depends on: 1358049
You need to log in before you can comment on or make changes to this bug.