1267069 - Handle quotes around font names so the font indicator works better

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Jorg K (CEST = GMT+2)]

Attachment: 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.

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: Easy fix (v1).

Comment 2

[Jorg K (CEST = GMT+2)]

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

[Ian Neal]

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?

Comment 4

[Jorg K (CEST = GMT+2)]

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.

Comment 5

[Jorg K (CEST = GMT+2)]

(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.

Comment 6

[Jorg K (CEST = GMT+2)]

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.

Comment 7

[Jorg K (CEST = GMT+2)]

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

[:aceman]

(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" ?

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 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.

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: Showing fonts with quotes and experimenting with quotes in font names (more).

More play cases.

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: 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? ;-)

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: Showing fonts with quotes and experimenting with quotes in font names (even more).

Now including the |"serif"| we talked about.

Comment 13

[:aceman]

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

Comment 14

[Jorg K (CEST = GMT+2)]

Thanks, not an ideal solution until bug 785687 gets fixed.

Comment 15

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/0dc66f3da448

Comment 16

[Jorg K (CEST = GMT+2)]

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.

Comment 17

[Jorg K (CEST = GMT+2)]

Aurora (TB 47):
https://hg.mozilla.org/releases/comm-aurora/rev/9a02557da435

Comment 18

[Kent James (:rkent)]

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