Bug 31076 – [Qt] Odd line wrapping in QtWebKit

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

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

Created attachment 42403 [details]
small example that reproduce the issue described.

[Pierre Rossi]

Created attachment 46369 [details]
suggested patch

[WebKit Review Bot]

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

[Benjamin Poulain]

Bug valid.

[Pierre Rossi]

Created attachment 46372 [details]
suggested patch (with no tabs in the ChangeLogs this time)

Thanks for the tabs, vi! ;)

[Simon Hausmann]

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.

[Pierre Rossi]

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.

[Pierre Rossi]

Created attachment 47401 [details]
oops !

with the additionnal test (better...)

[Eric Seidel (no email)]

Comment on attachment 47401 [details]
oops !

Seems the code needs a comment.  I don't understand why it's correct.

[Holger Freyther]

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.

[Holger Freyther]

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.

[Tor Arne Vestbø]

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

[Benjamin Poulain]

*** Bug 33793 has been marked as a duplicate of this bug. ***

[Simon Hausmann]

Pierre fixed this in Qt with commit e6ac173991223dbf3b1b6f7213550ebca4608cb6 , which can be found in the 4.6 branch and soon in 4.7 and master.

[Manuel Nickschas]

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?)...

[Benjamin Poulain]

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

[Benjamin Poulain]

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

[Manuel Nickschas]

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

[Benjamin Poulain]

*** Bug 32019 has been marked as a duplicate of this bug. ***