Bug 79448 - REGRESSION: Outlook 2007 doesn't display fonts correctly on emails composed by WebKit
: REGRESSION: Outlook 2007 doesn't display fonts correctly on emails composed b...
Status: RESOLVED FIXED
: WebKit
HTML Editing
: 528+ (Nightly build)
: Unspecified Unspecified
: P1 Normal
Assigned To:
:
: GoogleBug, InRadar, Regression
:
:
  Show dependency treegraph
 
Reported: 2012-02-23 23:02 PST by
Modified: 2012-05-02 11:00 PST (History)


Attachments
Screenshot: old create-models page (45.15 KB, image/png)
2012-02-25 16:10 PST, Ryosuke Niwa
no flags Details
Screenshot: Old merge-tests page (17.25 KB, image/png)
2012-02-25 16:11 PST, Ryosuke Niwa
no flags Details
Screenshot: shiny(tm) new admin page (69.33 KB, image/png)
2012-02-25 16:11 PST, Ryosuke Niwa
no flags Details
Needs a rubber stamp (16.86 KB, patch)
2012-02-25 16:25 PST, Ryosuke Niwa
no flags Review Patch | Details | Formatted Diff | Diff
Patch (11.21 KB, patch)
2012-02-29 21:31 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch (4.65 KB, patch)
2012-02-29 23:53 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch (4.16 KB, patch)
2012-03-01 00:03 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (4.15 KB, patch)
2012-03-01 00:55 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (4.24 KB, patch)
2012-03-01 04:02 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-02-23 23:02:54 PST
Something in CSS serializer must have changed recently in the last 6 months or so. We now produce single quotation around font family names:
e.g. <font face="'trebuchet ms', sans-serif">something</font>
instead of <font face="trebuchet ms, sans-serif">something</font>
and Outlook 2007 fails to render the latter properly.

A very simple fix is to strip any single quotation that appears in the font family name when creating a font element. Even though font family name could contain a quotation mark in the theory, that shouldn't be a problem in practice.
------- Comment #1 From 2012-02-24 05:58:38 PST -------
The spec requires quotes for font names if they don't consist of a sequence of one or more identifiers separated by single spaces, or if a font happens to be named the same thing as one of the reserved keywords:

http://www.w3.org/TR/CSS21/fonts.html#font-family-prop

