1139524 - Font indicator doesn't update when cursor is placed in text with this font

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Jorg K (CEST = GMT+2)]

Steps to reproduce:
Write a message with different fonts. One line of
Batang
Helvetica, Arial
Times
Courier
Aharoni.

Note: Instead of Batang and Aharoni you can use some other font.
Click on Batang and Aharoni: Font indicator updates.
Click on the other ones: Font indicator doesn't update.

Analysis:
http://mxr.mozilla.org/comm-central/source/editor/ui/composer/content/ComposerCommands.js#375
Added debug print:
dump("poking UI for "+uiID+", state="+desiredAttrib+"\n");

Result:
poking UI for cmd_fontFace, state=Batang
poking UI for cmd_fontFace, state=Helvetica,Arial,sans-serif
poking UI for cmd_fontFace, state=Times New Roman,Times,serif
poking UI for cmd_fontFace, state=Courier New,Courier,monospace

So core editor seems to return "font families" and therefore button menu gets confused.

I've also noticed that clicking on a text written without font (so default font) may return "sans-serif".

Comment 1

[Jorg K (CEST = GMT+2)]

Working on 31.5 and 36 beta, so clearly a regression.

Comment 2

[Jorg K (CEST = GMT+2)]

Sorry, already broken in 36 beta, works in 31.5.

Comment 3

[[e-mail]]

Not sure if this is related, but the Format - Font submenu is incorrect if there is no selection. (If you select some text in a font, then the menu displays that font correctly.)

Comment 4

[Jorg K (CEST = GMT+2)]

Good catch. It all hangs together in the message compose window.
You issue already exists in 31.5, so it's different.

I never use the "Format > Font" menu, but I get "Helvetica, Arial" displayed incorrectly in many occasions. Even if I change to "Courier" and revisit, it's back to "Helvetica, Arial".

Maybe better log a second bug.

Comment 5

[[e-mail]]

