REGRESSION: Outlook 2007 doesn't display fonts correctly on emails composed by WebKit

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

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.

[Aryeh Gregor]

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

[Ojan Vafai]

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?

[Alexey Proskuryakov]

Caused by changes in bug 64783?

[Alexey Proskuryakov]

<rdar://problem/10929517>

[Ryosuke Niwa]

(In reply to comment #3)
> Caused by changes in bug 64783?

The bug 64783 is fixing the CSS parser though, not the CSS serializer.

[Ryosuke Niwa]

Created attachment 128888 [details]
Screenshot: old create-models page

[Ryosuke Niwa]

Created attachment 128889 [details]
Screenshot: Old merge-tests page

[Ryosuke Niwa]

Created attachment 128890 [details]
Screenshot: shiny(tm) new admin page

[Ryosuke Niwa]

Oops, sorry, these screenshots are for the bug 79585 :(

[Ryosuke Niwa]

Created attachment 128892 [details]
Needs a rubber stamp

[Ryosuke Niwa]

Comment on attachment 128892 [details]
Needs a rubber stamp

Ugh... wrong bug again :(

[Kenichi Ishibashi]

I'll take a look.

[Kenichi Ishibashi]

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.

[Kenichi Ishibashi]

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

[Kenichi Ishibashi]

Created attachment 129629 [details]
Patch

[Kenichi Ishibashi]

(In reply to comment #15)
> Created an attachment (id=129629) [details]
> Patch

I'm not sure this is really a regression, though.

[Ryosuke Niwa]

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.

[Kenichi Ishibashi]

Created attachment 129645 [details]
Patch

[Ryosuke Niwa]

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.

[Kenichi Ishibashi]

Created attachment 129648 [details]
Patch

[Kenichi Ishibashi]

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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Thanks for the fix!  Please also comment on the google bug I cc'ed you.

(This was a Gmail bug).

[Kenichi Ishibashi]

Created attachment 129656 [details]
Patch for landing

[Kenichi Ishibashi]

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.

[WebKit Review Bot]

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

[Kenichi Ishibashi]

Created attachment 129678 [details]
Patch for landing

[WebKit Review Bot]

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

[Tony Chang]

Comment on attachment 129678 [details]
Patch for landing

Try again.

[WebKit Review Bot]

Comment on attachment 129678 [details]
Patch for landing

Clearing flags on attachment: 129678

Committed r109382: <http://trac.webkit.org/changeset/109382>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

*** Bug 71812 has been marked as a duplicate of this bug. ***