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
Product: WebKit
Classification: Unclassified
Component: HTML Editing
: 528+ (Nightly build)
: Unspecified Unspecified
: P1 Normal
Assigned To: Kenichi Ishibashi
: GoogleBug, InRadar, Regression
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-23 23:02 PST by Ryosuke Niwa
Modified: 2012-05-02 11:00 PDT (History)
9 users (show)

See Also:


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 Details | Formatted Diff | Diff
Patch (11.21 KB, patch)
2012-02-29 21:31 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch (4.65 KB, patch)
2012-02-29 23:53 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch (4.16 KB, patch)
2012-03-01 00:03 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch for landing (4.15 KB, patch)
2012-03-01 00:55 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch for landing (4.24 KB, patch)
2012-03-01 04:02 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 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 Aryeh Gregor 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 Ojan Vafai 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 Alexey Proskuryakov 2012-02-24 14:02:03 PST
Caused by changes in bug 64783?
Comment 4 Alexey Proskuryakov 2012-02-24 14:02:46 PST
<rdar://problem/10929517>
Comment 5 Ryosuke Niwa 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 Ryosuke Niwa 2012-02-25 16:10:40 PST
Created attachment 128888 [details]
Screenshot: old create-models page
Comment 7 Ryosuke Niwa 2012-02-25 16:11:00 PST
Created attachment 128889 [details]
Screenshot: Old merge-tests page
Comment 8 Ryosuke Niwa 2012-02-25 16:11:56 PST
Created attachment 128890 [details]
Screenshot: shiny(tm) new admin page
Comment 9 Ryosuke Niwa 2012-02-25 16:14:12 PST
Oops, sorry, these screenshots are for the bug 79585 :(
Comment 10 Ryosuke Niwa 2012-02-25 16:25:50 PST
Created attachment 128892 [details]
Needs a rubber stamp
Comment 11 Ryosuke Niwa 2012-02-25 16:26:45 PST
Comment on attachment 128892 [details]
Needs a rubber stamp

Ugh... wrong bug again :(
Comment 12 Kenichi Ishibashi 2012-02-27 18:33:50 PST
I'll take a look.
Comment 13 Kenichi Ishibashi 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 Kenichi Ishibashi 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 Kenichi Ishibashi 2012-02-29 21:31:14 PST
Created attachment 129629 [details]
Patch
Comment 16 Kenichi Ishibashi 2012-02-29 21:32:40 PST
(In reply to comment #15)
> Created an attachment (id=129629) [details]
> Patch

I'm not sure this is really a regression, though.
Comment 17 Ryosuke Niwa 2012-02-29 21:39:39 PST
Comment on attachment 129629 [details]
Patch

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 Kenichi Ishibashi 2012-02-29 23:53:08 PST
Created attachment 129645 [details]
Patch
Comment 19 Ryosuke Niwa 2012-02-29 23:56:02 PST
Comment on attachment 129645 [details]
Patch

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 Kenichi Ishibashi 2012-03-01 00:03:26 PST
Created attachment 129648 [details]
Patch
Comment 21 Kenichi Ishibashi 2012-03-01 00:04:05 PST
Comment on attachment 129645 [details]
Patch

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 Ryosuke Niwa 2012-03-01 00:13:38 PST
Comment on attachment 129648 [details]
Patch

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 Ryosuke Niwa 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 Kenichi Ishibashi 2012-03-01 00:55:15 PST
Created attachment 129656 [details]
Patch for landing
Comment 25 Kenichi Ishibashi 2012-03-01 00:56:41 PST
Comment on attachment 129648 [details]
Patch

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 WebKit Review Bot 2012-03-01 03:46:01 PST
Comment on attachment 129656 [details]
Patch for landing

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 Kenichi Ishibashi 2012-03-01 04:02:18 PST
Created attachment 129678 [details]
Patch for landing
Comment 28 WebKit Review Bot 2012-03-01 04:44:05 PST
Comment on attachment 129678 [details]
Patch for landing

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 Tony Chang 2012-03-01 10:47:45 PST
Comment on attachment 129678 [details]
Patch for landing

Try again.
Comment 30 WebKit Review Bot 2012-03-01 11:48:00 PST
Comment on attachment 129678 [details]
Patch for landing

Clearing flags on attachment: 129678

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