Bug 40483

The documentation looks nice in firefox chrome and safari.

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.

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.

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)

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?

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.

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.

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

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

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.

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.

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.

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.

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.

Created attachment 59140 [details] Second patch, removes tabs Second patch, removes tabs.

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?

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.

(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 on attachment 59140 [details] Second patch, removes tabs r=me

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

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