What seems to be happening is that Editor is now more aggressively returning the computed style for the font face instead of just reading the HTML tags. (Previously it only used to do this in CSS mode, i.e. SeaMonkey Composer, which doesn't have that indicator.)

Unfortunately this strips the spaces after the commas so the value we read back doesn't match up with the value we set.

Comment 6

[Jorg K (CEST = GMT+2)]

Indeed. I've already removed the spaces from the XUL file. Now I just have to figure out why my "mach build" doesn't seem to re-make the menu and then I have to figure out how to submit a patch. Hard to get started on all this.

I will also submit a fix that sometimes "sans-serif" is returned, where we have "" on the menus.

Comment 7

[Jorg K (CEST = GMT+2)]

oops, this stuff is in **four** XUL files:

editor\ui\composer\content\editorOverlay.xul
mail\components\compose\content\editorOverlay.xul
mail\components\preferences\compose.xul
suite\mailnews\compose\prefs\pref-composing_messages.xul

I've only changed the first one. Do I need to fix all four?

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: Patch to fix the problem. Removing spaces after comma and dealing with "sans-serif"

Comment 9

[Jorg K (CEST = GMT+2)]

@Neil from comment #3:
This doesn't fix the problem that the "Format > Font" menu only updates when you have a selection.
At least it updates correctly now when there is a selection.
Did you post another bug? If so, which?

Comment 10

[[e-mail]]

Comment on attachment 8573093 [details] [diff] [review]
Patch to fix the problem. Removing spaces after comma and dealing with "sans-serif"

Note that if someone with an earlier version replies to a message using these settings, then their font selector won't display your font correctly because it's looking for the string with spaces.

Comment 11

[[e-mail]]

Attachment: Alternate approach

[This doesn't address the sans-serif problem you mention.]

Comment 12

[Jorg K (CEST = GMT+2)]

OK, I thought about stripping the spaces. I can't decide what's better. I thinks it's clearer to fix the faulty XUL files then to do some "special" processing. More JS to be executed ;-(

As for the sans-serif, you need to take the bit I added:
+      // bug 1139524: Clicking on "default variable font" sometimes returns "", other times "sans-serif"
+      if (uiID == "cmd_fontFace" && desiredAttrib == "sans-serif")
+        desiredAttrib = "";
+

Comment 13

[Jorg K (CEST = GMT+2)]

Can we make a move on this? Decide which approach to take, get a review, check it in, get done with it? Or am I too impatient? ;-)
Not being able to reliably compose messages seriously impacts the usability of TB.

Comment 14

[[e-mail]]

(In reply to Jorg K from comment #12)
> As for the sans-serif, you need to take the bit I added:
> +      // bug 1139524: Clicking on "default variable font" sometimes returns
> "", other times "sans-serif"
> +      if (uiID == "cmd_fontFace" && desiredAttrib == "sans-serif")
> +        desiredAttrib = "";
It would be nice to know why we have to do this. (I have a sneaky suspicion that the result will be different if you change the default font in Preferences/Options.)

Comment 15

[Jorg K (CEST = GMT+2)]

Simple answer. Observed behaviour. Sometime it returns "", sometimes "sans-serif". If the code ever worked, it must have always returned "". If, as you suggested, you change the TB default display font to "Serif", "" seems to be returned always. I expected to see "serif" and "".

My test case was: Just reply to any e-mail quoting the original. The text "On such-and-such a date this person wrote ..." is set in the default font. Clicking on it sometimes returns "sans-serif".

More I cannot say about the internals.

Comment 16

[Jorg K (CEST = GMT+2)]

Just in case it needs more convincing, here some debug output:
poking UI for cmd_fontColor, state=rgb(0, 0, 0)
poking UI for cmd_align, state=left
poking UI for cmd_fontFace, state=sans-serif, will clobber
poking UI for cmd_fontFace, state=

Another case using a serif proportional default font:
poking UI for cmd_paragraphState, state=
poking UI for cmd_fontColor, state=rgb(0, 0, 0)
poking UI for cmd_fontFace, state=

from:
    var uiState = commandNode.getAttribute("state");
    if (desiredAttrib != uiState)
    {
      // bug 1139524: Clicking on "default variable font" sometimes returns "", other times "sans-serif"
      if (uiID == "cmd_fontFace" && desiredAttrib == "sans-serif") {
        dump("poking UI for "+uiID+", state="+desiredAttrib+", will clobber\n");
        desiredAttrib = "";
      }

      commandNode.setAttribute("state", desiredAttrib);
      dump("poking UI for "+uiID+", state="+desiredAttrib+"\n");
    }

Comment 17

[[e-mail]]

Comment on attachment 8573093 [details] [diff] [review]
Patch to fix the problem. Removing spaces after comma and dealing with "sans-serif"

>+      if (uiID == "cmd_fontFace" && desiredAttrib == "sans-serif")
>+        desiredAttrib = "";

(In reply to Jorg K from comment #15)
> Simple answer. Observed behaviour. Sometime it returns "", sometimes
> "sans-serif". If the code ever worked, it must have always returned "". If,
> as you suggested, you change the TB default display font to "Serif", ""
> seems to be returned always. I expected to see "serif" and "".

OK, so this is another regression from bug 756984, but (like bug 1140105) the fix belongs in core editor code. (In particular the core editor code already checks for "serif" which is why you don't see it, but it needs to check for sans-serif too.)

Comment 18

[[e-mail]]

(In reply to comment #17)
> OK, so this is another regression from bug 756984

Sorry, I meant bug 748313.

Comment 19

[[e-mail]]

(In reply to comment #18)
> OK, so this is another regression from bug 748313

Filed bug 1141017 for that.

Comment 20

[Jorg K (CEST = GMT+2)]

Thanks for taking care of the "sans-serif".

Now what about the second problem, the "comma space"? We need a patch for that.
My way by fixing the XUL or your way by removing the space on the fly?

Comment 21

[[e-mail]]

(In reply to Jorg K from comment #20)
> Now what about the second problem, the "comma space"? We need a patch for that.
> My way by fixing the XUL or your way by removing the space on the fly?

My concern is that if you change the XUL then you'll start sending out messages with <font face="Times,serif"> and older versions won't recognise it without the comma.

Comment 22

[Jorg K (CEST = GMT+2)]

Hmm, I don't understand this comment. Which messages? Sent where? Which older versions of what?

The XUL change is in the definition of menu items which are compared with what is returned from the editor to the listener. If the editor changed and is not likely to change back, I'd prefer not to add more code just to maintain the outdated XUL configuration.

Anyhow, if you prefer your change, perhaps just add a comment and get it checked in.

Comment 23

[[e-mail]]

(In reply to Jorg K from comment #22)
> The XUL change is in the definition of menu items which are compared with
> what is returned from the editor to the listener.

But it also specifies the DOM attribute to be set when you select that menuitem, which means that the menuitem will now start generating a slightly different DOM, one which won't be recognised by previous versions of the menuitem.

> Which older versions of what?
Working older versions of Thunderbird and SeaMonkey.

Comment 24

[Jorg K (CEST = GMT+2)]

Sorry, I still don't understand. This menu is part of the "Thunderbird package" (binaries, js, XUL configuration, etc.). Why would part of the package have to work with an older version of the same package? Or are you saying it could break an add-on?

Can you please state clearly which kind of patch you want, "my way" or "your way" and who will prepare it so that we can close this issue. We are talking about a one-line-change ("your way") or an 10-line-change ("my way", haven't counted exactly), so I'd like to get the fix over an done with to move onto something else.

If you favour your approach, please submit a new patch with comment or approve your own patch as is, or I can submit a new one with added comment. If you'd like me to do a test run first, I can do that too.

Comment 25

[Jorg K (CEST = GMT+2)]

Attachment: Modified original patch with changes to XUL files

Comment 26

[Jorg K (CEST = GMT+2)]

Attachment: Alternate approach by Neil, with added comment

Two options:
Changes to XUL files or stripping the space in js (also tested).
For option two there are two versions, one with comment, one without.
Please choose one.

Comment 27

[[e-mail]]

(In reply to Jorg K from comment #24)
> Or are you saying it could break an add-on?
No, I'm saying that if someone using e.g. ESR31 receives a message using the spaceless value then their reply window won't recognise the font you used.

I wonder whether Ian would care to chime in at this point.

Comment 28

[Jorg K (CEST = GMT+2)]

The patch will only be applied to TB 36 (if at all), TB 37 (if there is such thing, I heard something about 37 beta is coming), TB 38 and trunk.

TB 31 is not broken and doesn't need fixing. Obviously the behaviour of the m-c core changed between 31 and 36.

Sorry, I still don't see a problem.

Comment 29

[Jorg K (CEST = GMT+2)]

Or maybe I don't know the first thing about the source control here. I was under the impression that for TB 31 there is a *branch* for c-c as well as for m-c, same for TB 38. Is that not the case?

Comment 30

[Kent James (:rkent)]

It's not a branch for Thunderbird 31 exactly, it is a separate repository, but the idea is the same (http://hg.mozilla.org/releases/comm-esr31/).

TB 36 will have no further releases. TB 37 is in beta1, there is only a small chance there will be a second beta. So we would land on TB 39 initially, then port to TB 38 if appropriate. SM has a similar release schedule (though gecko36 might still be live for them).

Comment 31

[[e-mail]]

(In reply to Jorg K from comment #28)
> TB 31 is not broken and doesn't need fixing.

I didn't say that it needed fixing; I said that it might be confused if it sees a message created by a version of TB with your fix.

Comment 32

[Jorg K (CEST = GMT+2)]

Well, I keep out of it. Both proposed patches go well with TB 36 onwards. TB 36, 38 and daily all show the problem. I prefer my patch, but I'm happy as long as the problem gets fixed.

TB 37 is in beta1? Hmm, is that a virtual thing or can one download and install it?
https://ftp.mozilla.org/pub/mozilla.org/thunderbird/releases/latest-beta/win32/en-US/ has TB 36.

Comment 33

[Kent James (:rkent)]

TB 37 beta is built but is currently in QA. Will probably release today or tomorrow.

Comment 34

[Jorg K (CEST = GMT+2)]

Attachment: Patch to work around the core bug 1141017 that "sans-serif" is sometimes returned as font

Sadly no progress has been made on this trivial problem. The status is this:

Due to bug 1141017 the listener sometimes gets returned "sans-serif" which is not expected. As long as this is not fixed, we must discard "sans-serif". This is what this patch is about.

Also, one of the two other patches in attachment 8575191 [details] [diff] [review] or attachment 8575195 [details] [diff] [review] should be landed to guarantee a functioning font indicator. I'm changing the reviewer to R Kent since Neil somehow hasn't reviewed them and since the beta date for TB 38 is nearing.

Comment 35

[:aceman]

So which version is proper HTML (<font >)syntax? With commas or not?
1. We should make that the default TO BE PRODUCED from TB38 on.
2. Can we make TB38+ accept both variants if encountered in a message (TB38 will still receive msgs created in TB31 for undefined timeframe)?
3. Also, remember we are still making 31.x builds so we can create a fix for TB 31.6 that would make it recognize the new syntax in messages created with TB38+ (like Neil says in comment 27).

Would that work for everybody?

Comment 36

[:aceman]

If the editor is returning the font family names without spaces, we need to support it. But it may not be the proper syntax (there may actually be no intentional relation to the <font face> syntax) and may also change any time in the future. So we probably should not need to start producing different syntax of tags just to read back the values easier temporarily.

Comment 37

[Jorg K (CEST = GMT+2)]

31.x is not affected since 31.x is based on Gecko 31, so to me old versions are just a non-issue.

To cope with the "no space" font names, two patches are proposed:
patch 8575195 - stripping spaces out in code
patch 8575191 - removing spaces from the XUL config.
IMHO someone need to *make a decision* which patch to use for TB 38 onwards. If you want a "flexible solution", use the first option.

There is also the issue of "sans-serif" being returned. Patch 8581505 offers a workaround.

P.S.: AFAIK there is no "proper" HTML font syntax. With or without spaces is fine to use. But that's besides the point. Important is what the Gecko returns at the time of editing.
P.S.2: I don't understand the "in messages created with" issue. This is about the TB composer creating a new message. It has nothing to do with displaying sent/old/previous messages.

Comment 38

[Jorg K (CEST = GMT+2)]

I'm just re-reading this

(In reply to :aceman from comment #35)
> So which version is proper HTML (<font >)syntax? With commas or not?
> 1. We should make that the default TO BE PRODUCED from TB38 on.
> 2. Can we make TB38+ accept both variants if encountered in a message (TB38
> will still receive msgs created in TB31 for undefined timeframe)?

This is 100% off the topic.

The problem is that the listener in the TB message composer/editor for *NEW* messages gets the "no space" version returned from the core engine white composing/editing a *NEW* message. So the "font indicator" UI doesn't react correctly.

The HTML produced will be whatever Gecko hands back us as, most likely the "no space" version these days.

For TB to display an old/sent/previous message, it's passed to the Gecko engine for rendering. This engine can cope with both formats.

Comment 39

[:aceman]

(In reply to Jorg K from comment #38)
> I'm just re-reading this
> 
> (In reply to :aceman from comment #35)
> > So which version is proper HTML (<font >)syntax? With commas or not?
> > 1. We should make that the default TO BE PRODUCED from TB38 on.
> > 2. Can we make TB38+ accept both variants if encountered in a message (TB38
> > will still receive msgs created in TB31 for undefined timeframe)?
> 
> This is 100% off the topic.
> 
> The problem is that the listener in the TB message composer/editor for *NEW*
> messages gets the "no space" version returned from the core engine white
> composing/editing a *NEW* message. So the "font indicator" UI doesn't react
> correctly.
> 
> The HTML produced will be whatever Gecko hands back us as, most likely the
> "no space" version these days.
> 
> For TB to display an old/sent/previous message, it's passed to the Gecko
> engine for rendering. This engine can cope with both formats.

As Neil explained, people on TB31 (that can exist for a very long time) would start to receive newly formatted messages from TB38+. Displaying is fine. But if they REPLY to such a message and edit the text, their UI (with old code) would misbehave and have the bug here even in cases where there would be no bug had we not made a change for TB38. Because their old editor code is still behaving in the old way.

Comment 40

[:aceman]

> > The problem is that the listener in the TB message composer/editor for *NEW*
> > messages gets the "no space" version returned from the core engine white
> > composing/editing a *NEW* message. So the "font indicator" UI doesn't react
> > correctly.
> > 
> > The HTML produced will be whatever Gecko hands back us as, most likely the
> > "no space" version these days.

So my proposal is to keep the xul and font syntax message the same as is (and was in all previous TB versions). And adapt each new TB version to what the current editor returns. That should be easy to do and also does not need to cope with multiple source formats that would have been created in past versions of TB if we changed the XUL (thus tags in msgs source). Or decouple the menu XUL code from the HTML code that actually gets inserted into the composed message.

Comment 41

[Jorg K (CEST = GMT+2)]

My apologies. I was wrong. What you wrote was spot on and not at all off-topic. Please excuse the hasty and inappropriate reaction ;-(

You are 100% correct. Changing the XUL will create HTML that will not be recognised at earlier versions of TB (I tried it). So finally the penny dropped for me. So changing the XUL is therefore now an option. I will remove the patch.

So I think we all agree and can land attachment 8575195 [details] [diff] [review] (Neil's proposal) and attachment 8581505 [details] [diff] [review].

Comment 42

[:aceman]

Maybe Ehsan is the right person for /editor review.

Comment 43

[Jorg K (CEST = GMT+2)]

But Ehsan is a Mozilla core guy. Does he work on Thunderbird?
This is all about TB coping with what comes back from the core editor.

BTW, I re-read Neil's messages again. He had explained the problem all along, in comment 21, 23 and 27. I didn't get it. When I read "message" I was thinking of messages being passed around inside TB, so from the editor to the TB listener in the JS module. But he was talking about *mail messages* and the HTML they contain and that those newly formatted messages wouldn't be understood by older versions. My apologies again.

Comment 44

[:aceman]

(In reply to Jorg K from comment #43)
> But Ehsan is a Mozilla core guy. Does he work on Thunderbird?
> This is all about TB coping with what comes back from the core editor.
I think /editor is somehow "above" TB/SM. It is not inside /mozilla but I think it is still used by Firefox at some places. So it is at similar level as Toolkit (core tech shared by all projects) even if it is in another directory.
 
> BTW, I re-read Neil's messages again. He had explained the problem all
> along, in comment 21, 23 and 27. I didn't get it. When I read "message" I
> was thinking of messages being passed around inside TB, so from the editor
> to the TB listener in the JS module. But he was talking about *mail
> messages* and the HTML they contain and that those newly formatted messages
> wouldn't be understood by older versions. My apologies again.
Sure, no problem :)

Comment 45

[Kent James (:rkent)]

Although I am listed to review these patches, I have very little familiarity with the Editor code. I'll try to look at them, but Neil would be a better choice.

If you still need for me to look at them, perhaps aceman could give a feedback+ of the patches he has looked over and agreed with.

Comment 46

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #44)
> I think /editor is somehow "above" TB/SM. It is not inside /mozilla but I
> think it is still used by Firefox at some places. So it is at similar level
> as Toolkit (core tech shared by all projects) even if it is in another
> directory.

This time I can disagree a bit without being foolish ;-)

The editor is a part of Mozilla core and maintained by Mozilla core people. I know because I've just worked with Ehsan on bug 756984 and bug 1100966.

Ehsan expressed that he didn't know much about the internals of TB. We worked on those bugs exclusively using Firefox and <div contenteditable>.

Comment 47

[Jorg K (CEST = GMT+2)]

(In reply to Kent James (:rkent) from comment #45)
> Although I am listed to review these patches, I have very little familiarity
> with the Editor code. I'll try to look at them, but Neil would be a better
> choice.

I only hope that I didn't annoy Neil since I failed to understand what he tried to say about three times. Now I finally got it. Sorry.

I believe we all agree on what he suggested in the first place.

There is a little disagreement about the "sans-serif" patch, but I think we need to land it, since the M-C fix (bug 1141017) won't land in time.

Comment 48

[:aceman]

(In reply to Jorg K from comment #46)
> The editor is a part of Mozilla core and maintained by Mozilla core people.
> I know because I've just worked with Ehsan on bug 756984 and bug 1100966.
Then that is what I said :) 

> Ehsan expressed that he didn't know much about the internals of TB. We
> worked on those bugs exclusively using Firefox and <div contenteditable>.
Yes, but all patches I see are in /editor/ so there is nothing specific about TB in them (code wise). Am I missing any patches yet?

Comment 49

[:aceman]

(In reply to Kent James (:rkent) from comment #45)
> Although I am listed to review these patches, I have very little familiarity
> with the Editor code. I'll try to look at them, but Neil would be a better
> choice.
Yes, the problem is Neil wrote the patch :) So we look for somebody else.

> If you still need for me to look at them, perhaps aceman could give a
> feedback+ of the patches he has looked over and agreed with.
Yes, I can test it out.

Comment 50

[:aceman]

Comment on attachment 8581505 [details] [diff] [review]
Patch to work around the core bug 1141017 that "sans-serif" is sometimes returned as font

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

::: a/editor/ui/composer/content/ComposerCommands.js
@@ +373,5 @@
>      if (desiredAttrib != uiState)
>      {
> +      // bug 1139524: Clicking on "default variable font" sometimes returns "", other times "sans-serif"
> +      if (uiID == "cmd_fontFace" && desiredAttrib == "sans-serif")
> +        desiredAttrib = "";

Please add a comment that this needs to be removed after bug 1141017 lands.

Comment 51

[:aceman]

Comment on attachment 8573100 [details] [diff] [review]
Alternate approach

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

::: editor/ui/composer/content/editor.js
@@ +1589,5 @@
>    }
> +
> +  var findbar = document.getElementById("FindToolbar");
> +  if (findbar.browser != getBrowser())
> +    findbar.browser = getBrowser();

What is this for? Does it belong into this bug?

Comment 52

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #48)

> Yes, but all patches I see are in /editor/ so there is nothing specific
> about TB in them (code wise). Am I missing any patches yet?

Another misunderstanding? They are in 
comm-central/editor/ui/composer/content/editor.js
comm-central/editor/ui/composer/content/ComposerCommands.js

They are NOT in the Mozilla core, which is in
comm-central/mozilla/editor or in
mozilla-central/editor


So with all due respect, I will take Ehsan out.

Comment 53

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #51)

> What is this for? Does it belong into this bug?

Where do you see it? I can't see it in:
https://bug1139524.bugzilla.mozilla.org/attachment.cgi?id=8575195
https://bug1139524.bugzilla.mozilla.org/attachment.cgi?id=8581505

Comment 54

[Jorg K (CEST = GMT+2)]

Attachment: Patch to work around the core bug 1141017 that "sans-serif" is sometimes returned as font (take 2, more comment)

Added more comment.

Comment 55

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K from comment #53)
> (In reply to :aceman from comment #51)
> 
> > What is this for? Does it belong into this bug?

Sorry, that was in Neil's original patch (attachment 8573100 [details] [diff] [review]). I guess that was there by accident.

Comment 56

[Kent James (:rkent)]

(In reply to :aceman from comment #49)
> (In reply to Kent James (:rkent) from comment #45)
> > Although I am listed to review these patches, I have very little familiarity
> > with the Editor code. I'll try to look at them, but Neil would be a better
> > choice.
> Yes, the problem is Neil wrote the patch :) So we look for somebody else.

That somebody else at this point is you and Jorg. If the three of you agree then I'll probably just rubber stamp it. I would give it more attention if I was the only person looking over a patch from an inexperienced developer, but that is not the situation here.

Comment 57

[:aceman]

Comment on attachment 8583926 [details] [diff] [review]
Patch to work around the core bug 1141017 that "sans-serif" is sometimes returned as font (take 2, more comment)

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

::: editor/ui/composer/content/ComposerCommands.js
@@ +372,5 @@
>      var uiState = commandNode.getAttribute("state");
>      if (desiredAttrib != uiState)
>      {
> +      // Bug 1139524: Clicking on "default variable font" sometimes returns "",
> +      // other times "sans-serif".

May the "" be returned at the start of the text? The produced code of a message looks like this:
 <body bgcolor="#FFFFFF" text="#000000">
    SDFG SDFGD<font
        face="ClearlyU PUA">D</font>JFKG dfkg skdgf skd<font
      face="serif">jg<font face="sans-serif"> skdgd</font></font> fd<font
      face="Helvetica, Arial, sans-serif">gdfgsdgfsfdg</font><br>
  </body>

So the default font is not defined at the start of the message, but there is a font face serif block in the middle of it. Both of them show as "sans-serif" in the menulist (with this patch).

Comment 58

[:aceman]

Comment on attachment 8575195 [details] [diff] [review]
Alternate approach by Neil, with added comment

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

::: editor/ui/composer/content/editor.js
@@ +693,5 @@
>      for (var i=0; i < menuItems.length; i++)
>      {
>        var menuItem = menuItems.item(i);
> +      // bug 1139524: Menu item has for example "Helvetica, Arial, sans-serif", but we need to compare as "Helvetica,Arial,sans-serif"
> +      if (menuItem.getAttribute("label") && ("value" in menuItem && menuItem.value.toLowerCase().replace(/, /g, ",") == state.toLowerCase()))

Please wrap lines for 80 columns, at some logical place.

Why do we check for the label attribute here? Should it be changed to menuItem.hasAttribute("label")? Or should intentionally empty label (label="") skipped too?

You can cache 'state.toLowerCase()' in a temporary variable initialized before the loop. Could win some perf as there may be many items in the menupopup. If you wanted to be real fancy and actually implement your suggestion partially, you could precompute menuItem.value.toLowerCase().replace(/, /g, ",") for each menuitem and store it in some attribute. Hopefully the items do not change inside a compose session.

Comment 59

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #57)

I'm not sure I fully understand your comment.
If you look at bug 1141017 and the proposed patch,
https://bugzilla.mozilla.org/attachment.cgi?id=8574618&action=diff
you will see the the core editor strips out "serif" and "monospace", but fails to strip "sans-serif" for some reason. The proposed patch wasn't accepted since a lot of tests failed that test for "sans-serif".

My test case is this: I have the TB default font set to proportional, variable, sans-serif. When I reply to an e-mail the first line "On 24/03/2015 17:32, Bugzilla@Mozilla wrote:" is set in the default. 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.

Since "serif" or "monospace" is never returned from the core editor, but "" is returned, we can't distinguish those two. With the patch we can't distinguish "sans-serif" either, but that's expected. At least the font indicator gets updated to whatever the users default is.

I don't know which "menu list" you're referring to, since "sans-serif" is not on any menu.

Comment 60

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #58)

> Why do we check for the label attribute here? Should it be changed to
> menuItem.hasAttribute("label")? Or should intentionally empty label
> (label="") skipped too?

Please don't ask me that. Look:
Old: menuItem.value.toLowerCase()
New: menuItem.value.toLowerCase().replace(/, /g, ",")
I didn't change anything else.

I will submit a new patch with lines limited to 80 columns and state.toLowerCase() pre-calculated. Coming up in a second ;-)

Comment 61

[:aceman]

(In reply to Jorg K from comment #60)
> Please don't ask me that. Look:
> Old: menuItem.value.toLowerCase()
> New: menuItem.value.toLowerCase().replace(/, /g, ",")
> I didn't change anything else.

I do not necessarily ask you, I thought Neil was the author of this idea :)

Comment 62

[Jorg K (CEST = GMT+2)]

Attachment: Normalise font before comparing (Neil's idea)

Here comes the improved patch. Since we use a temporary variable I allowed for stripping the spaces out here was well, just in case the core editor should ever return them again. Better be safe than sorry ;-)

Comment 63

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #61)
> (In reply to Jorg K from comment #60)
> > Please don't ask me that. Look:
> > Old: menuItem.value.toLowerCase()
> > New: menuItem.value.toLowerCase().replace(/, /g, ",")
> > I didn't change anything else.
> 
> I do not necessarily ask you, I thought Neil was the author of this idea :)

Well, you'd need to ask the original author, who according to "blame" is bugzilla@250. Hmm.
All that was added here was .replace(/, /g, ",")
You're asking questions that go beyond the change ;-)

Comment 64

[:aceman]

Comment on attachment 8584081 [details] [diff] [review]
Normalise font before comparing (Neil's idea)

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

Thanks, works for me.

::: editor/ui/composer/content/editor.js
@@ +689,5 @@
>    else
>    {
>      var menuPopup = document.getElementById("FontFacePopup");
>      var menuItems = menuPopup.childNodes;
> +    

trailing whitespace

@@ +694,5 @@
> +    // 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, ",");
> +    

trailing whitespace

@@ +699,4 @@
>      for (var i=0; i < menuItems.length; i++)
>      {
>        var menuItem = menuItems.item(i);
> +      if (menuItem.getAttribute("label") && ("value" in menuItem && 

trailing whitespace

Comment 65

[:aceman]

(In reply to Jorg K from comment #63)
> All that was added here was .replace(/, /g, ",")
> You're asking questions that go beyond the change ;-)
Well, it is part of the logic of matching the right menuitem. But if the answer is unknown then of course leave it as it is, it should not get worse :)

Comment 66

[Jorg K (CEST = GMT+2)]

Attachment: Normalise font before comparing (Neil's idea), now without trailing whitespace

Damn that editor!

Comment 67

[Jorg K (CEST = GMT+2)]

Can you please set the feedback+ again on Attachment #8584098 [details] [diff].

What are we doing about the "sans-serif"?

Comment 68

[Kent James (:rkent)]

(In reply to Jorg K from comment #67)
> Can you please set the feedback+ again on Attachment #8584098 [details] [diff]
> [diff].

The convention is that you set feedback+ with a comment "carrying over feedback+ from Aceman"

Comment 69

[Kent James (:rkent)]

Comment on attachment 8584098 [details] [diff] [review]
Normalise font before comparing (Neil's idea), now without trailing whitespace

Looks good to me.

Comment 70

[:aceman]

(In reply to Jorg K from comment #59)
> Since "serif" or "monospace" is never returned from the core editor, but ""
> is returned, we can't distinguish those two. With the patch we can't
> distinguish "sans-serif" either, but that's expected. At least the font
> indicator gets updated to whatever the users default is.
> 
> I don't know which "menu list" you're referring to, since "sans-serif" is
> not on any menu.
I do have "serif", "monospace" and "sans-serif" in the composer font menu. The default compose text shows "sans-serif" as the font. I can choose another font and then overwrite it with sans-serif fine:
<body bgcolor="#FFFFFF" text="#000000">
    sdf a<font face="serif">sdf s fsdf asf</font>d asdf asd a<font
      face="Andale Mono">sdf <font face="sans-serif">sad</font>f a</font>sdf
    asd<br>
  </body>

Why would I want to suppress detecting sans-serif ?

Comment 71

[Jorg K (CEST = GMT+2)]

My font indicator menu reads:
Variable Width
Fixed Width
----------
Helvetica, Arial
Times
Courier
----------
(long list of fonts)

Please look at comment #59 very carefully. "serif" and "mono space" are already suppressed by the M-C core editor. If we don't suppress "sans-serif" as well, the font indicator doesn't work, as described in the comment. "sans-serif" was never returned, that's new in Gecko > 31.

If in 31.5 I click on the "On 24/03/2015 17:32, Bugzilla@Mozilla wrote:" the font indicator changes to "Variable Width". In TB 36+ is doesn't. So that's clearly a regression which is in the process of being fixed.

Comment 72

[Jorg K (CEST = GMT+2)]

Attachment: Font indicator in Windows 7

BTW: "sans-serif" is not a Font, it's a font family. It should never appear on any menu.

Comment 73

[Jorg K (CEST = GMT+2)]

OK, Linux users may not understand why "sans-serif" must be suppressed. Here a little chat:

[2015-03-26 23:46:08] <jorgk> aceman: OK, I'm on Linux now, yes, serif, monospace and sans-serif show in the menu.
[2015-03-26 23:50:40] <jorgk> OK. To understand the problem better, you need a Windows machine. Linux is different.
[2015-03-26 23:50:51] <jorgk> Anyway, the problem happens on Linux too.
[2015-03-26 23:51:34] <jorgk> If you set the default font to sans-serif and click on some old unformatted text in a reply, you get sans-serif in the font indicator.
[2015-03-26 23:52:25] <jorgk> If you set the default font to serif and click some old unformatted text in a reply, you get "Variable Width", since the menu for that is associated with "" and "serif" is returned as "".
[2015-03-26 23:53:04] <jorgk> In fact, if you set the default display font to serif and start typing it jumps back to "Variable Width".
[2015-03-26 23:53:57] <jorgk> If you type in monospace, the menu entry jumps to "Fixed Width" since the core editor is returning "tt", not a font.
[2015-03-26 23:54:10] <aceman> so why are the entries there?
[2015-03-26 23:54:22] <jorgk> If you apply the patch I proposed, the special treatment of sans-serif goes away.
[2015-03-26 23:54:46] <jorgk> I have no idea. I've never seen them in my life, only now, when I booted the Linux box.
[2015-03-26 23:55:31] <jorgk> I guess there is a system call to query the installed fonts and Linux foolishly returns these "non-fonts".
[2015-03-26 23:56:22] <jorgk> To make the behaviour consistent on Linux and to make it work on Windows, my patch is required.
[2015-03-26 23:56:35] <aceman> but on linux users will be confused
[2015-03-26 23:56:47] <jorgk> Just install the patch and try the different behavious.
[2015-03-26 23:56:54] <aceman> they select one font, type in it and then another font is shown?
[2015-03-26 23:57:10] <aceman> (edited) yes, that already happens without your patch for 2 "fonts" (serif and monospace)
[2015-03-26 23:57:53] <jorgk> Yes, if the select monospace (as the default display font) it jumps to Fixed Width.
[2015-03-26 23:58:08] <jorgk> If the select serif, it jumps to Variable Width
[2015-03-26 23:58:24] <jorgk> If they select sans-serif, it stays at sans-serif.
[2015-03-26 23:58:35] <jorgk> And that's at least inconsistent.

Conclusion: Please try my patch on Linux an see whether it becomes more consistent. Otherwise we need a special case for Windows, since on Windows "sans-serif" is simply not understood at all.

Comment 74

[:aceman]

Comment on attachment 8583926 [details] [diff] [review]
Patch to work around the core bug 1141017 that "sans-serif" is sometimes returned as font (take 2, more comment)

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

I find it useful to be able to compose the message in generic fonts (serif, sans-serif, monospace), as in comment 70, and have the recipient render it in his fonts set for these generic descriptions.

However, if this only works on Linux and already behaves strangely for 2 of the "fonts", I am fine with having it at least consistent for all 3 of them (meaning all 3 behaving strangely and falling back to other items in the menulist).

Would you be willing to look at it in another bug and making it consistent across platforms? Maybe hiding those 3 items on linux?
Also, how does the menulist behave if it encounters an unknown font (sent in a message from another computer)? It seems to me it selects nothing so the previous item is still selected (which would be misleading).

Comment 75

[:aceman]

(In reply to :aceman from comment #74)
> Would you be willing to look at it in another bug and making it consistent
> across platforms? Maybe hiding those 3 items on linux?
Or actually adding them on other platforms so we have the option of just using these generic names.

Comment 76

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #74)

> Also, how does the menulist behave if it encounters an unknown font (sent in
> a message from another computer)? It seems to me it selects nothing so the
> previous item is still selected (which would be misleading).

Correct.

I created a new bug 1148330 to discuss these issues.

I'll remove attachment 8583926 [details] [diff] [review], so please land attachment 8584098 [details] [diff] [review], which has been reviewed and accepted and fixes the regression.

Comment 77

[:aceman]

Why are you removing the other attachment? It isn't the perfect fix but if it improves at least one platform (Windows) then I have accepted it.

Comment 78

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #77)
> Why are you removing the other attachment? It isn't the perfect fix but if
> it improves at least one platform (Windows) then I have accepted it.

It moved to the other bug. This bug is done. Let's shift the discussion to bug 1148330.
Hey, that was your suggestion and your word is my command ;-)

Comment 79

[:aceman]

(In reply to Jorg K from comment #78)
> It moved to the other bug. This bug is done. Let's shift the discussion to
> bug 1148330.
> Hey, that was your suggestion and your word is my command ;-)
Yeah sorry, I only meant to move the "platform consistency" and "unknown font" problems into new bug :)

Comment 80

[Jorg K (CEST = GMT+2)]

I think your suggestion was very good, to split the accepted from the doubtful and yet unexplored ;-)

So can someone land the agreed patch here.

Comment 81

[Kent James (:rkent)]

Comment on attachment 8584098 [details] [diff] [review]
Normalise font before comparing (Neil's idea), now without trailing whitespace

[Approval Request Comment]
Consider for comm-aurora after nightly cycle

https://hg.mozilla.org/comm-central/rev/fb4c85295ffa

Comment 82

[Kent James (:rkent)]

Jorg, I'm not sure if you are intending to mark this bug as done and continue in another bug, or if you want to keep it open.

Also, please prepare your patches so that the bug number, description, and the username are already preset in the patch.

Comment 83

[Jorg K (CEST = GMT+2)]

(In reply to Kent James (:rkent) from comment #82)
> Jorg, I'm not sure if you are intending to mark this bug as done and
> continue in another bug, or if you want to keep it open.

I'd like to close this bug. There is already bug 1148330 with more problems concerning the font indicator. However, these problems are not so important. The font indicator sometimes doesn't work when clicking in the reply text. This bug was much more important, since the font indicator didn't work on new text in a new e-mail.

> Also, please prepare your patches so that the bug number, description, and
> the username are already preset in the patch.

Of course, sorry. I will in the future. Since so many versions were tossed around, the last version ended up here without the proper header.

Comment 84

[[e-mail]]

(In reply to aceman from comment #70)
> Why would I want to suppress detecting sans-serif ?

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.

Comment 85

[Kent James (:rkent)]

Comment on attachment 8584098 [details] [diff] [review]
Normalise font before comparing (Neil's idea), now without trailing whitespace

[Triage Comment]

Landed https://hg.mozilla.org/releases/comm-beta/rev/6d656cd76b60

Comment 86

[:aceman]

(In reply to Kent James (:rkent) from comment #85)
> Comment on attachment 8584098 [details] [diff] [review]
> Normalise font before comparing (Neil's idea), now without trailing
> whitespace
> 
> [Triage Comment]
> 
> Landed https://hg.mozilla.org/releases/comm-beta/rev/6d656cd76b60

Kent, does this bug still need "tracking-TB38 ?" when it apparently already landed in 38?

Comment 87

[Kent James (:rkent)]

Someone convinced me that normal practice is to leave the tracking flag set even after the patch lands, so that if it ever gets backed out it would get noticed. So tracking-38 flag is still set, and you have to reject items with status-thunderbird38 = fixed to see what needs attention.

Comment 88

[:aceman]

(In reply to Kent James (:rkent) from comment #87)
> Someone convinced me that normal practice is to leave the tracking flag set
> even after the patch lands, so that if it ever gets backed out it would get
> noticed. So tracking-38 flag is still set, and you have to reject items with
> status-thunderbird38 = fixed to see what needs attention.

I meant whether the value of tracking-TB38 should specifically be at "?" (question mark), instead of "+" (plus), i.e. actually set.

Comment 89

[Kent James (:rkent)]

Right, fixed.

Comment 90

[Jorg K (CEST = GMT+2)]

Landed on C-C on 2015-03-27 as per comment #81, so Target Milestone should be Thunderbird 39.