Bug 40483 – [Qt] The new Qt's documentation text layout is messed up with QtWebKit.

• Status: CLOSED
• Resolution: FIXED
• Component: Text

[Jocelyn Turcotte]

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

[Jocelyn Turcotte]

The documentation looks nice in firefox chrome and safari.

[Kenneth Rohde Christiansen]

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.

[Jocelyn Turcotte]

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.

[Csaba Osztrogonác]

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)

[ananth jasty]

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?

[ananth jasty]

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.

[ananth jasty]

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.

[ananth jasty]

Word-spacing is being inherited, but the link element render start position is not taking this (adjusted previous text ending) into account when rendering.

[ananth jasty]

RenderText.cpp is returning the wrong width value at times, possible by-product of an optimization patch.

[ananth jasty]

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.

[Jocelyn Turcotte]

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.

[ananth jasty]

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.

[ananth jasty]

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.

[WebKit Review Bot]

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.

[ananth jasty]

Created attachment 59140 [details]
Second patch, removes tabs

Second patch, removes tabs.

[Simon Hausmann]

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?

[ananth jasty]

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.

[Simon Hausmann]

(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!

[Simon Hausmann]

Comment on attachment 59140 [details]
Second patch, removes tabs

r=me

[Simon Hausmann]

Committed r61445: <http://trac.webkit.org/changeset/61445>

[Simon Hausmann]

Revision r61445 cherry-picked into qtwebkit-2.0 with commit 19de3d2848b715f937eb375a078672cc8e9b8185