1265181 - Font selector showing bad information for "Helvetica, Arial", "Courier" and "Times".

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Jorg K (CEST = GMT+2)]

Choosing "Helvetica, Arial" shows "Helvetica, Arial, sans-serif (not installed)", "Courier" shows "Courier New, Courier, monospace (not installed)" and "Times" shows "Times New Roman, Times, serif (not installed)".

Regression from bug 1148790.

Great, no one ever tested the stuff from bug 1148790 on the three standard fonts.

Aceman, can you please fix, I approve ;-)

Comment 1

[Jorg K (CEST = GMT+2)]

The problem is that you set
itemNode.setAttribute("value_parsed", aFontName.toLowerCase().replace(/, /g, ","));
only for the fonts that come from the system, not for the first five items on the menu:
Variable, Fixed, "Helvetica, Arial", "Courier" and "Times".

The comparison here
dump (menuItem.getAttribute("value_parsed")+"|"+stateToLower+"\n");
if (menuItem.getAttribute("value_parsed") == stateToLower) {

looks like this:
|helvetica,arial,sans-serif
|helvetica,arial,sans-serif
|helvetica,arial,sans-serif
|helvetica,arial,sans-serif
|helvetica,arial,sans-serif
aharoni|helvetica,arial,sans-serif
anastasia extended 1|helvetica,arial,sans-serif
anastasia extended 2|helvetica,arial,sans-serif
anastasia extra|helvetica,arial,sans-serif
anastasia figured bass|helvetica,arial,sans-serif
anastasia rehearsals|helvetica,arial,sans-serif
andalus|helvetica,arial,sans-serif

Comment 2

[Jorg K (CEST = GMT+2)]

Variable and Fixed are detected elsewhere, but "Helvetica, Arial", "Courier" and "Times" rely on the comparison with the menu items.

Comment 3

[:aceman]

Attachment: patch

Thanks for noticing. There are 2 occurences of this menu in TB (one in the toolbar and one in Format->Font menu). And then 2 in base editor that are overriden by the TB overlay.

Comment 4

[Jorg K (CEST = GMT+2)]

Comment on attachment 8742084 [details] [diff] [review]
patch

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

Very good. Thanks for the quick and simple fix.

Please submit another patch for SM:
https://dxr.mozilla.org/comm-central/source/suite/mailnews/compose/prefs/pref-composing_messages.xul#154

Comment 5

[Jorg K (CEST = GMT+2)]

Comment on attachment 8742084 [details] [diff] [review]
patch

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky):

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky):

[Approval Request Comment]
Regression caused by (bug #): Bug 1148790.
User impact if declined: Very ugly looking font indicator in some cases.
Testing completed (on c-c, etc.): Manual testing.
Risk to taking this patch (and alternatives if risky):
No risk. Add a few attributes in XUL files.

Comment 6

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/398d0dcc371b

Leave-open to wait for SM fix.

Comment 7

[Jorg K (CEST = GMT+2)]

Aurora (TB 47 only, SM to follow):
https://hg.mozilla.org/releases/comm-aurora/rev/eb8f339d7d06

Comment 8

[Jorg K (CEST = GMT+2)]

There was an apply order problem due to bug 1260697 and bug 1265170, so this had to come out and be landed again. Sorry about that.

Aurora (TB 47 only, SM to follow):
https://hg.mozilla.org/releases/comm-aurora/rev/b46542f375b6 - Backout
https://hg.mozilla.org/releases/comm-aurora/rev/c6fe825dd6dc - Landed again.

SM patch to follow.

Comment 9

[Jorg K (CEST = GMT+2)]

Beta and ESR landers: Please apply patches from bug 1260697 and bug 1265170 first.

Comment 10

[:aceman]

(In reply to Jorg K (GMT+2) from comment #4)
> Please submit another patch for SM:
> https://dxr.mozilla.org/comm-central/source/suite/mailnews/compose/prefs/
> pref-composing_messages.xul#154

I don't think this needs a SM version. The linked file looks like font selection in preferences, which would NOT need this fix. I did not find other occurrences of the font selection, it is possible SM uses the editor/ui/composer/content/editorOverlay.xul version, which I have patched.

Comment 11

[Jorg K (CEST = GMT+2)]

Yes, you're right, my mistake. This is the SM composition preference. Ratty, can you please confirm.

Repeating:

Beta and ESR landers:
https://hg.mozilla.org/releases/comm-aurora/rev/c6fe825dd6dc

Please apply patches from bug 1260697 and bug 1265170 first.

Comment 12

[Kent James (:rkent)]

http://hg.mozilla.org/releases/comm-esr45/rev/70063c391758

Comment 13

[Philip Chee]

(In reply to Jorg K (GMT+2) from comment #11)
> Yes, you're right, my mistake. This is the SM composition preference. Ratty,
> can you please confirm.
Confirmed.