The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Thunderbird 39.0

Status

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

People

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

Tracking

({regression})

36 Branch
Thunderbird 39.0
x86_64
Windows 7
regression

Thunderbird Tracking Flags

(thunderbird36? affected, thunderbird37? affected, thunderbird38+ fixed)

Details

Attachments

(2 attachments, 7 obsolete attachments)

(Assignee)

Description

2 years ago
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".
(Assignee)

Comment 1

2 years ago
Working on 31.5 and 36 beta, so clearly a regression.
Keywords: regression
(Assignee)

Comment 2

2 years ago
Sorry, already broken in 36 beta, works in 31.5.
Version: 38 → 36

Comment 3

2 years ago
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.)
(Assignee)

Comment 4

2 years ago
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

2 years ago
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.
(Assignee)

Comment 6

2 years ago
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.
(Assignee)

Comment 7

2 years ago
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?
(Assignee)

Updated

2 years ago
Blocks: 756984
(Assignee)

Updated

2 years ago
status-thunderbird36: --- → affected
status-thunderbird37: --- → affected
status-thunderbird38: --- → affected
tracking-thunderbird36: --- → ?
tracking-thunderbird37: --- → ?
tracking-thunderbird38: --- → ?
(Assignee)

Comment 8

2 years ago
Created attachment 8573093 [details] [diff] [review]
Patch to fix the problem. Removing spaces after comma and dealing with "sans-serif"
(Assignee)

Updated

2 years ago
Assignee: nobody → mozilla
(Assignee)

Updated

2 years ago
Attachment #8573093 - Attachment description: Patch to fix the problem. Removing spaces after comma and dealing with "sans-serif → Patch to fix the problem. Removing spaces after comma and dealing with "sans-serif"
(Assignee)

Updated

2 years ago
Attachment #8573093 - Flags: review?(neil)
(Assignee)

Comment 9

2 years ago
@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 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.
Created attachment 8573100 [details] [diff] [review]
Alternate approach

