Bug 100106 – [Qt] Make default typesettings configurable

• Status: RESOLVED
• Resolution: INVALID
• Component: WebKit Qt

[Allan Sandfeld Jensen]

WebCore has the ability to draw fonts with kerning and common ligatures, and we support that in the Qt backend. It is however disabled by default and only enabled when websites explicitly enable it (the settings can basically be auto, on, off, and we let auto == off).

Safari already allows this setting to be user-configurable, and I believe we should give users the same option. We might later also enable it by default for printing.

[Allan Sandfeld Jensen]

Created attachment 170114 [details]
Patch

[Allan Sandfeld Jensen]

Created attachment 170147 [details]
Patch

Made it configurable in WebKit2 by making the Mac-only setting generally available

[Build Bot]

Comment on attachment 170147 [details]
Patch

Attachment 170147 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14487876

[Allan Sandfeld Jensen]

Created attachment 170157 [details]
Patch

[Allan Sandfeld Jensen]

Created attachment 170176 [details]
Patch

Expose setting in QtTestBrowser for visually testing

[Pierre Rossi]

Comment on attachment 170176 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170176&action=review

> Source/WebKit/qt/Api/qwebsettings.cpp:492
> +    \value KerningAndLigaturesEnabledByDefault This setting enables font kerning and common ligatures everywhere they have not been explicitly disabled. This setting is disabled by default.

I think it's fine to expose this for people using QtWebKit to do things other than web browsers. As long as it is off by default it shouldn't hurt, but since it's gonna bring performance down quite dramatically when turned on, maybe there could be a mention of the implications setting this to true has.
The CSS way with "text-rendering: optimizeLegibility" seems more suited for people who have control over their content.
Maybe the docs should mention something about those trade-offs ?

[Allan Sandfeld Jensen]

(In reply to comment #6)
> (From update of attachment 170176 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170176&action=review
> 
> > Source/WebKit/qt/Api/qwebsettings.cpp:492
> > +    \value KerningAndLigaturesEnabledByDefault This setting enables font kerning and common ligatures everywhere they have not been explicitly disabled. This setting is disabled by default.
> 
> I think it's fine to expose this for people using QtWebKit to do things other than web browsers. As long as it is off by default it shouldn't hurt, but since it's gonna bring performance down quite dramatically when turned on, maybe there could be a mention of the implications setting this to true has.
> The CSS way with "text-rendering: optimizeLegibility" seems more suited for people who have control over their content.

Maybe, but I don't think we support custum UA or even user stylesheets, and this is controlling the default when text-rendering mode is in default.

> Maybe the docs should mention something about those trade-offs ?

Good idea, I will add that.

[Allan Sandfeld Jensen]

Created attachment 171201 [details]
Patch

Rebased and improved documentation

[Pierre Rossi]

Comment on attachment 171201 [details]
Patch

Looks good to me, but I'm not a reviewer ;)

[Jocelyn Turcotte]

Comment on attachment 171201 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171201&action=review

> Source/WebKit/qt/Api/qwebsettings.h:86
> +        KerningAndLigaturesEnabledByDefault

I would leave the "ByDefault" part out for consistency with other parameters. The doc you added explains it well.

> Source/WebKit2/Shared/WebProcessCreationParameters.cpp:40
> +    , shouldEnableKerningAndLigaturesByDefault(false)

This patch should normally either not touch the WebKit2 part, or do something with this bool and expose it in QWebPreferences.

> Source/WebKit2/UIProcess/WebContext.cpp:390
> +    parameters.shouldEnableKerningAndLigaturesByDefault = m_shouldEnableKerningAndLigaturesByDefault;

We should probably remove that part from WebContextMac.mm

[Allan Sandfeld Jensen]

(In reply to comment #10)
> (From update of attachment 171201 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171201&action=review
> 
> > Source/WebKit/qt/Api/qwebsettings.h:86
> > +        KerningAndLigaturesEnabledByDefault
> 
> I would leave the "ByDefault" part out for consistency with other parameters. The doc you added explains it well.
> 
The problem is that it only changes the default. In most cases that means it will enable it, but web-sites can still override the default. I originally used a different name, but changed it to match the name of the Mac settings. 

> > Source/WebKit2/UIProcess/WebContext.cpp:390
> > +    parameters.shouldEnableKerningAndLigaturesByDefault = m_shouldEnableKerningAndLigaturesByDefault;
> 
> We should probably remove that part from WebContextMac.mm

What do you mean? The part that sets this in WebContextMac, overrides this setting and is needed to apply the Mac settings.

Mac will not be able to apply the setting at runtime with this system, but it will continue to work as well as it already does, and they are free to migrate to using the platform-independent settings later.

You are probably right the WebKit2 settings are not ready yet.

[Allan Sandfeld Jensen]

Created attachment 171658 [details]
Patch

Moves the default typesetting features into page/Setting, and exports it qwebpreferences

[WebKit Review Bot]

Attachment 171658 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2200:  Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Allan Sandfeld Jensen]

Comment on attachment 171658 [details]
Patch

I am going to split this patch in two.

[Simon Hausmann]

In general I like the idea a lot.

While I understand the motivation of disabling kerning and ligatures for western languages by default in a browser, I'm personally a big fan of those features and I totally see that for example in an email reader application it is desirable to enable them by default (where the text layout performance is less critical). This is a decision that should be possible to control by the app developer.

[Allan Sandfeld Jensen]

(In reply to comment #15)
> In general I like the idea a lot.
> 
> While I understand the motivation of disabling kerning and ligatures for western languages by default in a browser, I'm personally a big fan of those features and I totally see that for example in an email reader application it is desirable to enable them by default (where the text layout performance is less critical). This is a decision that should be possible to control by the app developer.

We may need a separate API for it though. Apple are not going to allow us to make it a preference, because they have APIs using the text system without a Page and thus without Settings.

The question is then how we would do that in Qt. A global setting is essentially something set on QApplication level, but kerning is usually enabled by default in Qt, and a WebKit specific setting in QApplication would be a mess.

We may end up having to do it using user style-sheets though that is more error-prone.

[Allan Sandfeld Jensen]

Created attachment 174405 [details]
Patch

Changed the kerning setting to be global, since making it a real runtime setting has some opposition

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.