Bug 40483 - [Qt] The new Qt's documentation text layout is messed up with QtWebKit.
Summary: [Qt] The new Qt's documentation text layout is messed up with QtWebKit.
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Major
Assignee: ananth jasty
URL: http://doc.qt.nokia.com/4.7-snapshot/...
Keywords: Qt, QtTriaged
Depends on:
Blocks: 35784
  Show dependency treegraph
 
Reported: 2010-06-11 11:11 PDT by Jocelyn Turcotte
Modified: 2010-06-18 15:13 PDT (History)
7 users (show)

See Also:


Attachments
Screenshot (89.39 KB, image/png)
2010-06-14 01:45 PDT, Jocelyn Turcotte
no flags Details
Screenshots and rendertrees of various settings. (deleted)
2010-06-17 11:23 PDT, ananth jasty
no flags Details
Screenshot of partial fix (78.23 KB, image/jpeg)
2010-06-17 15:23 PDT, ananth jasty
no flags Details
Patch to fix wordspacing in text rendering optimization. (1.29 KB, patch)
2010-06-18 11:17 PDT, ananth jasty
no flags Details | Formatted Diff | Diff
Second patch, removes tabs (1.31 KB, patch)
2010-06-18 11:23 PDT, ananth jasty
hausmann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jocelyn Turcotte 2010-06-11 11:11:03 PDT
The function signature const, & and * overlap with the following text.

This is even more annoying since QtWebKit is used in the Qt assistant.

See http://doc.qt.nokia.com/4.7-snapshot/qwidget.html#grabShortcut
Comment 1 Jocelyn Turcotte 2010-06-11 11:11:33 PDT
The documentation looks nice in firefox chrome and safari.
Comment 2 Kenneth Rohde Christiansen 2010-06-11 18:53:40 PDT
Is this due to use using larger font sizes by default that Chrome etc? or do you have any clue? Some screenshots would be nice.
Comment 3 Jocelyn Turcotte 2010-06-14 01:45:12 PDT
Created attachment 58629 [details]
Screenshot

The problem seems to be related to the width of some words.
For example, trying to select individual letters of the "const" keyword I can only select the 3 first letters and half of the 4th one.

I could reproduce the problem on Windows and Linux.
Comment 4 Csaba Osztrogonác 2010-06-14 13:09:13 PDT
I can reproduce this bug on the Qt Linux bot too.