But "font-family: trebuchet ms, sans-serif" is valid without quotes around "trebuchet ms".
------- Comment #2 From 2012-02-24 10:29:11 PST -------
IIRC, browsers are very inconsistent here. If we're going to add quotations back in, we should double check what IE/Opera/Firefox do. Specifically, do they use double or single quotes?
------- Comment #3 From 2012-02-24 14:02:03 PST -------
Caused by changes in bug 64783?
------- Comment #4 From 2012-02-24 14:02:46 PST -------
<rdar://problem/10929517>
------- Comment #5 From 2012-02-24 14:06:27 PST -------
(In reply to comment #3)
> Caused by changes in bug 64783?

The bug 64783 is fixing the CSS parser though, not the CSS serializer.
------- Comment #6 From 2012-02-25 16:10:40 PST -------
Created an attachment (id=128888) [details]
Screenshot: old create-models page
------- Comment #7 From 2012-02-25 16:11:00 PST -------
Created an attachment (id=128889) [details]
Screenshot: Old merge-tests page
------- Comment #8 From 2012-02-25 16:11:56 PST -------
Created an attachment (id=128890) [details]
Screenshot: shiny(tm) new admin page
------- Comment #9 From 2012-02-25 16:14:12 PST -------
Oops, sorry, these screenshots are for the bug 79585 :(
------- Comment #10 From 2012-02-25 16:25:50 PST -------
Created an attachment (id=128892) [details]
Needs a rubber stamp
------- Comment #11 From 2012-02-25 16:26:45 PST -------
(From update of attachment 128892 [details])
Ugh... wrong bug again :(
------- Comment #12 From 2012-02-27 18:33:50 PST -------
I'll take a look.
------- Comment #13 From 2012-02-28 04:08:02 PST -------
valueForFamily() in CSSComputedStyleDeclaration.cpp creates CSSPrimitiveValue based on the given faily name with type CSS_STRING. CSSPrimitiveValue::customCssText() returns single-quoted value if the type is CSS_STRING and the value contains spaces.

I couldn't find when the behavior changed. In r80463, which was landed March 2011, valueForFamily() creates CSSPrimitiveValue with type CSS_STRING and CSSPrimitiveValue::customCssText() quotes the value.

Maybe we can fix the regression by introducing a new primitive type and . Not sure it's worth to do.
------- Comment #14 From 2012-02-28 04:10:48 PST -------
(In reply to comment #13)
> Maybe we can fix the regression by introducing a new primitive type and .

... and just return the value as is in customCSssText().
------- Comment #15 From 2012-02-29 21:31:14 PST -------
Created an attachment (id=129629) [details]
Patch
------- Comment #16 From 2012-02-29 21:32:40 PST -------
(In reply to comment #15)
> Created an attachment (id=129629) [details] [details]
> Patch

I'm not sure this is really a regression, though.
------- Comment #17 From 2012-02-29 21:39:39 PST -------
(From update of attachment 129629 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=129629&action=review

> Source/WebCore/css/CSSPrimitiveValue.cpp:844
> +            text = m_value.string;

?? We can't do this per Aryeh's comment, right? If you have something like A ! B as a font name, then we need to quote it.
------- Comment #18 From 2012-02-29 23:53:08 PST -------
Created an attachment (id=129645) [details]
Patch
------- Comment #19 From 2012-02-29 23:56:02 PST -------
(From update of attachment 129645 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=129645&action=review

> LayoutTests/editing/style/font-face-unquote.html:9
> +<script src="script-tests/font-face-unquote.js"></script>

Please use http://trac.webkit.org/browser/trunk/Tools/Scripts/make-new-script-test instead so that you don't have a separate js file.
------- Comment #20 From 2012-03-01 00:03:26 PST -------
Created an attachment (id=129648) [details]
Patch
------- Comment #21 From 2012-03-01 00:04:05 PST -------
(From update of attachment 129645 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=129645&action=review

Thank you for review.

>> LayoutTests/editing/style/font-face-unquote.html:9
>> +<script src="script-tests/font-face-unquote.js"></script>
> 
> Please use http://trac.webkit.org/browser/trunk/Tools/Scripts/make-new-script-test instead so that you don't have a separate js file.

Done.
------- Comment #22 From 2012-03-01 00:13:38 PST -------
(From update of attachment 129648 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=129648&action=review

> Source/WebCore/ChangeLog:8
> +        Reviewed by NOBODY (OOPS!).

This line should appear before the description.

> LayoutTests/ChangeLog:9
> +        Reviewed by NOBODY (OOPS!).

Ditto.

> LayoutTests/editing/style/font-face-unquote-expected.txt:1
> +Test to make sure WebKit doesn't quote font face.

in face content attribute? I mean we do quote them in font-family property value, right?

> LayoutTests/editing/style/font-face-unquote.html:6
> +<head>
> +<meta charset="utf-8">
> +<script src="../../fast/js/resources/js-test-pre.js"></script>
> +</head>

You can just put the script element in body and there's no need for the meta.
------- Comment #23 From 2012-03-01 00:14:17 PST -------
Thanks for the fix!  Please also comment on the google bug I cc'ed you.

(This was a Gmail bug).
------- Comment #24 From 2012-03-01 00:55:15 PST -------
Created an attachment (id=129656) [details]
Patch for landing
------- Comment #25 From 2012-03-01 00:56:41 PST -------
(From update of attachment 129648 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=129648&action=review

Thanks!

>> Source/WebCore/ChangeLog:8
>> +        Reviewed by NOBODY (OOPS!).
> 
> This line should appear before the description.

Done.

>> LayoutTests/ChangeLog:9
>> +        Reviewed by NOBODY (OOPS!).
> 
> Ditto.

Done.

>> LayoutTests/editing/style/font-face-unquote-expected.txt:1
>> +Test to make sure WebKit doesn't quote font face.
> 
> in face content attribute? I mean we do quote them in font-family property value, right?

You are right. Changed the description.

>> LayoutTests/editing/style/font-face-unquote.html:6
>> +</head>
> 
> You can just put the script element in body and there's no need for the meta.

Done.
------- Comment #26 From 2012-03-01 03:46:01 PST -------
(From update of attachment 129656 [details])
Rejecting attachment 129656 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/11767379
------- Comment #27 From 2012-03-01 04:02:18 PST -------
Created an attachment (id=129678) [details]
Patch for landing
------- Comment #28 From 2012-03-01 04:44:05 PST -------
(From update of attachment 129678 [details])
Rejecting attachment 129678 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/11766434
------- Comment #29 From 2012-03-01 10:47:45 PST -------
(From update of attachment 129678 [details])
Try again.
------- Comment #30 From 2012-03-01 11:48:00 PST -------
(From update of attachment 129678 [details])
Clearing flags on attachment: 129678

Committed r109382: <http://trac.webkit.org/changeset/109382>
------- Comment #31 From 2012-03-01 11:48:07 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #32 From 2012-05-02 11:00:53 PST -------
*** Bug 71812 has been marked as a duplicate of this bug. ***