Closed Bug 1148330 Opened 9 years ago Closed 9 years ago

Font indicator doesn't update when cursor is placed in text where core returns sans-serif (Windows). Serif and monospace don't work (Linux).

Categories

(Thunderbird :: Message Compose Window, defect)

All
Linux
defect
Not set
normal

Tracking

(thunderbird38+ fixed, thunderbird39 fixed, thunderbird40 fixed)

RESOLVED FIXED
Thunderbird 40.0
Tracking Status
thunderbird38 + fixed
thunderbird39 --- fixed
thunderbird40 --- fixed

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

Details

(Keywords: polish)

Attachments

(2 files, 7 obsolete files)

1.32 KB, patch
jorgk-bmo
: review+
jorgk-bmo
: feedback+
Details | Diff | Splinter Review
3.59 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
Attached patch Suppress the sans-serif (obsolete) — Splinter Review
This problem is split off from what was reported in bug 1139524.

Summary:
Windows:
Due to bug 1141017 the core editor strips out "serif" and "monospace", but fails to strip "sans-serif" for some reason.

Test case: Have the TB default font set to proportional, variable, sans-serif. Reply to an e-mail, the first line (for example)
"On 24/03/2015 17:32, Bugzilla@Mozilla wrote:"
is set in the default font. Clicking on it, does not update the font indicator. It should jump to "Variable Width". If I set me default font to "serif", clicking on the reply text *does* update the font indicator.

Linux:
On Linux "serif", "monospace" and "sans-serif" appear on the font menus (very strange, since they are not fonts but font families).
When the user selects "serif", the font indicator jumps to "Variable Width", since the core editor returns font "", which is wired up to menu entry "Variable Width". If the user selects "monospace", the font indicator jumps to "Fixed Width", since the core editor returns "tt", which is handled separately. If the user selects "sans-serif", the font indicator displays "sans-serif" as desired.
This behaviour is inconsistent.

Two fixes are necessary:
1) Suppress "sans-serif" so that the behaviour will be consistent, as long as bug 1141017 isn't fixed.
2) Linux only: Remove the "non-fonts" from the menus.

Another observation: If the user clicks on a reply-text whose font information TB can't interpret, the font indicator does not update. Not nice, but it's always been that way. There in no entry "Unknown" on the menu.
Please also see new comment in bug 1141017 comment #12.
If "serif" and "monospace" were returned, that would be a whole different kettle of fish.
My hasty patch is here now, but I think we need to wait until some decisions are made.

First question: What happens to bug 1141017? Will the "sans-serif" be suppressed or not? If not, will the now suppressed "serif" and "monospace" be resurrected?

If so, we would have things work better in Linux. Then we can think about a Windows fix. We could add serif, sans-serif and monospace to the menu to make it more like Linux.

Or we can do exactly the opposite: Suppress serif, sans-serif and monospace and remove these three from the Linux menus. That would be what I said in the description.

I'm open to both approaches, however, I favour the suppression of the non-fonts.

Perhaps Neil can tell us his opinion.
Flags: needinfo?(neil)
1. Yes, I would support the keeping of serif, sans-serif and monospace items in the menu and also add them to other platforms, because it looks useful to me to be able to compose messages like
<font face=serif>abc</font><font face=sans-serif>def</font> and have them rendered in each user's (sender and receiver) default fonts for these classes.

But this needs support from the editor so that it actually returns these "fonts" if they are in the msg body. Unless that is done, we need to hide them on Linux. We can do that quickly in this bug and keep another bug for the editor open with the request of resurrecting these "non-fonts".

2. For the "unknown font" problem, yes there is no "unknown" entry in the font list. Either we create it, or we can even append new fonts names to the menulist. I think we can emit the font tags with unknown font names even if the font is unknown. It will not be rendered correctly, but that may not be a problem. Even in today's selection of fonts I have on linux, I can't see any rendering difference in some of them.
I'm not sure who's working on this bug. Let's assume that I am ;-)
We need to address this issue asap.

Here is a patch what will suppress the sans-serif and also remove the three non-fonts serif, sans-serif and monospace from the menus in the font indicator and the preferences. Since I'm not on Linux, I can't test it.