(It might be in relation with https://bugs.webkit.org/show_bug.cgi?id=40584)
Comment 5 ananth jasty 2010-06-15 15:30:41 PDT
Is fast padding enabled? This is not reprod in chrome or safari (though there is a small distortion).

QtWeb 3.3 also has a distortion, with the t at the end of const registering as a very small select area (roughly 3 px).

The rendertree seems to be recalculating the justification, and ending up with a smaller answer than it should, which speaks to the kerning/width weighting.

Perhaps a later factor in the style is overriding the font selection/sizing?
Comment 6 ananth jasty 2010-06-17 11:23:32 PDT
Created attachment 59019 [details]
Screenshots and rendertrees of various settings.

http://vpn.x3ns.net/qwidget.html -- my test page

Tried again without h3.fn word-spacing (was originally 3px), not reproducible.

Essentially, while 90% of the time the word-spacing is correct, when rendering the "const", the word-spacing is -3 instead of 3. Included are render-tree dumps of the 3 cases, renderTree = reported bug, renderTre3e = without word-spacing, renderTree4 = -3 word-spacing.

Also included are the screenshots from +3 and -3 word-spacing.

Next is to determine where the word-spacing is being inverted.
Comment 7 ananth jasty 2010-06-17 12:15:07 PDT
Quick follow up, http://vpn.x3ns.net/simple.html

The <a> elements are either not inheriting, or not respecting the word-space properties of their parents.

Chrome works properly.

Checking if this is in the css or rendering.
Comment 8 ananth jasty 2010-06-17 12:56:01 PDT
Word-spacing is being inherited, but the link element render start position is not taking this (adjusted previous text ending) into account when rendering.
Comment 9 ananth jasty 2010-06-17 14:14:02 PDT
RenderText.cpp is returning the wrong width value at times, possible by-product of an optimization patch.
Comment 10 ananth jasty 2010-06-17 15:23:30 PDT
Created attachment 59041 [details]
Screenshot of partial fix

Partially fixed problem, screenshot attached. Word spacing was only being added on a per-RenderText basis, not a per-word basis apparently. Need to fix the final issue of RenderText boundaries.
Comment 11 Jocelyn Turcotte 2010-06-18 01:55:54 PDT
Bisection pointed to this commit as the first one we could see the problem.

http://trac.webkit.org/changeset/55406

The "- m_wordSpacing" part seems suspicious.
Comment 12 ananth jasty 2010-06-18 10:07:34 PDT
Yeah that seems to have gotten it. I think they were trying to optimize around the whitespace case, but I still don't see why they removed the m_wordSpacing. A simple reversion should fix this for the next changeset.
Comment 13 ananth jasty 2010-06-18 11:17:52 PDT
Created attachment 59139 [details]
Patch to fix wordspacing in text rendering optimization.

Review requested. Removed m_wordSpacing compensation from FontQt.cpp when calculating whitespace width.

Tested with webkit-tests and acid3, as well as original test case and subsequent reduction.
Comment 14 WebKit Review Bot 2010-06-18 11:20:58 PDT
Attachment 59139 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 3 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 ananth jasty 2010-06-18 11:23:44 PDT
Created attachment 59140 [details]
Second patch, removes tabs

Second patch, removes tabs.
Comment 16 Simon Hausmann 2010-06-18 14:25:06 PDT
Comment on attachment 59140 [details]
Second patch, removes tabs


>      if (run.length() == 1 && treatAsSpace(run[0]))
> -        return QFontMetrics(font()).width(run[0]) - m_wordSpacing + run.padding();
> +        return QFontMetrics(font()).width(run[0]) + run.padding();

I don't quite understand what's wrong with subtracting the word spacing in the original code?

If you leave out this optimization altogether, then QFontMetrics::width() will be called with the same string, and AFAICS m_wordSpacing will also be subtracted.

Could the difference between the code paths perhaps be space normalization?
Comment 17 ananth jasty 2010-06-18 14:56:41 PDT
Taking out the opt completely fixes the issue also. I believe the m_wordSpacing is there because of an assumption that the single space case would be already padded by the FontMetrics.

This appears not to be the case, leading to 2 different results for width calculation, 1 for rendering, 1 for offset/layout.

At least this is my take on the problem.

I do not know why FontMetrics would be concerned about m_wordSpacing if it were merely trying to render a single space character, but I could be mistaken.
Comment 18 Simon Hausmann 2010-06-18 15:06:22 PDT
(In reply to comment #17)
> Taking out the opt completely fixes the issue also. I believe the m_wordSpacing is there because of an assumption that the single space case would be already padded by the FontMetrics.
> 
> This appears not to be the case, leading to 2 different results for width calculation, 1 for rendering, 1 for offset/layout.
> 
> At least this is my take on the problem.
> 
> I do not know why FontMetrics would be concerned about m_wordSpacing if it were merely trying to render a single space character, but I could be mistaken.

Ah yes, you're correct!

In the single-char case we call the QFontMetrics::width() overload that takes a character.

In the other case we call the QFontMetrics::width() overload that takes a _string_ and that's considered possibly a word, etc. Therefore QFont's wordSpacing/letterSpacing/etc. takes effect and therefore needs to be subtracted afterwards.

In other words: Your patch is correct!
Comment 19 Simon Hausmann 2010-06-18 15:06:51 PDT
Comment on attachment 59140 [details]
Second patch, removes tabs

r=me
Comment 20 Simon Hausmann 2010-06-18 15:11:43 PDT
Committed r61445: <http://trac.webkit.org/changeset/61445>
Comment 21 Simon Hausmann 2010-06-18 15:13:11 PDT
Revision r61445 cherry-picked into qtwebkit-2.0 with commit 19de3d2848b715f937eb375a078672cc8e9b8185