The default bug view has changed. See this FAQ.

Handle quotes around font names so the font indicator works better

RESOLVED FIXED in Thunderbird 48.0

Status

Thunderbird
Message Compose Window
RESOLVED FIXED
11 months ago
11 months ago

People

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

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

11 months 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

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

Comment 2

11 months 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

11 months 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

11 months 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

11 months 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

11 months 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

11 months 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

11 months 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

11 months 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

11 months 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

11 months 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

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

Comment 12

11 months 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

11 months 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

11 months ago
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
OS: Unspecified → All
Hardware: Unspecified → All

Updated

11 months ago
Attachment #8744716 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 14

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

Comment 15

11 months ago
https://hg.mozilla.org/comm-central/rev/0dc66f3da448
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months 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

11 months 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

11 months ago
Aurora (TB 47):
https://hg.mozilla.org/releases/comm-aurora/rev/9a02557da435
status-thunderbird47: affected → fixed

Comment 18

11 months ago
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

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

Updated

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