So, Aceman, please test and give me some feedback. Thanking you in advance ;-)
Assignee: nobody → mozilla
Attachment #8584371 - Attachment is obsolete: true
Attachment #8585039 - Flags: feedback?(acelists)
Actually, it's only removed from the "composition font" preference.
It's not removed from the "display font" preference. Changes in
mail/components/preferences/display.js
mail/components/preferences/fonts.js
would be necessary to do so. Look for "FontBuilder.buildFontList".
That comes from mozilla-central: toolkit/mozapps/preferences/fontbuilder.js

Let me know what you think before taking the matter further.
I don't think we need to remove those non-fonts from display list. Let the user display msgs in whatever he wants. For this bug, only disable creating messages with such non-fonts.
Perfect, then just give me feedback on the patch ;-)
Comment on attachment 8585039 [details] [diff] [review]
Suppress the sans-serif and remove serif, sans-serif and monospace from font lists

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

>1148330 - Font indicator doesn't update, sans-serif issue, Linux; r=aceman
Thanks for properly formatting the patch now. Please make the commit message also say what the patch is doing (how it is fixing it), not only the description of the bug. E.g.:
"Bug 1148330 - Hide serif,sans-serif and monospace font items from available fonts in message composer as they do not properly work due to editor core not returning them when queried for the style of a text span."

Please remove me from the line as I am not a reviewer yet ;)

Other than that, the patch does work for me, thanks.

Are you planning to do the "unknown font in received message" problem in this bug/patch?
Here is small update, just with a better header line as per Aceman's request.
Please give me feedback+

