Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Handle quotes around font names so the font indicator works better

RESOLVED FIXED in Thunderbird 48.0

Status

Thunderbird
Message Compose Window
RESOLVED FIXED
a year ago
a year ago

People

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

Tracking

Trunk
Thunderbird 48.0

Thunderbird Tracking Flags

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

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

a year ago
Created attachment 8744707 [details]
Showing fonts with quotes

In a sample e-mail I found this font:

<span style="font-size:11.0pt;font-family:&quot;Calibri&quot;,&quot;sans-serif&quot;;color:#1F497D">

Looks like this is produced by some MS software.
(Assignee)

Comment 1

a year ago
Created attachment 8744708 [details] [diff] [review]
Easy fix (v1).
Attachment #8744708 - Flags: review?(acelists)
(Assignee)

Comment 2

a year ago
The current font indicator shows:
"Calibri","sans-serif" (not installed)

And if you use this font, the resulting HTML is:
<font face="&quot;Calibri&quot;,&quot;sans-serif&quot;">

Incredible ;-)

Comment 3

a year ago
Comment on attachment 8744708 [details] [diff] [review]
Easy fix (v1).

>--- a/editor/ui/composer/content/editor.js
>+++ b/editor/ui/composer/content/editor.js
>@@ -685,16 +685,19 @@ function onFontFaceChange(fontFaceMenuLi
>     fontFaceMenuList.selectedIndex = 1;
>     return;
>   default:
>   }
> 
>   let menuPopup = fontFaceMenuList.menupopup;
>   let menuItems = menuPopup.childNodes;
> 
>+  // Strip quotes in font names.
>+  editorFont = editorFont.replace(/"/g, "");
Shouldn't this be part of the default switch case?


>@@ -949,16 +952,19 @@ function initFontFaceMenu(menuPopup)
>         // Generic fixed width.
>         editorFont = "tt";
>         break;
>       default:
>         editorFont = editorFont.toLowerCase().replace(/, /g, ","); // bug 1139524
>       }
>     }
> 
>+    // Strip quotes in font names.
>+    editorFont = editorFont.replace(/"/g, "");
Shouldn't this be part of the default switch case?
(Assignee)

Comment 4

a year ago
Comment on attachment 8744708 [details] [diff] [review]
Easy fix (v1).

Turns out that this is no good.

Font names can be quoted strings:
https://www.w3.org/TR/CSS2/fonts.html#font-family-prop

So "Aldo's font" or 'Aldo\'s font' or 'Calibri' are valid, too.

So blind replacement doesn't help us here.
Attachment #8744708 - Flags: review?(acelists)
(Assignee)

Comment 5

a year ago
(In reply to Ian Neal from comment #3)
> Shouldn't this be part of the default switch case?
No, I think it's OK as proposed.
(Assignee)

Comment 6

a year ago
Comment on attachment 8744708 [details] [diff] [review]
Easy fix (v1).

This would be OK for now. I've played around with
    <font face="Aldo's font">Aldo's font</font><br>
    <font face='Aldo"s font'>Aldo"s font</font><br>
    <font face='Aldo\'s font'>Aldo's font</font><br>
    <font face='"Calibri"'>"Calibri"</font><br>
    <font face="'Calibri'">'Calibri'</font><br>
and on the last two, are interpreted correctly by core, and the font is enclosed by double quotes, even if there are single quotes in the source. So the patch will work for those cases.
Attachment #8744708 - Flags: review?(acelists)
(Assignee)

Comment 7

a year ago
OK, also this
    <span style='font-family:"Calibri"'>"Calibri"</span><br>
    <span style="font-family:'Calibri'">'Calibri'</span><br>
returns "Calibri" in both cases from the editor.

So just blindly stripping all double quotes we see is OK.

Comment 8

a year ago
(In reply to Jorg K (GMT+2) from comment #5)
> (In reply to Ian Neal from comment #3)
> > Shouldn't this be part of the default switch case?
> No, I think it's OK as proposed.

I think it should be done before the switch(). What if you get "sans-serif" ?
(Assignee)

Comment 9

a year ago
Created attachment 8744711 [details]
Showing fonts with quotes and experimenting with quotes in font names.

I've been experimenting with font names. The only quotes ever returned from the editor are around the name, so "Calibri". Doesn't return 'Calibri'. Doesn't return "Aldo's font". Doesn't even handle font/font-family strings with quotes.

So we can safely strip double quotes.
Attachment #8744707 - Attachment is obsolete: true
(Assignee)

Comment 10

a year ago
Created attachment 8744713 [details]
Showing fonts with quotes and experimenting with quotes in font names (more).

More play cases.
Attachment #8744711 - Attachment is obsolete: true
(Assignee)

Comment 11

a year ago
Created attachment 8744716 [details] [diff] [review]
Easy fix (v2).

Now before the switch. Now "'serif'" returns |"serif"| from the editor and is mapped to variable width in the case statement. Happy? ;-)
Attachment #8744708 - Attachment is obsolete: true
Attachment #8744708 - Flags: review?(acelists)
Attachment #8744716 - Flags: review?(acelists)

Updated

a year ago
Attachment #8744713 - Attachment mime type: message/rfc822 → text/plain
(Assignee)

Comment 12

a year ago
Created attachment 8744718 [details]
Showing fonts with quotes and experimenting with quotes in font names (even more).

Now including the |"serif"| we talked about.
Attachment #8744713 - Attachment is obsolete: true

Comment 13

a year ago
Comment on attachment 8744716 [details] [diff] [review]
Easy fix (v2).

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

::: editor/ui/composer/content/editor.js
@@ +668,5 @@
>    var editorFont = commandNode.getAttribute("state");
>  
> +  // Strip quotes in font names. Experiments have shown that we only
> +  // ever get double quotes around the font name, never single quotes,
> +  // even if the were in the HTML source. Also single or double

they

@@ +942,5 @@
>      var editorFont = GetCurrentEditor().getFontFaceState(mixed);
> +
> +    // Strip quotes in font names. Experiments have shown that we only
> +    // ever get double quotes around the font name, never single quotes,
> +    // even if the were in the HTML source. Also single or double

they
Attachment #8744716 - Flags: review?(iann_bugzilla)
Attachment #8744716 - Flags: review?(acelists)
Attachment #8744716 - Flags: review+

Updated

a year ago
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
OS: Unspecified → All
Hardware: Unspecified → All

Updated

a year ago
Attachment #8744716 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 14

a year ago
Thanks, not an ideal solution until bug 785687 gets fixed.
(Assignee)

Comment 15

a year ago
https://hg.mozilla.org/comm-central/rev/0dc66f3da448
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-thunderbird46: --- → wontfix
status-thunderbird47: --- → affected
status-thunderbird48: --- → fixed
status-thunderbird_esr45: --- → affected
tracking-thunderbird_esr45: --- → +
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 48.0
(Assignee)

Comment 16

a year ago
Comment on attachment 8744716 [details] [diff] [review]
Easy fix (v2).

[Approval Request Comment]
Regression caused by (bug #): no regression
User impact if declined: Font indicator improved in bug 1265326 still not working in all cases.
Testing completed (on c-c, etc.): Manual texting.
Risk to taking this patch (and alternatives if risky):
Simple two-liner, not risky.
Attachment #8744716 - Flags: approval-comm-esr45?
Attachment #8744716 - Flags: approval-comm-beta-
Attachment #8744716 - Flags: approval-comm-aurora+
(Assignee)

Comment 17

a year ago
Aurora (TB 47):
https://hg.mozilla.org/releases/comm-aurora/rev/9a02557da435
status-thunderbird47: affected → fixed
For TB 45.1: http://hg.mozilla.org/releases/comm-esr45/rev/64ab19c84279
For TB 45.1b1: http://hg.mozilla.org/releases/comm-beta/rev/1391b4dd016b
status-thunderbird_esr45: affected → fixed
tracking-thunderbird_esr45: + → 46+

Updated

a year ago
Attachment #8744716 - Flags: approval-comm-esr45? → approval-comm-esr45+
(Assignee)

Updated

a year ago
Duplicate of this bug: 1268635
You need to log in before you can comment on or make changes to this bug.