Created attachment 123786 [details] testcase demonstrating overlapping text caused by soft hyphen Using a browser with QtWebKit (tested: Konqueror, Rekonq, Arora, QtTestBrowser), this html is not displayed correctly when the browser window is narrow: <table border="1"> <tr> <td>aaaaaaaaaaaaaa­bbbbbbbbbbbbbb</td> <td>something</td> </tr> </table> Actual Results: 1) WebKit does not break the line. 2) Worse, it apparently thinks it has broken the line, allowing that "something" to the right to overlap with the "bbbbbbbbbbbbbb". 3) The overlapping text is unreadable. Expected Results: The soft hyphen should appear as a hyphen at the end of the "aaaaaaaaaaaaaa", and "bbbbbbbbbbbbbb" should move to the next line (out of the way for that "something" to the right). Non-Webkit browsers work fine. I have not tested non-QtWebKit WebKit browsers. The bug was first submitted here: https://bugs.kde.org/show_bug.cgi?id=289182 Version numbers (How to infer the version of WebKit?): libQtWebKit4 4.7.4+2.2.0-2.1.2 kwebkitpart 1.2.0git20111019-1.2
This works for me in Safari on Mac.
Works with QtWebKit from trunks (using MiniBrowser --desktop). Confirmed broken with QtWebKit from Qt 4.7.4.
This bug is related to bug 27593. The root of the issue is the same: QTextBoundaryFinder::QTextBoundaryFinder() does not respect soft hyphen as a line break. The manifestation in this case is that the line inside the table cell never gets broken at the soft hyphen, causing a collision. This differs from the problem cited in bug 27593, namely that the hyphen is never drawn but lines break properly. Having just run that test, I'd disagree that the lines break properly at all: the lines simply continue to break on word boundaries, not respecting soft hyphen at all.
The bug also appears fixed in Qt 4.8.0 To summarize my tests (all on Linux): QtWebkit from Qt 4.7.4 : Bug confirmed QtWebkit from 4.8.0: Bug fixed (all soft hyphens works as expected) QtWebkit from trunk: Bug fixed (all soft hyphens works as expected) Should this bug be closed as fixed, or does it still occur in recent Qt versions on some platforms? Btw. The same test results counts for bug 27593
Created attachment 132152 [details] Patch
Note that this patch appears to fix bug 27593 as well. I hesitate to mark as duplicate because the symptoms differ, but the root cause is the same.
Also note that I am using the latest available 4.8 SDK from Nokia (http://qt.nokia.com/downloads ). Bug 27593 (recently closed) mentions that nokia has fixed the soft hyphen problem in their implementation of QTextBoundaryFinder, but this fix does not appear to be widely released at this point. The patch provided here is therefore an interim solution until Nokia releases and updated SDK.
Comment on attachment 132152 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132152&action=review Is the issue reported in Qt? > Source/WebCore/platform/text/qt/TextBreakIteratorQt.cpp:129 > + int softHyphenPos = bi->string().indexOf(QChar(SOFT_HYPHEN), currPos); this QChar could be static. > Source/WebCore/platform/text/qt/TextBreakIteratorQt.cpp:161 > + // Note that this code will essentially no-op in the case of QTextBoundaryFinder handling soft hyphen. no-op but the indexOf will still be run right?
(In reply to comment #8) > (From update of attachment 132152 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=132152&action=review > > Is the issue reported in Qt? Yes. In fact, the related bug (bug 27593) mentions that this has been fixed in QT (presumably a version greater than the released 4.8 SDK). > > > Source/WebCore/platform/text/qt/TextBreakIteratorQt.cpp:129 > > + int softHyphenPos = bi->string().indexOf(QChar(SOFT_HYPHEN), currPos); > > this QChar could be static. I was going for compactness since this is somewhat of a corner case. Are you saying I should declare a static instance of a QChar and pass it in instead? > > > Source/WebCore/platform/text/qt/TextBreakIteratorQt.cpp:161 > > + // Note that this code will essentially no-op in the case of QTextBoundaryFinder handling soft hyphen. > > no-op but the indexOf will still be run right? Yes. By no-op I meant that the if condition would never be met if/when QTextBoundaryFinder handles soft hyphen correctly.
(In reply to comment #9) > (In reply to comment #8) > > (From update of attachment 132152 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=132152&action=review > > > > Is the issue reported in Qt? > Yes. In fact, the related bug (bug 27593) mentions that this has been fixed in QT (presumably a version greater than the released 4.8 SDK). > > > > > Source/WebCore/platform/text/qt/TextBreakIteratorQt.cpp:129 > > > + int softHyphenPos = bi->string().indexOf(QChar(SOFT_HYPHEN), currPos); > > > > this QChar could be static. > I was going for compactness since this is somewhat of a corner case. Are you saying I should declare a static instance of a QChar and pass it in instead? > > > > > Source/WebCore/platform/text/qt/TextBreakIteratorQt.cpp:161 > > > + // Note that this code will essentially no-op in the case of QTextBoundaryFinder handling soft hyphen. > > > > no-op but the indexOf will still be run right? > Yes. By no-op I meant that the if condition would never be met if/when QTextBoundaryFinder handles soft hyphen correctly. Provided it is fixed in a patch release of Qt, 4.8.1 I don't think we should put the workaround. Though I'm in favor of adding the test for regression testing. Pierre?
(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > (From update of attachment 132152 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=132152&action=review > > > > > > Is the issue reported in Qt? > > Yes. In fact, the related bug (bug 27593) mentions that this has been fixed in QT (presumably a version greater than the released 4.8 SDK). > > > > > > > Source/WebCore/platform/text/qt/TextBreakIteratorQt.cpp:129 > > > > + int softHyphenPos = bi->string().indexOf(QChar(SOFT_HYPHEN), currPos); > > > > > > this QChar could be static. > > I was going for compactness since this is somewhat of a corner case. Are you saying I should declare a static instance of a QChar and pass it in instead? > > > > > > > Source/WebCore/platform/text/qt/TextBreakIteratorQt.cpp:161 > > > > + // Note that this code will essentially no-op in the case of QTextBoundaryFinder handling soft hyphen. > > > > > > no-op but the indexOf will still be run right? > > Yes. By no-op I meant that the if condition would never be met if/when QTextBoundaryFinder handles soft hyphen correctly. > > Provided it is fixed in a patch release of Qt, 4.8.1 I don't think we should put the workaround. Though I'm in favor of adding the test for regression testing. > > Pierre? I had a quick look at the changes that had gone into QTextBoundaryFinder in Qt5 and I'm not sure this has even been fixed. I'm starting to suspect it's only the switch to ICU that made the bug go away for Allan and I. So while I agree with Alexis the test would be nice to have in WebKit, I'm not sure if the proper fix doesn't belong in QTextBoundaryFinder, so that others can benefit from it.
(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #9) > > > (In reply to comment #8) > > > > (From update of attachment 132152 [details] [details] [details] [details]) > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=132152&action=review > > > > > > > > Is the issue reported in Qt? > > > Yes. In fact, the related bug (bug 27593) mentions that this has been fixed in QT (presumably a version greater than the released 4.8 SDK). > > > > > > > > > Source/WebCore/platform/text/qt/TextBreakIteratorQt.cpp:129 > > > > > + int softHyphenPos = bi->string().indexOf(QChar(SOFT_HYPHEN), currPos); > > > > > > > > this QChar could be static. > > > I was going for compactness since this is somewhat of a corner case. Are you saying I should declare a static instance of a QChar and pass it in instead? > > > > > > > > > Source/WebCore/platform/text/qt/TextBreakIteratorQt.cpp:161 > > > > > + // Note that this code will essentially no-op in the case of QTextBoundaryFinder handling soft hyphen. > > > > > > > > no-op but the indexOf will still be run right? > > > Yes. By no-op I meant that the if condition would never be met if/when QTextBoundaryFinder handles soft hyphen correctly. > > > > Provided it is fixed in a patch release of Qt, 4.8.1 I don't think we should put the workaround. Though I'm in favor of adding the test for regression testing. > > > > Pierre? > > I had a quick look at the changes that had gone into QTextBoundaryFinder in Qt5 and I'm not sure this has even been fixed. I'm starting to suspect it's only the switch to ICU that made the bug go away for Allan and I. So while I agree with Alexis the test would be nice to have in WebKit, I'm not sure if the proper fix doesn't belong in QTextBoundaryFinder, so that others can benefit from it. I think we are all in heated agreement that this should be fixed in QTextBoundaryFinder :-) I think the fact that it does not appear to be fixed currently in QT presents a good argument for landing this patch: 1. It solves the issue in webkit, in a safe way. 2. The fix is easy to back out later when QT is fixed. 3. The test case is valuable. Is there any reason this patch shouldn't be landed at this point?
(In reply to comment #12) > (In reply to comment #11) > > (In reply to comment #10) > > > (In reply to comment #9) > > > > (In reply to comment #8) > > > > > (From update of attachment 132152 [details] [details] [details] [details] [details]) > > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=132152&action=review > > > > > > > > > > Is the issue reported in Qt? > > > > Yes. In fact, the related bug (bug 27593) mentions that this has been fixed in QT (presumably a version greater than the released 4.8 SDK). > > > > > > > > > > > Source/WebCore/platform/text/qt/TextBreakIteratorQt.cpp:129 > > > > > > + int softHyphenPos = bi->string().indexOf(QChar(SOFT_HYPHEN), currPos); > > > > > > > > > > this QChar could be static. > > > > I was going for compactness since this is somewhat of a corner case. Are you saying I should declare a static instance of a QChar and pass it in instead? > > > > > > > > > > > Source/WebCore/platform/text/qt/TextBreakIteratorQt.cpp:161 > > > > > > + // Note that this code will essentially no-op in the case of QTextBoundaryFinder handling soft hyphen. > > > > > > > > > > no-op but the indexOf will still be run right? > > > > Yes. By no-op I meant that the if condition would never be met if/when QTextBoundaryFinder handles soft hyphen correctly. > > > > > > Provided it is fixed in a patch release of Qt, 4.8.1 I don't think we should put the workaround. Though I'm in favor of adding the test for regression testing. > > > > > > Pierre? > > > > I had a quick look at the changes that had gone into QTextBoundaryFinder in Qt5 and I'm not sure this has even been fixed. I'm starting to suspect it's only the switch to ICU that made the bug go away for Allan and I. So while I agree with Alexis the test would be nice to have in WebKit, I'm not sure if the proper fix doesn't belong in QTextBoundaryFinder, so that others can benefit from it. > > I think we are all in heated agreement that this should be fixed in QTextBoundaryFinder :-) I think the fact that it does not appear to be fixed currently in QT presents a good argument for landing this patch: > 1. It solves the issue in webkit, in a safe way. > 2. The fix is easy to back out later when QT is fixed. > 3. The test case is valuable. > > Is there any reason this patch shouldn't be landed at this point? Yes we add a workaround for a little amount of user. Unreleased QtWebKit version (trunk) building against Qt 4.8.0 which is unsupported/no QA'ed combination. We recommend QtWebKit 2.2 for Qt 4.8.0. Maybe the bug is in QtWebKit 2.2 also but I think it would me more valuable to fix Qt rather than a workaround of the bug in here. At the end it will benefit users of Qt who are not using QtWebKit. You mentioned on IRC that the bug is maybe fix in Qt 4.8 branch then cool no need to add workaround. What you can re-upload is just the test case, that is good to have.
Added bug 81964 for uploading only the test case. Shall we close this bug then?
Comment on attachment 132152 [details] Patch Clearing review flag as per comments.
How to reassign to the Qt people? Isn't this supposed to be the right bugzilla for even Qt specific bugs in QtWebKit? Please advise me. A verified bug like this should stay open _somewhere_ until it gets fixed. I wanted to make sure this was reported to Qt. According to [1], bugs in QtWebKit should be reported to [2], even bugs that are specific to Qt. I could not find it there, but its «Template shortcut» [3] for reporting new bugs redirects to this bugzilla (https://bugs.webkit.org/enter_bug.cgi?…). Also, I found the equivalent of bug 27593 on qt-project.org [4], but the message was clear: «please move this issue to bugs.webkit.org» [1] https://qt-project.org/wiki/ReportingBugsInQt [2] http://trac.webkit.org/wiki/QtWebKitBugs [3] http://webkit.org/new-qtwebkit-bug [4] https://bugreports.qt-project.org/browse/QTWEBKIT-58
(In reply to comment #16) > How to reassign to the Qt people? > > Isn't this supposed to be the right bugzilla for even Qt specific bugs in QtWebKit? Please advise me. A verified bug like this should stay open _somewhere_ until it gets fixed. > > I wanted to make sure this was reported to Qt. According to [1], bugs in QtWebKit should be reported to [2], even bugs that are specific to Qt. I could not find it there, but its «Template shortcut» [3] for reporting new bugs redirects to this bugzilla (https://bugs.webkit.org/enter_bug.cgi?…). > > Also, I found the equivalent of bug 27593 on qt-project.org [4], but the message was clear: «please move this issue to bugs.webkit.org» > > [1] https://qt-project.org/wiki/ReportingBugsInQt > [2] http://trac.webkit.org/wiki/QtWebKitBugs > [3] http://webkit.org/new-qtwebkit-bug > [4] https://bugreports.qt-project.org/browse/QTWEBKIT-58 Just FYI, the patch uploaded for this bug is still valid (fixes the issue in a safe way in webkit -- will be transparent if/when QT fixes it). If there is consensus, I can attempt to get this reviewed again and landed.
(In reply to comment #17) > Just FYI, the patch uploaded for this bug is still valid (fixes the issue > in a safe way in webkit -- will be transparent if/when QT fixes it). If > there is consensus, I can attempt to get this reviewed again and landed. If your workaround is harmless, I'm for it, but I'm not competent. As long as the real bug is in QTextBoundaryFinder, we must let the Qt people know. Ideally, someone competent should write a bugreport on the misbehavior of QTextBoundaryFinder. Alternatively, we could hand this report over to them. The latter was what I meant by «reassign to the Qt people». If the problem is reported over at Qt, I'm happy with closing the bug here. Happy 17th May ;)
(In reply to comment #18) > (In reply to comment #17) > > Just FYI, the patch uploaded for this bug is still valid (fixes the issue > > in a safe way in webkit -- will be transparent if/when QT fixes it). If > > there is consensus, I can attempt to get this reviewed again and landed. > > If your workaround is harmless, I'm for it, but I'm not competent. > > As long as the real bug is in QTextBoundaryFinder, we must let the Qt people know. Ideally, someone competent should write a bugreport on the misbehavior of QTextBoundaryFinder. Alternatively, we could hand this report over to them. The latter was what I meant by «reassign to the Qt people». > > If the problem is reported over at Qt, I'm happy with closing the bug here. > > Happy 17th May ;) The problem in QTextBoundaryFinder is now hidden by the fact that we switched to ICU, so this is really not a WebKit bug per se. Here's the fix for Qt5: https://codereview.qt-project.org/#change,26469 And the request for the Qt 4 repo: https://codereview.qt-project.org/#change,26470 As for bug reporting, bugs with the [Qt] keyword are specific to QtWebKit, and people hacking on the Qt port usually also contribute to Qt as a direct consequence. Happy 17th of May indeed :)
Any progress on landing the Qt4.8 fix?
(In reply to comment #20) > Any progress on landing the Qt4.8 fix? (In reply to comment #20) > Any progress on landing the Qt4.8 fix? The problem is that it's technically a behavior change, and it's a bit against the policy to land behavior changes in patch releases in Qt, unless we're fixing a critical bug that way. So the fact that we're stuck with 4.8 makes it tricky... Aren't we gonna use ICU by default for QtWebKit 2.3 though ? That should fix it for most people.
(In reply to comment #21) > (In reply to comment #20) > > Any progress on landing the Qt4.8 fix? > > (In reply to comment #20) > > Any progress on landing the Qt4.8 fix? > > The problem is that it's technically a behavior change, and it's a bit against the policy to land behavior changes in patch releases in Qt, unless we're fixing a critical bug that way. So the fact that we're stuck with 4.8 makes it tricky... > Aren't we gonna use ICU by default for QtWebKit 2.3 though ? That should fix it for most people. I agree, I think the dependency to ICU is the best way of solving this.
(In reply to comment #22) > (In reply to comment #21) > > (In reply to comment #20) > > > Any progress on landing the Qt4.8 fix? > > > > (In reply to comment #20) > > > Any progress on landing the Qt4.8 fix? > > > > The problem is that it's technically a behavior change, and it's a bit against the policy to land behavior changes in patch releases in Qt, unless we're fixing a critical bug that way. So the fact that we're stuck with 4.8 makes it tricky... > > Aren't we gonna use ICU by default for QtWebKit 2.3 though ? That should fix it for most people. > > I agree, I think the dependency to ICU is the best way of solving this. So far we are not using ICU in QtWebKit 2.3, but it might be a better solution, though I feel sad for not fixing bugs in Qt.
(In reply to comment #23) > (In reply to comment #22) > > (In reply to comment #21) > > > (In reply to comment #20) > > > > Any progress on landing the Qt4.8 fix? > > > > > > (In reply to comment #20) > > > > Any progress on landing the Qt4.8 fix? > > > > > > The problem is that it's technically a behavior change, and it's a bit against the policy to land behavior changes in patch releases in Qt, unless we're fixing a critical bug that way. So the fact that we're stuck with 4.8 makes it tricky... > > > Aren't we gonna use ICU by default for QtWebKit 2.3 though ? That should fix it for most people. > > > > I agree, I think the dependency to ICU is the best way of solving this. > > So far we are not using ICU in QtWebKit 2.3, but it might be a better solution, though I feel sad for not fixing bugs in Qt. Well it's been fixed in Qt5, it's just not something that can be applied to Qt4 nicely at this point.
(In reply to comment #23) > (In reply to comment #22) > > (In reply to comment #21) > > > (In reply to comment #20) > > > > Any progress on landing the Qt4.8 fix? > > > > > > (In reply to comment #20) > > > > Any progress on landing the Qt4.8 fix? > > > > > > The problem is that it's technically a behavior change, and it's a bit against the policy to land behavior changes in patch releases in Qt, unless we're fixing a critical bug that way. So the fact that we're stuck with 4.8 makes it tricky... > > > Aren't we gonna use ICU by default for QtWebKit 2.3 though ? That should fix it for most people. > > > > I agree, I think the dependency to ICU is the best way of solving this. > > So far we are not using ICU in QtWebKit 2.3, but it might be a better solution, though I feel sad for not fixing bugs in Qt. The upside is that QTextBoundaryFinder is getting active feature development and maintenance in Qt 5 from Ritt :)