(In reply to :aceman from comment #8)
> Other than that, the patch does work for me, thanks.
> Are you planning to do the "unknown font in received message" problem in
> this bug/patch?

Well, honestly, this has never worked and I don't see any urgency to get it fixed now. I will be away for a few weeks and I don't know whether I will manage to get a fix in for it.

I believe we should land this change now to avoid a regression of the font indicator not updating (since sans-serif is returned and not understood). At the same time this patch fixes the strange behaviour observed on Linux that you select one thing and the indicator jumps to another.

Just in case Kent will ask: I'd leave this bug open for further consideration later. We also don't know what the decision in the Mozilla core (bug 1141017) will be: suppress "sans-serif" or resurrect "serif" and "monospace" (or do nothing ;-() All we know is that we won't get a decision for mozilla38, so we need to act *now* and work around the currently existing problem.
Attachment #8585039 - Attachment is obsolete: true
Attachment #8585039 - Flags: feedback?(acelists)
Attachment #8585053 - Flags: review?(kent)
Attachment #8585053 - Flags: feedback?(acelists)
Comment on attachment 8585053 [details] [diff] [review]
Suppress the sans-serif and remove serif, sans-serif and monospace from font lists (revised header line)

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

OK :)
Attachment #8585053 - Flags: feedback?(acelists) → feedback+
(In reply to Jorg K from comment #9)
> (In reply to :aceman from comment #8)
> > Other than that, the patch does work for me, thanks.
> > Are you planning to do the "unknown font in received message" problem in
> > this bug/patch?
> 
> Well, honestly, this has never worked and I don't see any urgency to get it
> fixed now. I will be away for a few weeks and I don't know whether I will
> manage to get a fix in for it.

Then you shouldn't have mentioned it in this bug description ;) Please copy the problem to a followup bug and I can look at it in the next weeks.
Status: NEW → ASSIGNED
Keywords: polish
OS: Windows 7 → Linux
Hardware: x86_64 → All
Summary: Font indicator doesn't update when cursor is placed in text with this font (follow up bug), Linux inconsistencies → Font indicator doesn't update right when cursor is placed in text with fonts in message source like serif, sans-serif, monospace. The fonts do exist in the menu on Linux, but are not returned by the core style system.
OK. Here is the comes the follow-up: bug 1148790

Just in case Kent should ask:
We can close the bug once the patch has landed. The remaining problem has been carried over to bug 1148790.
Summary: Font indicator doesn't update right when cursor is placed in text with fonts in message source like serif, sans-serif, monospace. The fonts do exist in the menu on Linux, but are not returned by the core style system. → Font indicator doesn't update when cursor is placed in text where core returns sans-serif (Windows). Removed fonts from menu which don't work (Linux).
You do not need to change the summary to what was actually done. It just describes the problem.
Better? I just wanted to shorten your long narrative ;-)
Summary: Font indicator doesn't update when cursor is placed in text where core returns sans-serif (Windows). Removed fonts from menu which don't work (Linux). → Font indicator doesn't update when cursor is placed in text where core returns sans-serif (Windows). Serif and monospace don't work (Linux).
Better :)
Kent and Aceman:
Can we hold-off the review for another day or so. Looks like over in bug 1141017 we're going for the resurrection of "serif" and "monospace", so we need to do a bit more suppressing here ;-)
I've made the previously proposed patch more future-proof.
We will resurrect "serif" and "monospace" in the core editor (bug 1141017).

So now I reinterpret "serif" or "sans-serif" as default variable font, "" on the menu. I reinterpret "monospace" as "tt. Note that bug 1141017 hasn't landed, but when it lands, we are already prepared and don't have to revisit this. Right now, "serif" and "monospace" are not returned, but "sans-serif" is, which needs to be reinterpreted anyway.

For Linux the fonts "serif", "sans-serif" and "monospace" are removed from the menu since the names collide with the font family names returned from the editor. Since right now the code editor does not return "serif" and "monospace", selecting these fonts will automatically update the menu to "Variable Width" and "Fixed Width". This doesn't happen with "sans-serif" and is therefore inconsistent behaviour.

Please not that Neil wrote about this in bug 1139524 comment #84 (quote):

> You see sans-serif in that list because it's a list of system fonts and
> Linux supports a font called sans-serif which is set somewhere in 
> Linux preferences.

> Unfortunately this causes real confusion with this menu because that wants
> to list CSS fonts and the CSS sans-serif font is the one in Thunderbird
> preferences which admittedly does default to the one in Linux preferences
> but does not necessarily apply.

> When the composer was first written the idea was that you would select
> Helvetica, Arial or Times or Courier if you wanted sans-serif, serif or
> monospace. (This has the added advantage of showing up correctly on other
> people's replies.) Nobody ever envisaged the confusion arising from having
> system fonts with the same name as CSS fonts.
Attachment #8585053 - Attachment is obsolete: true
Attachment #8585053 - Flags: review?(kent)
Attachment #8585223 - Flags: review?(rkent)
Attachment #8585223 - Flags: feedback?(acelists)
Comment on attachment 8585223 [details] [diff] [review]
Reinterpret font families serif, sans-serif and monospace. Hide them from fonts menu as they do not work due to a core editor problem

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

So by reading the code, this probably does what you say.

But I wonder about one thing (not caused by your patch):
when the user selects Variable width font, it's value is "". No <font> tag is output into the message source. How does it then render on the recipients client? Will it be his default font, which may also be fixed width?

Isn't then the menuitem misleading? Shouldn't it better be called "unset". And have the "Variable width" actually set a variable width font, e.g. "serif" ?
Attachment #8585223 - Flags: feedback?(acelists) → feedback+
(In reply to :aceman from comment #18)
> But I wonder about one thing (not caused by your patch):
> when the user selects Variable width font, it's value is "". No <font> tag
> is output into the message source. How does it then render on the recipients
> client? Will it be his default font, which may also be fixed width?

Yes, absolutely. It's always been that way historically.

There is a very nasty side effect with this, which is when the font is lost (bug 250539 and bug 756984). If you have your default composition font and your default display font same, and you lose the font during the composition, the following gets sent:
<font face="default composition font">So far so good</font>Now we lost the font.
On the screen of the sender, the line has the exact same font. But the recipient sees something else.
Anyway, that's fixed now ;-) Even TB experts like Kent James have fallen victim of this problem. The other day he wrote to me and in the last two paragraphs of his message the font had gotten lost ;-)

I think, whoever wants to force a font to be sent needs to select it as default composition font (and make sure it doesn't get lost).
Bug 1141017 has landed, so we need the patch to correctly process the "serif", "sans-serif" and "monospace" returned from the core editor.

We also need this patch in TB 38, since "sans-serif" was already returned by mozilla38.
Comment on attachment 8585223 [details] [diff] [review]
Reinterpret font families serif, sans-serif and monospace. Hide them from fonts menu as they do not work due to a core editor problem

I am not at all familiar with this code, and both neil and ian are active editors in this area. So I'm going to give this to neil. If that does not work out, try iann.
Attachment #8585223 - Flags: review?(rkent) → review?(neil)
Since some work is going on in bug 1148790 we might not want to change the current erratic Linux behaviour now only to change it back to something else later.
In this case only the change in editor/ui/composer/content/ComposerCommands.js should be landed, not the changes editor/ui/composer/content/editor.js and mail/components/preferences/compose.js.

Neil, can you please give your opinion *now* and review the patch. I can revise it accordingly.

Without the patch, TB 38 would be shipped with a regression where "variable width" is not recognised any more.
Comment on attachment 8585223 [details] [diff] [review]
Reinterpret font families serif, sans-serif and monospace. Hide them from fonts menu as they do not work due to a core editor problem

>+      if (uiID == "cmd_fontFace") {
>+        if (desiredAttrib == "sans-serif" || desiredAttrib == "serif") {
>+          // Bug 1148330: Clicking on "default variable font" may return
>+          // "sans-serif" or "serif". Interpret it as "default variable".
>+          desiredAttrib = "";
>+        } else if (desiredAttrib == "monospace") {
>+          // Interpret font family "monospace" as "tt".
>+          desiredAttrib = "tt";
>+        }
>+      }
I don't like this approach. Unfortunately I've only just got around to coding up my proposal, which I'm going to attach.

>       if (gLocalFonts[i] != "")
>       {
>+        // Bug 1148330: Remove fonts from the list that collide with font family names (Linux).
>+        if (gLocalFonts[i] == "serif" ||
>+            gLocalFonts[i] == "monospace" ||
>+            gLocalFonts[i] == "sans-serif")
>+          continue;
I have something similar, as you'll see in my patch.

>diff --git a/mail/components/preferences/compose.js b/mail/components/preferences/compose.js
>--- a/mail/components/preferences/compose.js
>+++ b/mail/components/preferences/compose.js
Sorry, but I don't have review authority here.
Flags: needinfo?(neil)
Attachment #8585223 - Flags: review?(neil)
Attached patch Possible patch (obsolete) — Splinter Review
Attachment #8591391 - Flags: review?(iann_bugzilla)
I tried the "possible patch" in attachment 8591391 [details] [diff] [review]. It works fine.

You said about mail/components/preferences/compose.js
> Sorry, but I don't have review authority here.

Well, if we hide the Linux sans-serif, serif and monospace fonts, it doesn't make much sense to have them on Options, Composition, Font, don't you think?

I revised my patch and I think we need it. So who can review this?
Flags: needinfo?(neil)
Attachment #8585223 - Attachment is obsolete: true
Attached patch Possible patch (obsolete) — Splinter Review
Sorry, typo in previous patch.
Attachment #8591391 - Attachment is obsolete: true
Attachment #8591391 - Flags: review?(iann_bugzilla)
Flags: needinfo?(neil)
Attachment #8591401 - Flags: review?(iann_bugzilla)
(In reply to Jorg K (at the beach until 22nd April) from comment #25)
> Well, if we hide the Linux sans-serif, serif and monospace fonts, it doesn't
> make much sense to have them on Options, Composition, Font, don't you think?

I agree. I'm not familiar with the Thunderbird reviewers, I've been piling reviews on mkmelin a lot recently so I don't really want to burden him down further if I can help it ;-)
Comment on attachment 8591397 [details] [diff] [review]
Addition to Neil's patch, need to remove fonts from compose preferences

I'll ask Kent for a reviewer. Perhaps you can give a feedback+ in the meantime.
Flags: needinfo?(kent)
Attachment #8591397 - Flags: feedback?(neil)
I think mkmelin is a good choice for this part of code.
Flags: needinfo?(kent)
Attachment #8591397 - Flags: review?(mkmelin+mozilla)
Attachment #8591397 - Flags: feedback?(neil) → feedback+
Comment on attachment 8591397 [details] [diff] [review]
Addition to Neil's patch, need to remove fonts from compose preferences

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

LGTM, assuming Neil's patch lands of course

::: mail/components/preferences/compose.js
@@ +214,5 @@
>        var localFontCount = { value: 0 }
>        var localFonts = enumerator.EnumerateAllFonts(localFontCount);
>        for (var i = 0; i < localFonts.length; ++i)
>        {
> +        // Remove Linux system generic fonts that collide with CSS generic fonts

nit: add a period at the end of the sentence.
Attachment #8591397 - Flags: review?(mkmelin+mozilla) → review+
Neil: When you get your review, can you land both together, please. Can you add the missing period at the end. I can submit a new patch, but then the reviewed one loses its "review+".
Fixed nit.
Carrying forward Neil's feedback+ and Magnus' review+
Attachment #8591397 - Attachment is obsolete: true
Attachment #8594451 - Flags: review+
Attachment #8594451 - Flags: feedback+
Comment on attachment 8591401 [details] [diff] [review]
Possible patch

>+++ b/editor/ui/composer/content/editor.js

>+    // Generic variable-width

>+    // Generic fixed width
Nit: these comments should be consistent either both with a "-" or both without. Also should both end with a "."
r=me with that addressed
Attachment #8591401 - Flags: review?(iann_bugzilla) → review+
Carrying forward Ian's review+.
Fixed nit.
Attachment #8591401 - Attachment is obsolete: true
Attachment #8598644 - Flags: review+
Comment on attachment 8598644 [details] [diff] [review]
Neil's patch (nit fixed).

[Approval Request Comment]
Requested for beta (TB38) and aurora (TB39) to avoid regression due to changed m-c behaviour.
Attachment #8598644 - Flags: approval-comm-beta?
Attachment #8598644 - Flags: approval-comm-aurora?
Comment on attachment 8594451 [details] [diff] [review]
Addition to Neil's patch, need to remove fonts from compose preferences

[Approval Request Comment]
Requested for beta (TB38) and aurora (TB39) to avoid regression due to changed m-c behaviour.

Second part of the fix so ensure consistent behaviour.
Attachment #8594451 - Flags: approval-comm-beta?
Attachment #8594451 - Flags: approval-comm-aurora?
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 40.0
Comment on attachment 8594451 [details] [diff] [review]
Addition to Neil's patch, need to remove fonts from compose preferences

https://hg.mozilla.org/releases/comm-aurora/rev/49615525b791
https://hg.mozilla.org/releases/comm-beta/rev/3bd15f5dd687
Attachment #8594451 - Flags: approval-comm-beta?
Attachment #8594451 - Flags: approval-comm-beta+
Attachment #8594451 - Flags: approval-comm-aurora?
Attachment #8594451 - Flags: approval-comm-aurora+
Comment on attachment 8598644 [details] [diff] [review]
Neil's patch (nit fixed).

https://hg.mozilla.org/releases/comm-aurora/rev/e0fa5f7f4288
https://hg.mozilla.org/releases/comm-beta/rev/30bd41ec59a3
Attachment #8598644 - Flags: approval-comm-beta?
Attachment #8598644 - Flags: approval-comm-beta+
Attachment #8598644 - Flags: approval-comm-aurora?
Attachment #8598644 - Flags: approval-comm-aurora+
Here's some text that should go somewhere into the release notes:

Linux provides system fonts "sans-serif", "serif" and "monospace". These font names conflict with the generic CSS font family names. Therefore and for compatibility with Thunderbirds on other platforms,the ability to compose messages using these three fonts was removed on Linux (bug 1148330). Fonts "sans-serif", "serif" and "monospace" will no longer be shown on the font menu in the message composition window and have been removed from the composition font preference. Users who had selected one of these fonts as their default composition font have to select a different font. Instead of "sans-serif" users should select "Helvetica, Arial" or "Variable Width", instead of "serif" users should select "Times" or also "Variable Width", and instead of "monospace", users should select "Courier" or "Fixed Width".

The three aforementioned Linux system fonts can still be used as default display fonts.
You need to log in before you can comment on or make changes to this bug.