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

RESOLVED FIXED in Thunderbird 48.0

Status

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jorgk, Assigned: aceman)

Tracking

({regression})

Trunk
Thunderbird 48.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird46 wontfix, thunderbird47 fixed, thunderbird48 fixed, thunderbird_esr4546+ fixed)

Details

Attachments

(1 attachment)

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 ;-)
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
Variable and Fixed are detected elsewhere, but "Helvetica, Arial", "Courier" and "Times" rely on the comparison with the menu items.
Posted patch patchSplinter Review
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.
Assignee: nobody → acelists
Status: NEW → ASSIGNED
Attachment #8742084 - Flags: review?(mozilla)
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
Attachment #8742084 - Flags: review?(mozilla) → review+
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.
Attachment #8742084 - Flags: approval-comm-esr45?
Attachment #8742084 - Flags: approval-comm-beta?
Attachment #8742084 - Flags: approval-comm-aurora+
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.
Beta and ESR landers: Please apply patches from bug 1260697 and bug 1265170 first.
(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.
Flags: needinfo?(iann_bugzilla)
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.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Flags: needinfo?(iann_bugzilla) → needinfo?(philip.chee)
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 48.0
Keywords: leave-open
Attachment #8742084 - Flags: approval-comm-esr45? → approval-comm-esr45+
(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.
Flags: needinfo?(philip.chee)
Attachment #8742084 - Flags: approval-comm-beta? → approval-comm-beta-
Duplicate of this bug: 1270264
You need to log in before you can comment on or make changes to this bug.