[This doesn't address the sans-serif problem you mention.]
Attachment #8573100 - Flags: feedback?(mozilla)
(Assignee)

Comment 12

2 years ago
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 = "";
+
(Assignee)

Comment 13

2 years ago
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.
Flags: needinfo?(neil)
(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.)
(Assignee)

Comment 15

2 years ago
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.
(Assignee)

Comment 16

2 years ago
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 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.)
Flags: needinfo?(neil)
Attachment #8573093 - Flags: review?(neil) → review-
(In reply to comment #17)
> OK, so this is another regression from bug 756984

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

Filed bug 1141017 for that.
(Assignee)

Comment 20

2 years ago
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?
(Assignee)

Updated

2 years ago
Flags: needinfo?(neil)
(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.
Flags: needinfo?(neil)
(Assignee)

Comment 22

2 years ago
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.
Flags: needinfo?(neil)
(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.
Flags: needinfo?(neil)
(Assignee)

Comment 24

2 years ago
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.
Flags: needinfo?(neil)
(Assignee)

Comment 25

2 years ago
Created attachment 8575191 [details] [diff] [review]
Modified original patch with changes to XUL files
Attachment #8573093 - Attachment is obsolete: true
Attachment #8575191 - Flags: review?(neil)
(Assignee)

Comment 26

2 years ago
Created attachment 8575195 [details] [diff] [review]
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.
Attachment #8575195 - Flags: review?(neil)
(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.
Flags: needinfo?(iann_bugzilla)
(Assignee)

Comment 28

2 years ago
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.
(Assignee)

Comment 29

2 years ago
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

2 years ago
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).
(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.
(Assignee)

Comment 32

2 years ago
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

2 years ago
TB 37 beta is built but is currently in QA. Will probably release today or tomorrow.
(Assignee)

Updated

2 years ago
No longer blocks: 756984
Depends on: 1141017
(Assignee)

Updated

2 years ago
Attachment #8573100 - Flags: feedback?(mozilla)
(Assignee)

Comment 34

2 years ago
Created attachment 8581505 [details] [diff] [review]
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.
Attachment #8581505 - Flags: review?(rkent)
(Assignee)

Updated

2 years ago
Attachment #8575195 - Flags: review?(neil) → review?(rkent)
(Assignee)

Updated

2 years ago
Attachment #8575191 - Flags: review?(neil) → review?(rkent)

Comment 35

2 years ago
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

2 years ago
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.
(Assignee)

Comment 37

2 years ago
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.
(Assignee)

Comment 38

2 years ago
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

2 years ago
(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

2 years ago
> > 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.
(Assignee)

Comment 41

2 years ago
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].
(Assignee)

Updated

2 years ago
Attachment #8575191 - Attachment is obsolete: true
Attachment #8575191 - Flags: review?(rkent)
(Assignee)

Updated

2 years ago
Attachment #8581505 - Attachment description: Patch to work aorund the core bug 1141017 that "sans-serif" is sometimes returned as font → Patch to work around the core bug 1141017 that "sans-serif" is sometimes returned as font

Comment 42

2 years ago
Maybe Ehsan is the right person for /editor review.
Flags: needinfo?(ehsan)
(Assignee)

Comment 43

2 years ago
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

2 years ago
(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

2 years ago
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.
(Assignee)

Comment 46

2 years ago
(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>.
(Assignee)

Comment 47

2 years ago
(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

2 years ago
(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

2 years ago
(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

2 years ago
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

2 years ago
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?
(Assignee)

Comment 52

2 years ago
(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.
Flags: needinfo?(ehsan)
(Assignee)

Comment 53

2 years ago
(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
(Assignee)

Comment 54

2 years ago
Created 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)

Added more comment.
Attachment #8581505 - Attachment is obsolete: true
Attachment #8581505 - Flags: review?(rkent)
Attachment #8583926 - Flags: review?(rkent)
(Assignee)

Comment 55

2 years ago
(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.
(Assignee)

Updated

2 years ago
Attachment #8573100 - Attachment is obsolete: true

Comment 56

2 years ago
(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

2 years ago
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

2 years ago
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.
(Assignee)

Comment 59

2 years ago
(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.
(Assignee)

Comment 60

2 years ago
(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

2 years ago
(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 :)
(Assignee)

Comment 62

2 years ago
Created attachment 8584081 [details] [diff] [review]
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 ;-)
Attachment #8575195 - Attachment is obsolete: true
Attachment #8575195 - Flags: review?(rkent)
Attachment #8584081 - Flags: review?(rkent)
Attachment #8584081 - Flags: feedback?(acelists)
(Assignee)

Comment 63

2 years ago
(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

2 years ago
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
Attachment #8584081 - Flags: feedback?(acelists) → feedback+

Comment 65

2 years ago
(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 :)
(Assignee)

Comment 66

2 years ago
Created attachment 8584098 [details] [diff] [review]
Normalise font before comparing (Neil's idea), now without trailing whitespace

Damn that editor!
Attachment #8584081 - Attachment is obsolete: true
Attachment #8584081 - Flags: review?(rkent)
Attachment #8584098 - Flags: review?(kent)
(Assignee)

Comment 67

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

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

Comment 68

2 years ago
(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"

Updated

2 years ago
Attachment #8584098 - Flags: feedback+

Comment 69

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

Looks good to me.
Attachment #8584098 - Flags: review?(kent) → review+

Comment 70

2 years ago
(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 ?
(Assignee)

Comment 71

2 years ago
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.
(Assignee)

Comment 72

2 years ago
Created attachment 8584117 [details]
Font indicator in Windows 7

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

Comment 73

2 years ago
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

2 years ago
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).
Attachment #8583926 - Flags: feedback+

Comment 75

2 years ago
(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.
(Assignee)

Comment 76

2 years ago
(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.
Flags: needinfo?(neil)
Flags: needinfo?(iann_bugzilla)
(Assignee)

Updated

2 years ago
Attachment #8583926 - Attachment is obsolete: true
Attachment #8583926 - Flags: review?(rkent)

Comment 77

2 years ago
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.

Updated

2 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 78

2 years ago
(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

2 years ago
(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 :)
(Assignee)

Comment 80

2 years ago
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

2 years ago
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
Attachment #8584098 - Flags: approval-comm-aurora?

Comment 82

2 years ago
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.
(Assignee)

Comment 83

2 years ago
(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.
(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

2 years ago
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
Attachment #8584098 - Flags: approval-comm-aurora? → approval-comm-beta+

Updated

2 years ago
status-thunderbird38: affected → fixed
(Assignee)

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED

Comment 86

2 years ago
(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

2 years ago
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

2 years ago
(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

2 years ago
Right, fixed.
tracking-thunderbird38: ? → +
(Assignee)

Comment 90

2 years ago
Landed on C-C on 2015-03-27 as per comment #81, so Target Milestone should be Thunderbird 39.
Target Milestone: --- → Thunderbird 39.0
You need to log in before you can comment on or make changes to this bug.