Handle quotes around font names so the font indicator works better

RESOLVED FIXED in Thunderbird 48.0

Status

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jorgk, Assigned: jorgk)

Tracking

Trunk
Thunderbird 48.0

Thunderbird Tracking Flags

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

Details

Attachments

(2 attachments, 4 obsolete attachments)

Posted file Showing fonts with quotes (obsolete) —
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.
Posted patch Easy fix (v1). (obsolete) — Splinter Review
Attachment #8744708 - Flags: review?(acelists)
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 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 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)
(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 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)
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.
(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" ?
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
More play cases.
Attachment #8744711 - Attachment is obsolete: true
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)
Attachment #8744713 - Attachment mime type: message/rfc822 → text/plain
Now including the |"serif"| we talked about.
Attachment #8744713 - Attachment is obsolete: true
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+
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
OS: Unspecified → All
Hardware: Unspecified → All
Attachment #8744716 - Flags: review?(iann_bugzilla) → review+
Thanks, not an ideal solution until bug 785687 gets fixed.
https://hg.mozilla.org/comm-central/rev/0dc66f3da448
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 48.0
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+
Attachment #8744716 - Flags: approval-comm-esr45? → approval-comm-esr45+
Duplicate of this bug: 1268635
You need to log in before you can comment on or make changes to this bug.