Bug 31076

Summary: [Qt] Odd line wrapping in QtWebKit
Product: WebKit Reporter: qt-info
Component: Layout and RenderingAssignee: QtWebKit Unassigned <webkit-qt-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: acheron, benjamin, eekaikko, eric, hausmann, laszlo.gombos, mitz, ossy, sputnick, webkit.review.bot, zecke
Priority: P2 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
small example that reproduce the issue described.
none
suggested patch
none
suggested patch (with no tabs in the ChangeLogs this time)
hausmann: review-
Added a Layout test
none
oops ! zecke: review-

Description qt-info 2009-11-03 11:55:51 PST
(See the attached example to reproduce the problem.)

It seems unicode characters for special quotes (“” ‘’ « ») combined with <span> are considered as a breakable character, leading to some unexpected linewrapping. It can also break right after accented characters, basically splitting the line in the middle of a word.


Steps to reproduce:

1) run the attached example

2) resize the window to observe text wrapping in the QWebView

3) use different characters in the word ending, like the quotes mentioned above.


Expected behaviour:
the whole word is wrapped when needed (e.g "“foobar”." or "foobarée.")

Actual behaviour:
only the last characters are wrapped at first (e.g "”." or "e." )


-Tested against QtWebKit from trunk (gitorious clone, not the svn).
-This same html file works fine in Safari, so it looks like a Qt specific issue.
-even when <span> is not used, the dot ('.') is sometimes sent to the line alone, which seems odd as well.
Comment 1 qt-info 2009-11-03 11:57:30 PST
Created attachment 42403 [details]
small example that reproduce the issue described.
Comment 2 Pierre Rossi 2010-01-12 06:37:05 PST
Created attachment 46369 [details]
suggested patch
Comment 3 WebKit Review Bot 2010-01-12 06:40:17 PST
Attachment 46369 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 3
Comment 4 Benjamin Poulain 2010-01-12 06:44:22 PST
Bug valid.
Comment 5 Pierre Rossi 2010-01-12 06:47:05 PST
Created attachment 46372 [details]
suggested patch (with no tabs in the ChangeLogs this time)

Thanks for the tabs, vi! ;)
Comment 6 Simon Hausmann 2010-01-12 13:04:33 PST
Comment on attachment 46372 [details]
suggested patch (with no tabs in the ChangeLogs this time)

I'm a bit sceptical about this fix. Please make me less sceptical by turning your manual test into a layout test :)

The updated layout test only changes its metrics, but the actual line break doesn't change. So it doesn't really test this bug AFAICS.
Comment 7 Pierre Rossi 2010-01-26 04:38:19 PST
Created attachment 47400 [details]
Added a Layout test

Does that help in making you less sceptical ? :) Feel free to ping me on IRC when reviewing it, there might very well be a better solution, this I'm aware of.
Comment 8 Pierre Rossi 2010-01-26 04:48:02 PST
Created attachment 47401 [details]
oops !

with the additionnal test (better...)
Comment 9 Eric Seidel (no email) 2010-02-01 15:22:53 PST
Comment on attachment 47401 [details]
oops !

Seems the code needs a comment.  I don't understand why it's correct.
Comment 10 Holger Freyther 2010-02-01 17:44:46 PST
Besides a comment. What about doing the following as well:

We load the same wikipedia article in different languages (English, German, Hindi, Japanese, traditional Chinese, simplified Chinese, Korean, Arabian) and we instrument any build using ICU and wil print the Strings used and the result of ICU. Then we can compare how ICU and QTextBoundary Finder do things differently?

While being in Korea two local engineers highlighted that the Korean/Japanese text coming by in Font::floatWidthForComplexText is most of the time a single charachter. So far I have no idea if that is correct or not but being able to compare it to ICU would give a strong indication.
Comment 11 Holger Freyther 2010-02-01 23:25:30 PST
Comment on attachment 47401 [details]
oops !

Based on Eric's comment and my own feeling, I think we really need to compare this to ICU and understand the situation better.
Comment 12 Tor Arne Vestbø 2010-03-05 09:39:30 PST
Please follow the QtWebKit bug reporting guidelines when reporting bugs.

See http://trac.webkit.org/wiki/QtWebKitBugs

Specifically:

  - The 'QtWebKit' component should be used for bugs/features in the public QtWebKit API layer, not to signify that the bug is specific to the Qt port of WebKit

    http://trac.webkit.org/wiki/QtWebKitBugs#Component
Comment 13 Benjamin Poulain 2010-03-06 08:59:16 PST
*** Bug 33793 has been marked as a duplicate of this bug. ***
Comment 14 Simon Hausmann 2010-06-13 09:09:19 PDT
Pierre fixed this in Qt with commit e6ac173991223dbf3b1b6f7213550ebca4608cb6 , which can be found in the 4.6 branch and soon in 4.7 and master.
Comment 15 Manuel Nickschas 2010-06-22 01:01:13 PDT
Simon,

this commit in Qt changes behavior of QTextBoundaryFinder and breaks applications relying on the old behavior. Even if the old behavior was inconsistent, I'm not convinced Qt should introduce a behavior change like this, especially not in a patch release for 4.6 (and preferably also not for 4.7).

Can't this be made opt-in for apps and classes that need it? Otherwise, apps using QTextBoundaryFinder will have to be updated with a workaround for Qt >= $someversion (would it be 4.6.4 then?)...
Comment 16 Benjamin Poulain 2010-06-22 02:33:27 PDT
(In reply to comment #15)
> this commit in Qt changes behavior of QTextBoundaryFinder and breaks applications relying on the old behavior. Even if the old behavior was inconsistent, I'm not convinced Qt should introduce a behavior change like this, especially not in a patch release for 4.6 (and preferably also not for 4.7).
> 
> Can't this be made opt-in for apps and classes that need it? Otherwise, apps using QTextBoundaryFinder will have to be updated with a workaround for Qt >= $someversion (would it be 4.6.4 then?)...

In some languages (French for example) text sometimes looks like shit without this patch. I definitely consider that as an important bug of QtWebKit.

I also don't like the idea of an opt-in to have broken layout of text.
Comment 17 Benjamin Poulain 2010-06-22 03:10:41 PDT
(In reply to comment #16)
> In some languages (French for example) text sometimes looks like shit without this patch. I definitely consider that as an important bug of QtWebKit.
> 
> I also don't like the idea of an opt-in to have broken layout of text.

My bad, I just learned there might be a bug in the new implementation. I was talking about the word wrapping issue as described in this bug report.
Comment 18 Manuel Nickschas 2010-06-22 10:08:39 PDT
(In reply to comment #17)
> > In some languages (French for example) text sometimes looks like shit without this patch. I definitely consider that as an important bug of QtWebKit.
> > 
> > I also don't like the idea of an opt-in to have broken layout of text.
> 
> My bad, I just learned there might be a bug in the new implementation. I was talking about the word wrapping issue as described in this bug report.

Yeah, the problem is that the way Qt fixed this breaks existing applications using the old behavior of the text boundary finder. Probably they could just add +1 within QtWebkit instead and leave QTextBoundaryFinder alone...
Comment 19 Benjamin Poulain 2011-03-06 01:56:21 PST
*** Bug 32019 has been marked as a duplicate of this bug. ***